cmd: nvedit: add whitelist option for env import

Message ID 20180104103915.23165-1-quentin.schulz@free-electrons.com
State New
Headers show
Series
  • cmd: nvedit: add whitelist option for env import
Related show

Commit Message

Quentin Schulz Jan. 4, 2018, 10:39 a.m.
While the `env export` can take as parameters variables to be exported,
`env import` does not have such a mechanism of variable selection.

Let's add a `-w` option that asks `env import` to look for the
`whitelisted_vars` env variable for a space-separated list of variables
that are whitelisted.

Every env variable present in env at `addr` and in `whitelisted_vars`
env variable will override the value of the variable in the current env.
All the remaining variables are left untouched.

One of its use case could be to load a secure environment from the
signed U-Boot binary and load only a handful of variables from an
other, unsecure, environment without completely losing control of
U-Boot.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 cmd/nvedit.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 7 deletions(-)

Comments

Quentin Schulz March 2, 2018, 3:35 p.m. | #1
Hi all,

On Thu, Jan 04, 2018 at 11:39:15AM +0100, Quentin Schulz wrote:
> While the `env export` can take as parameters variables to be exported,

> `env import` does not have such a mechanism of variable selection.

> 

> Let's add a `-w` option that asks `env import` to look for the

> `whitelisted_vars` env variable for a space-separated list of variables

> that are whitelisted.

> 

> Every env variable present in env at `addr` and in `whitelisted_vars`

> env variable will override the value of the variable in the current env.

> All the remaining variables are left untouched.

> 

> One of its use case could be to load a secure environment from the

> signed U-Boot binary and load only a handful of variables from an

> other, unsecure, environment without completely losing control of

> U-Boot.

> 

> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>


It's been two months since I posted this patch and there hasn't been any
review.

Is there something to improve?
Is there an other approach to take?

I'm all ears.

Thanks,
Quentin

> ---

>  cmd/nvedit.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------

>  1 file changed, 64 insertions(+), 7 deletions(-)

> 

> diff --git a/cmd/nvedit.c b/cmd/nvedit.c

> index 4e79d03856..9f983b41a1 100644

> --- a/cmd/nvedit.c

> +++ b/cmd/nvedit.c

> @@ -971,7 +971,7 @@ sep_err:

>  

>  #ifdef CONFIG_CMD_IMPORTENV

