[PATCH] net conf: add parameter value check to 'net conf setparm'

Michael Adam obnox at samba.org
Wed Nov 23 10:41:35 UTC 2016


Hi Ralph,

good catch and nice patches, thank you!

Only thought I had was this:

wouldn't it be natural to put the call to
lp_parameter_value_is_valid()
into lp_canonicalize_parameter_with_value()
which is already doing some value sanitation?

This is also called from smbconf_reg_set_value(),
so that other clients than "net conf" would
also benefit from the change.

See the attached add-on patchset.

Since your patch has not landed yet, this
could be combined with your patchset so as to
avoid doing the back and forth business.
Attached second patchset demonstrates how
this could look like.

Finally: I guess we need tests for this. :-)
With the addon-patches, we should be able to
do this in source3/lib/smbconf/testsuite.c.

Cheers - Michael

On 2016-11-22 at 15:24 +0100, Ralph Wuerthner wrote:
> Hi!
> 
> Please see attached patchset to prevent bricking your registry like this:
> 
>   $ net conf setparm global 'server max protocol' foo
>   $ net conf setparm global 'server max protocol' SMB3
>   WARNING: Ignoring invalid value 'foo' for parameter 'server max protocol'
>   $ net conf setparm global 'server max protocol' SMB3
>   WARNING: Ignoring invalid value 'foo' for parameter 'server max protocol'
> 
> 
> Regards
> 
>     Ralph
> 

