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

[Laurent Pinchart]

Hi Kieran,

On Fri, Jun 18, 2021 at 09:43:34PM +0100, Kieran Bingham wrote:
> 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 ;-)

No, you're right, 'exception' isn't right. I'll change this.

> 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..

Maybe Android doesn't care about security ? ;-) No, to be fair, camera
access goes through the camera service, so applications can't inject an
arbitrary environment variable.

> 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,

Laurent Pinchart