[libcamera-devel,v2,4/4] libcamera: utils: Use internal secure_getenv() implementation
diff mbox series

Message ID 20190401110315.4148-5-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • Cleanup and non-GNU C library support
Related show

Commit Message

Kieran Bingham April 1, 2019, 11:03 a.m. UTC 
The secure_getenv() call is not provided by all toolchains. Support
this feature by implementing our own version.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/utils.h |  2 ++
 src/libcamera/log.cpp         |  4 ++--
 src/libcamera/utils.cpp       | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart April 1, 2019, 8:41 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Mon, Apr 01, 2019 at 06:03:15PM +0700, Kieran Bingham wrote:
> The secure_getenv() call is not provided by all toolchains. Support

s/toolchains/C libraries/ ?

> this feature by implementing our own version.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/utils.h |  2 ++
>  src/libcamera/log.cpp         |  4 ++--
>  src/libcamera/utils.cpp       | 20 ++++++++++++++++++++
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> index 1b2a62c0fda7..79038a96feab 100644
> --- a/src/libcamera/include/utils.h
> +++ b/src/libcamera/include/utils.h
> @@ -24,6 +24,8 @@ std::unique_ptr<T> make_unique(Args&&... args)
>  	return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
>  }
>  
> +char *secure_getenv(const char *name);
> +
>  } /* namespace utils */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index eb444c31857d..71cfbc422ba0 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -122,7 +122,7 @@ Logger::Logger()
>   */
>  void Logger::parseLogFile()
>  {
> -	const char *file = secure_getenv("LIBCAMERA_LOG_FILE");
> +	const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>  	if (!file)
>  		return;
>  
> @@ -140,7 +140,7 @@ void Logger::parseLogFile()
>   */
>  void Logger::parseLogLevels()
>  {
> -	const char *debug = secure_getenv("LIBCAMERA_LOG_LEVELS");
> +	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
>  	if (!debug)
>  		return;
>  
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 70936e36c5d5..c49e65136514 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <string.h>
> +#include <sys/auxv.h>
>  
>  #include "utils.h"
>  
> @@ -35,6 +36,25 @@ const char *basename(const char *path)
>         return base ? base + 1 : path;
>  }
>  
> +/**
> + * \brief Get an environment variable
> + *
> + * The environment list is searched to find the variable 'name', and returns a
> + * pointer to the corresponding string.

The first half of the sentence doesn't seem to provide a subject for
"returns" in the second half.

> + * If 'secure execution' is required then this function always returns NULL to
> + * avoid vulnerabilities that could occur if the set-user-ID or set-group-ID

s/if the/if/

> + * programs accidentally trusted the environment.
> + *
> + * \returns A pointer to the value in the environment or NULL if the match fails
> + * or a secure environment is required.

"..., or NULL if the requested environment variable doesn't exist or if
secure execution is required." ?

> + */
> +char *secure_getenv(const char *name)
> +{
> +       if (getauxval(AT_SECURE))
> +               return NULL;
> +       else

You can drop the else.

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +               return getenv(name);
> +}
>  
>  /**
>   * \fn libcamera::utils::make_unique(Args &&... args)
Kieran Bingham April 3, 2019, 3:54 a.m. UTC | #2 
Hi Laurent,

On 01/04/2019 21:41, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Mon, Apr 01, 2019 at 06:03:15PM +0700, Kieran Bingham wrote:
>> The secure_getenv() call is not provided by all toolchains. Support
> 
> s/toolchains/C libraries/ ?

Sure. Also changed in the documentation of basename().

>> this feature by implementing our own version.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/include/utils.h |  2 ++
>>  src/libcamera/log.cpp         |  4 ++--
>>  src/libcamera/utils.cpp       | 20 ++++++++++++++++++++
>>  3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>> index 1b2a62c0fda7..79038a96feab 100644
>> --- a/src/libcamera/include/utils.h
>> +++ b/src/libcamera/include/utils.h
>> @@ -24,6 +24,8 @@ std::unique_ptr<T> make_unique(Args&&... args)
>>  	return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
>>  }
>>  
>> +char *secure_getenv(const char *name);
>> +
>>  } /* namespace utils */
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>> index eb444c31857d..71cfbc422ba0 100644
>> --- a/src/libcamera/log.cpp
>> +++ b/src/libcamera/log.cpp
>> @@ -122,7 +122,7 @@ Logger::Logger()
>>   */
>>  void Logger::parseLogFile()
>>  {
>> -	const char *file = secure_getenv("LIBCAMERA_LOG_FILE");
>> +	const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>>  	if (!file)
>>  		return;
>>  
>> @@ -140,7 +140,7 @@ void Logger::parseLogFile()
>>   */
>>  void Logger::parseLogLevels()
>>  {
>> -	const char *debug = secure_getenv("LIBCAMERA_LOG_LEVELS");
>> +	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
>>  	if (!debug)
>>  		return;
>>  
>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>> index 70936e36c5d5..c49e65136514 100644
>> --- a/src/libcamera/utils.cpp
>> +++ b/src/libcamera/utils.cpp
>> @@ -6,6 +6,7 @@
>>   */
>>  
>>  #include <string.h>
>> +#include <sys/auxv.h>
>>  
>>  #include "utils.h"
>>  
>> @@ -35,6 +36,25 @@ const char *basename(const char *path)
>>         return base ? base + 1 : path;
>>  }
>>  
>> +/**
>> + * \brief Get an environment variable
>> + *
>> + * The environment list is searched to find the variable 'name', and returns a
>> + * pointer to the corresponding string.
> 
> The first half of the sentence doesn't seem to provide a subject for
> "returns" in the second half.

Reworded to:


> /**
>  * \brief Get an environment variable
>  * \param[in] name The name of the variable to return
>  *
>  * The environment list is searched to find the variable 'name', and the
>  * corresponding string is returned.
>  *
>  * If 'secure execution' is required then this function always returns NULL to
>  * avoid vulnerabilities that could occur if set-user-ID or set-group-ID
>  * programs accidentally trust the environment.
>  *
>  * \returns A pointer to the value in the environment or NULL if the requested
>  * environment variable doesn't exist or if secure execution is required.
>  */
> char *secure_getenv(const char *name)


which also incorporates the comments below...

> 
>> + * If 'secure execution' is required then this function always returns NULL to
>> + * avoid vulnerabilities that could occur if the set-user-ID or set-group-ID
> 
> s/if the/if/
> 
>> + * programs accidentally trusted the environment.
>> + *
>> + * \returns A pointer to the value in the environment or NULL if the match fails
>> + * or a secure environment is required.
> 
> "..., or NULL if the requested environment variable doesn't exist or if
> secure execution is required." ?
> 
>> + */
>> +char *secure_getenv(const char *name)
>> +{
>> +       if (getauxval(AT_SECURE))
>> +               return NULL;
>> +       else
> 
> You can drop the else.

Done.


> With these small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +               return getenv(name);
>> +}
>>  
>>  /**
>>   * \fn libcamera::utils::make_unique(Args &&... args)

Patch
diff mbox series 

diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
index 1b2a62c0fda7..79038a96feab 100644
--- a/src/libcamera/include/utils.h
+++ b/src/libcamera/include/utils.h
@@ -24,6 +24,8 @@  std::unique_ptr<T> make_unique(Args&&... args)
 	return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
 }
 
+char *secure_getenv(const char *name);
+
 } /* namespace utils */
 
 } /* namespace libcamera */
diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
index eb444c31857d..71cfbc422ba0 100644
--- a/src/libcamera/log.cpp
+++ b/src/libcamera/log.cpp
@@ -122,7 +122,7 @@  Logger::Logger()
  */
 void Logger::parseLogFile()
 {
-	const char *file = secure_getenv("LIBCAMERA_LOG_FILE");
+	const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
 	if (!file)
 		return;
 
@@ -140,7 +140,7 @@  void Logger::parseLogFile()
  */
 void Logger::parseLogLevels()
 {
-	const char *debug = secure_getenv("LIBCAMERA_LOG_LEVELS");
+	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
 	if (!debug)
 		return;
 
diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
index 70936e36c5d5..c49e65136514 100644
--- a/src/libcamera/utils.cpp
+++ b/src/libcamera/utils.cpp
@@ -6,6 +6,7 @@ 
  */
 
 #include <string.h>
+#include <sys/auxv.h>
 
 #include "utils.h"
 
@@ -35,6 +36,25 @@  const char *basename(const char *path)
        return base ? base + 1 : path;
 }
 
+/**
+ * \brief Get an environment variable
+ *
+ * The environment list is searched to find the variable 'name', and returns a
+ * pointer to the corresponding string.
+ * If 'secure execution' is required then this function always returns NULL to
+ * avoid vulnerabilities that could occur if the set-user-ID or set-group-ID
+ * programs accidentally trusted the environment.
+ *
+ * \returns A pointer to the value in the environment or NULL if the match fails
+ * or a secure environment is required.
+ */
+char *secure_getenv(const char *name)
+{
+       if (getauxval(AT_SECURE))
+               return NULL;
+       else
+               return getenv(name);
+}
 
 /**
  * \fn libcamera::utils::make_unique(Args &&... args)