> From 0d15762e149914286470ec8929fab3a6c14e6b69 Mon Sep 17 00:00:00 2001
> From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> Date: Tue, 22 Nov 2016 11:20:22 +0100
> Subject: [PATCH 1/3] net conf: fix error message
> 
> Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> ---
>  source3/utils/net_conf_util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/source3/utils/net_conf_util.c b/source3/utils/net_conf_util.c
> index a188097..ec6a479 100644
> --- a/source3/utils/net_conf_util.c
> +++ b/source3/utils/net_conf_util.c
> @@ -61,7 +61,7 @@ bool net_conf_param_valid(const char *service,
>  		 * So the value must be invalid.
>  		 */
>  		d_fprintf(stderr, "invalid value '%s' given for "
> -			  "parameter '%s'\n", param, valstr);
> +			  "parameter '%s'\n", valstr, param);
>  		return false;
>  	}
>  
> -- 
> 1.9.1
> 
> 
> From 2b412ee25ade38c6ba94e4e87118c1fae4db11f6 Mon Sep 17 00:00:00 2001
> From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> Date: Mon, 21 Nov 2016 14:56:52 +0100
> Subject: [PATCH 2/3] param: add lp_parameter_value_is_valid() function
> 
> Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> ---
>  source3/include/proto.h  |  1 +
>  source3/param/loadparm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index 33e3f6c..f5f1414 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -934,6 +934,7 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
>  					  const char *val,
>  					  const char **canon_parm,
>  					  const char **canon_val);
> +bool lp_parameter_value_is_valid(const char *parm_name, const char *val);
>  void show_parameter_list(void);
>  bool lp_invert_boolean(const char *str, const char **inverse_str);
>  bool lp_canonicalize_boolean(const char *str, const char**canon_str);
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 3e1a15e..7bb993c 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -1852,6 +1852,71 @@ static void show_parameter(int parmIndex)
>  	printf("\n");
>  }
>  
> +/*
> + * Check the value for a P_ENUM
> + */
> +static bool check_enum_parameter(struct parm_struct *parm, const char *value)
> +{
> +	int i;
> +
> +	for (i = 0; parm->enum_list[i].name; i++) {
> +		if (strwicmp(value, parm->enum_list[i].name) == 0) {
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +/**************************************************************************
> + Check whether the given value is valid for the given parameter name.
> +**************************************************************************/
> +
> +bool lp_parameter_value_is_valid(const char *parm_name, const char *val)
> +{
> +	bool ret = false, tmp_bool;
> +	int num = lpcfg_map_parameter(parm_name), tmp_int;
> +	uint64_t tmp_int64 = 0;
> +	struct parm_struct *parm;
> +
> +	if (num >= 0) {
> +		parm = &parm_table[num];
> +		switch (parm->type) {
> +			case P_BOOL:
> +			case P_BOOLREV:
> +				ret = set_boolean(val, &tmp_bool);
> +				break;
> +
> +			case P_INTEGER:
> +				ret = (sscanf(val, "%d", &tmp_int) == 1);
> +				break;
> +
> +			case P_OCTAL:
> +				ret = (sscanf(val, "%o", &tmp_int) == 1);
> +				break;
> +
> +			case P_ENUM:
> +				ret = check_enum_parameter(parm, val);
> +				break;
> +
> +			case P_BYTES:
> +				if (conv_str_size_error(val, &tmp_int64) &&
> +				    tmp_int64 <= INT_MAX) {
> +					ret = true;
> +				}
> +				break;
> +
> +			case P_CHAR:
> +			case P_LIST:
> +			case P_STRING:
> +			case P_USTRING:
> +			case P_CMDLIST:
> +				ret = true;
> +				break;
> +		}
> +	}
> +	return ret;
> +}
> +
>  /***************************************************************************
>   Show all parameter's name, type, [values,] and flags.
>  ***************************************************************************/
> -- 
> 1.9.1
> 
> 
> From f45848b50c5ce596bd7cdfc9cf2171160c5437c3 Mon Sep 17 00:00:00 2001
> From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> Date: Mon, 21 Nov 2016 15:00:04 +0100
> Subject: [PATCH 3/3] net conf: add parameter value check to
>  net_conf_param_valid()
> 
> Check a new paramter value prior the actual update to prevent bricking
> your registry like this:
> 
>   $ net conf setparm global 'server max protocol' foo
>   $ net conf setparm global 'server max protocol' SMB3
>   WARNING: Ignoring invalid value 'foo' for parameter 'server max protocol'
>   $ net conf setparm global 'server max protocol' SMB3
>   WARNING: Ignoring invalid value 'foo' for parameter 'server max protocol'
> 
> Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> ---
>  source3/utils/net_conf_util.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/source3/utils/net_conf_util.c b/source3/utils/net_conf_util.c
> index ec6a479..a02a721 100644
> --- a/source3/utils/net_conf_util.c
> +++ b/source3/utils/net_conf_util.c
> @@ -65,5 +65,12 @@ bool net_conf_param_valid(const char *service,
>  		return false;
>  	}
>  
> +	if (!lp_parameter_value_is_valid(canon_param, canon_valstr))
> +	{
> +		d_fprintf(stderr, "invalid value '%s' given for "
> +			  "parameter '%s'\n", valstr, param);
> +		return false;
> +	}
> +
>  	return true;
>  }
> -- 
> 1.9.1
> 

-------------- next part --------------
From 88660c6bbc52643ed319754298ff3490a18b2da5 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Nov 2016 11:12:42 +0100
Subject: [PATCH 1/4] param: use early return in
 lp_canonicalize_parameter_with_value()

This reduces the indentation and streamlines the flow.
View with "git show -w" to see it's mostly indentation change.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/param/loadparm.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 7bb993c..ee8a585 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1712,16 +1712,17 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 		/* parametric option */
 		*canon_parm = parm_name;
 		*canon_val = val;
-	} else {
-		*canon_parm = parm_table[num].label;
-		if (inverse) {
-			if (!lp_invert_boolean(val, canon_val)) {
-				*canon_val = NULL;
-				return false;
-			}
-		} else {
-			*canon_val = val;
+		return true;
+	}
+
+	*canon_parm = parm_table[num].label;
+	if (inverse) {
+		if (!lp_invert_boolean(val, canon_val)) {
+			*canon_val = NULL;
+			return false;
 		}
+	} else {
+		*canon_val = val;
 	}
 
 	return true;
-- 
2.7.4


From b426aafa61259bdb0d112fc1bf1ef5d8e5e39584 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Nov 2016 11:14:54 +0100
Subject: [PATCH 2/4] param: validate value in
 lp_canonicalize_parameter_with_value()

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/param/loadparm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index ee8a585..7aa2c99 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1700,6 +1700,7 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 {
 	int num;
 	bool inverse;
+	bool ret;
 
 	if (!lp_parameter_is_valid(parm_name)) {
 		*canon_parm = NULL;
@@ -1725,7 +1726,9 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 		*canon_val = val;
 	}
 
-	return true;
+	ret = lp_parameter_value_is_valid(*canon_parm, *canon_val);
+
+	return ret;
 }
 
 /***************************************************************************
-- 
2.7.4


From 5020c4537d5308155230a06b59b3c09219c48f26 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Nov 2016 11:15:44 +0100
Subject: [PATCH 3/4] Revert "net conf: add parameter value check to
 net_conf_param_valid()"

Now this is checked in lp_canonicalize_parameter_with_value(),
the extra check in net_conf_param_valid() is not needed any more.

This reverts commit 5aa146a6fb201b0e720487cb05175d99f0de54d1.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/utils/net_conf_util.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/source3/utils/net_conf_util.c b/source3/utils/net_conf_util.c
index a02a721..ec6a479 100644
--- a/source3/utils/net_conf_util.c
+++ b/source3/utils/net_conf_util.c
@@ -65,12 +65,5 @@ bool net_conf_param_valid(const char *service,
 		return false;
 	}
 
-	if (!lp_parameter_value_is_valid(canon_param, canon_valstr))
-	{
-		d_fprintf(stderr, "invalid value '%s' given for "
-			  "parameter '%s'\n", valstr, param);
-		return false;
-	}
-
 	return true;
 }
-- 
2.7.4


From cdfdea539f59a77ffaca40438f1a711eb2a18d46 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Nov 2016 11:18:04 +0100
Subject: [PATCH 4/4] param: make lp_parameter_value_is_valid() static

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/include/proto.h  | 1 -
 source3/param/loadparm.c | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/source3/include/proto.h b/source3/include/proto.h
index f5f1414..33e3f6c 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -934,7 +934,6 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 					  const char *val,
 					  const char **canon_parm,
 					  const char **canon_val);
-bool lp_parameter_value_is_valid(const char *parm_name, const char *val);
 void show_parameter_list(void);
 bool lp_invert_boolean(const char *str, const char **inverse_str);
 bool lp_canonicalize_boolean(const char *str, const char**canon_str);
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 7aa2c99..a76e828 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1062,6 +1062,7 @@ static bool hash_a_service(const char *name, int number);
 static void free_service_byindex(int iService);
 static void show_parameter(int parmIndex);
 static bool is_synonym_of(int parm1, int parm2, bool *inverse);
+static bool lp_parameter_value_is_valid(const char *parm_name, const char *val);
 
 /*
  * This is a helper function for parametrical options support.  It returns a
@@ -1875,7 +1876,7 @@ static bool check_enum_parameter(struct parm_struct *parm, const char *value)
  Check whether the given value is valid for the given parameter name.
 **************************************************************************/
 
-bool lp_parameter_value_is_valid(const char *parm_name, const char *val)
+static bool lp_parameter_value_is_valid(const char *parm_name, const char *val)
 {
 	bool ret = false, tmp_bool;
 	int num = lpcfg_map_parameter(parm_name), tmp_int;
-- 
2.7.4

-------------- next part --------------
From dfaaaa676d1c1e0c1555dbc1490e14649b004fe4 Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Tue, 22 Nov 2016 11:20:22 +0100
Subject: [PATCH 1/4] net conf: fix error message

Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
---
 source3/utils/net_conf_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source3/utils/net_conf_util.c b/source3/utils/net_conf_util.c
index a188097..ec6a479 100644
--- a/source3/utils/net_conf_util.c
+++ b/source3/utils/net_conf_util.c
@@ -61,7 +61,7 @@ bool net_conf_param_valid(const char *service,
 		 * So the value must be invalid.
 		 */
 		d_fprintf(stderr, "invalid value '%s' given for "
-			  "parameter '%s'\n", param, valstr);
+			  "parameter '%s'\n", valstr, param);
 		return false;
 	}
 
-- 
2.7.4


From e3d0ea85444fa0feebd167145f0c4afe6c6938a0 Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Mon, 21 Nov 2016 14:56:52 +0100
Subject: [PATCH 2/4] param: add lp_parameter_value_is_valid() function

Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
---
 source3/param/loadparm.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 3e1a15e..b5b9190 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1062,6 +1062,7 @@ static bool hash_a_service(const char *name, int number);
 static void free_service_byindex(int iService);
 static void show_parameter(int parmIndex);
 static bool is_synonym_of(int parm1, int parm2, bool *inverse);
+static bool lp_parameter_value_is_valid(const char *parm_name, const char *val);
 
 /*
  * This is a helper function for parametrical options support.  It returns a
@@ -1852,6 +1853,71 @@ static void show_parameter(int parmIndex)
 	printf("\n");
 }
 
+/*
+ * Check the value for a P_ENUM
+ */
+static bool check_enum_parameter(struct parm_struct *parm, const char *value)
+{
+	int i;
+
+	for (i = 0; parm->enum_list[i].name; i++) {
+		if (strwicmp(value, parm->enum_list[i].name) == 0) {
+			return true;
+		}
+	}
+	return false;
+}
+
+/**************************************************************************
+ Check whether the given value is valid for the given parameter name.
+**************************************************************************/
+
+static bool lp_parameter_value_is_valid(const char *parm_name, const char *val)
+{
+	bool ret = false, tmp_bool;
+	int num = lpcfg_map_parameter(parm_name), tmp_int;
+	uint64_t tmp_int64 = 0;
+	struct parm_struct *parm;
+
+	if (num >= 0) {
+		parm = &parm_table[num];
+		switch (parm->type) {
+			case P_BOOL:
+			case P_BOOLREV:
+				ret = set_boolean(val, &tmp_bool);
+				break;
+
+			case P_INTEGER:
+				ret = (sscanf(val, "%d", &tmp_int) == 1);
+				break;
+
+			case P_OCTAL:
+				ret = (sscanf(val, "%o", &tmp_int) == 1);
+				break;
+
+			case P_ENUM:
+				ret = check_enum_parameter(parm, val);
+				break;
+
+			case P_BYTES:
+				if (conv_str_size_error(val, &tmp_int64) &&
+				    tmp_int64 <= INT_MAX) {
+					ret = true;
+				}
+				break;
+
+			case P_CHAR:
+			case P_LIST:
+			case P_STRING:
+			case P_USTRING:
+			case P_CMDLIST:
+				ret = true;
+				break;
+		}
+	}
+	return ret;
+}
+
 /***************************************************************************
  Show all parameter's name, type, [values,] and flags.
 ***************************************************************************/
-- 
2.7.4


From ed434b0ff1ecb806692eac1462814aabf200cf84 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Nov 2016 11:12:42 +0100
Subject: [PATCH 3/4] param: use early return in
 lp_canonicalize_parameter_with_value()

This reduces the indentation and streamlines the flow.
View with "git show -w" to see it's mostly indentation change.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/param/loadparm.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index b5b9190..6ac7b69 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1713,16 +1713,17 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 		/* parametric option */
 		*canon_parm = parm_name;
 		*canon_val = val;
-	} else {
-		*canon_parm = parm_table[num].label;
-		if (inverse) {
-			if (!lp_invert_boolean(val, canon_val)) {
-				*canon_val = NULL;
-				return false;
-			}
-		} else {
-			*canon_val = val;
+		return true;
+	}
+
+	*canon_parm = parm_table[num].label;
+	if (inverse) {
+		if (!lp_invert_boolean(val, canon_val)) {
+			*canon_val = NULL;
+			return false;
 		}
+	} else {
+		*canon_val = val;
 	}
 
 	return true;
-- 
2.7.4


From bde5d6a7e0c4cf1571a00d8711fd7631fea13dc8 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 23 Nov 2016 11:14:54 +0100
Subject: [PATCH 4/4] param: validate value in
 lp_canonicalize_parameter_with_value()

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/param/loadparm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 6ac7b69..a76e828 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1701,6 +1701,7 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 {
 	int num;
 	bool inverse;
+	bool ret;
 
 	if (!lp_parameter_is_valid(parm_name)) {
 		*canon_parm = NULL;
@@ -1726,7 +1727,9 @@ bool lp_canonicalize_parameter_with_value(const char *parm_name,
 		*canon_val = val;
 	}
 
-	return true;
+	ret = lp_parameter_value_is_valid(*canon_parm, *canon_val);
+
+	return ret;
 }
 
 /***************************************************************************
-- 
2.7.4

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161123/f217d413/signature.sig>


More information about the samba-technical mailing list