[libcamera-devel] [PATCH] libcamera: utils: Support systems that lack secure_getenv and issetugid

[Kieran Bingham]

Hi Laurent,

On 16/06/2021 16:52, Laurent Pinchart wrote:
> Android provides neither secure_getenv() nor issetugid(). Enable
> compilation on that platform by using a plain getenv(), as that seems to
> be the best we can do.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  meson.build             | 4 ++++
>  src/libcamera/utils.cpp | 7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index f6ab7380f1a5..4d7d936f09e2 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -33,6 +33,10 @@ if cc.has_header_symbol('execinfo.h', 'backtrace')
>      config_h.set('HAVE_BACKTRACE', 1)
>  endif
>  
> +if cc.has_header_symbol('unistd.h', 'issetugid')
> +    config_h.set('HAVE_ISSETUGID', 1)
> +endif
> +
>  if cc.has_header_symbol('stdlib.h', 'secure_getenv', prefix : '#define _GNU_SOURCE')
>      config_h.set('HAVE_SECURE_GETENV', 1)
>  endif
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 24a5c9720fbe..ed7e0177ebe1 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -61,6 +61,10 @@ const char *basename(const char *path)
>   * avoid vulnerabilities that could occur if set-user-ID or set-group-ID
>   * programs accidentally trust the environment.
>   *
> + * \note Not all platforms may support the features required to implement the
> + * secure execution check, in which case this function behaves as getenv(). A
> + * notable exception is Android.

This almost reads like Android is an exception, of the previous
statement which would be in the inverse of your intention. Perhaps
instead of 'exception' 'A notable example of this is Android'?

But it might also just be me reading it differently ;-)

secure_getenv is a pain though isn't it. We want to parse environment
variables, and it seems fairly impossible to do it generically across
all systems in a way that is actually secure.

But .. I agree, we can't call issetugid if we don't have it..

a somewhat strenuous (because I hate this function, not the patch)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> + *
>   * \return A pointer to the value in the environment or NULL if the requested
>   * environment variable doesn't exist or if secure execution is required.
>   */
> @@ -69,9 +73,10 @@ char *secure_getenv(const char *name)
>  #if HAVE_SECURE_GETENV
>  	return ::secure_getenv(name);
>  #else
> +#if HAVE_ISSETUGID
>  	if (issetugid())
>  		return NULL;
> -
> +#endif
>  	return getenv(name);
>  #endif
>  }
> 

-- 
Regards
--
Kieran