>  /*

> - * env import [-d] [-t [-r] | -b | -c] addr [size]

> + * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]

>   *	-d:	delete existing environment before importing;

>   *		otherwise overwrite / append to existing definitions

>   *	-t:	assume text format; either "size" must be given or the

> @@ -982,6 +982,10 @@ sep_err:

>   *		for line endings. Only effective in addition to -t.

>   *	-b:	assume binary format ('\0' separated, "\0\0" terminated)

>   *	-c:	assume checksum protected environment format

> + *	-w:	specify that whitelisting of variables should be used when

> + *		importing environment. The space-separated list of variables

> + *		that should override the ones in current environment is stored

> + *		in `whitelisted_vars`.

>   *	addr:	memory address to read from

>   *	size:	length of input data; if missing, proper '\0'

>   *		termination is mandatory

> @@ -991,11 +995,16 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,

>  {

>  	ulong	addr;

>  	char	*cmd, *ptr;

> +	char	**array = NULL;

>  	char	sep = '\n';

>  	int	chk = 0;

>  	int	fmt = 0;

>  	int	del = 0;

>  	int	crlf_is_lf = 0;

> +	int	wl = 0;

> +	int	wl_count = 0;

> +	int ret;

> +	unsigned int i;

>  	size_t	size;

>  

>  	cmd = *argv;

> @@ -1026,6 +1035,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,

>  			case 'd':

>  				del = 1;

>  				break;

> +			case 'w':

> +				wl = 1;

> +				break;

>  			default:

>  				return CMD_RET_USAGE;

>  			}

> @@ -1082,14 +1094,59 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,

>  		ptr = (char *)ep->data;

>  	}

>  

> -	if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,

> -			crlf_is_lf, 0, NULL) == 0) {

> +	if(wl) {

> +		char *str, *token, *tmp;

> +		wl_count = 1;

> +

> +		str = env_get("whitelisted_vars");

> +		if (!str) {

> +			puts("## Error: whitelisted_vars is not set.\n");

> +			return CMD_RET_USAGE;

> +		}

> +

> +		tmp = malloc(sizeof(char) * (strlen(str) + 1));

> +		strcpy(tmp, str);

> +

> +		token = strchr(tmp, ' ');

> +		while (!token) {

> +			wl_count++;

> +			token = strchr(token + 1, ' ');

> +		}

> +

> +		strcpy(tmp, str);

> +

> +		array = malloc(sizeof(char *) * wl_count);

> +		wl_count = 0;

> +

> +		token = strtok(tmp, " ");

> +		while (token) {

> +			array[wl_count] = malloc(sizeof(char) *

> +						 (strlen(token) + 1));

> +			strcpy(array[wl_count], token);

> +			wl_count++;

> +			token = strtok(NULL, " ");

> +		}

> +

> +		free(tmp);

> +	}

> +

> +	ret = himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,

> +			crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL);

> +	if (!ret) {

>  		pr_err("Environment import failed: errno = %d\n", errno);

> -		return 1;

> +		ret = 1;

> +	} else {

> +		gd->flags |= GD_FLG_ENV_READY;

> +		ret = 0;

>  	}

> -	gd->flags |= GD_FLG_ENV_READY;

>  

> -	return 0;

> +	if (wl) {

> +		for (i = 0; i < wl_count; i++)

> +			free(array[i]);

> +		free(array);

> +	}

> +

> +	return ret;

>  

>  sep_err:

>  	printf("## %s: only one of \"-b\", \"-c\" or \"-t\" allowed\n",

> @@ -1212,7 +1269,7 @@ static char env_help_text[] =

>  #endif

>  #endif

>  #if defined(CONFIG_CMD_IMPORTENV)

> -	"env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n"

> +	"env import [-d] [-t [-r] | -b | -c] [-w] addr [size] - import environment\n"

>  #endif

>  	"env print [-a | name ...] - print environment\n"

>  #if defined(CONFIG_CMD_RUN)

> -- 

> 2.14.1

>

Patch

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 4e79d03856..9f983b41a1 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -971,7 +971,7 @@  sep_err:
 
 #ifdef CONFIG_CMD_IMPORTENV
 /*
- * env import [-d] [-t [-r] | -b | -c] addr [size]
+ * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
  *	-d:	delete existing environment before importing;
  *		otherwise overwrite / append to existing definitions
  *	-t:	assume text format; either "size" must be given or the
@@ -982,6 +982,10 @@  sep_err:
  *		for line endings. Only effective in addition to -t.
  *	-b:	assume binary format ('\0' separated, "\0\0" terminated)
  *	-c:	assume checksum protected environment format
+ *	-w:	specify that whitelisting of variables should be used when
+ *		importing environment. The space-separated list of variables
+ *		that should override the ones in current environment is stored
+ *		in `whitelisted_vars`.
  *	addr:	memory address to read from
  *	size:	length of input data; if missing, proper '\0'
  *		termination is mandatory
@@ -991,11 +995,16 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 {
 	ulong	addr;
 	char	*cmd, *ptr;
+	char	**array = NULL;
 	char	sep = '\n';
 	int	chk = 0;
 	int	fmt = 0;
 	int	del = 0;
 	int	crlf_is_lf = 0;
+	int	wl = 0;
+	int	wl_count = 0;
+	int ret;
+	unsigned int i;
 	size_t	size;
 
 	cmd = *argv;
@@ -1026,6 +1035,9 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 			case 'd':
 				del = 1;
 				break;
+			case 'w':
+				wl = 1;
+				break;
 			default:
 				return CMD_RET_USAGE;
 			}
@@ -1082,14 +1094,59 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 		ptr = (char *)ep->data;
 	}
 
-	if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
-			crlf_is_lf, 0, NULL) == 0) {
+	if(wl) {
+		char *str, *token, *tmp;
+		wl_count = 1;
+
+		str = env_get("whitelisted_vars");
+		if (!str) {
+			puts("## Error: whitelisted_vars is not set.\n");
+			return CMD_RET_USAGE;
+		}
+
+		tmp = malloc(sizeof(char) * (strlen(str) + 1));
+		strcpy(tmp, str);
+
+		token = strchr(tmp, ' ');
+		while (!token) {
+			wl_count++;
+			token = strchr(token + 1, ' ');
+		}
+
+		strcpy(tmp, str);
+
+		array = malloc(sizeof(char *) * wl_count);
+		wl_count = 0;
+
+		token = strtok(tmp, " ");
+		while (token) {
+			array[wl_count] = malloc(sizeof(char) *
+						 (strlen(token) + 1));
+			strcpy(array[wl_count], token);
+			wl_count++;
+			token = strtok(NULL, " ");
+		}
+
+		free(tmp);
+	}
+
+	ret = himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
+			crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL);
+	if (!ret) {
 		pr_err("Environment import failed: errno = %d\n", errno);
-		return 1;
+		ret = 1;
+	} else {
+		gd->flags |= GD_FLG_ENV_READY;
+		ret = 0;
 	}
-	gd->flags |= GD_FLG_ENV_READY;
 
-	return 0;
+	if (wl) {
+		for (i = 0; i < wl_count; i++)
+			free(array[i]);
+		free(array);
+	}
+
+	return ret;
 
 sep_err:
 	printf("## %s: only one of \"-b\", \"-c\" or \"-t\" allowed\n",
@@ -1212,7 +1269,7 @@  static char env_help_text[] =
 #endif
 #endif
 #if defined(CONFIG_CMD_IMPORTENV)
-	"env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n"
+	"env import [-d] [-t [-r] | -b | -c] [-w] addr [size] - import environment\n"
 #endif
 	"env print [-a | name ...] - print environment\n"
 #if defined(CONFIG_CMD_RUN)