udhcpc6 expects string for bootfile-param opt(60)

[Geoff Hanson]

Hi Xabier. Thanks for reviewing it.

In most cases, there's no printf directive so this just means it's copying
the string.
But this would cause problems in the case where the string did contain %'s.

I've attached a new patch with an alternate way of copying the parameters.

Thanks,
Geoff

On Mon, Jan 31, 2022 at 11:09 AM Xabier Oneca -- xOneca <xoneca at gmail.com>
wrote:

> Hi Geoff,
>
> > [...]
> > + if (envptr > envval)
> > +    *envptr++ = ' ';
> > + snprintf(envptr, parm_len + 1, optptr);
>
> You forgot the format string (i.e. "%s").
>
> > + envptr += parm_len;
> > + optptr += parm_len;
> > [...]
>
> Cheers,
>
> Xabier Oneca_,,_
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20220201/d804dbf8/attachment.html>
-------------- next part --------------
Index: busybox-1.33.1/networking/udhcp/d6_dhcpc.c
===================================================================
--- busybox-1.33.1.orig/networking/udhcp/d6_dhcpc.c
+++ busybox-1.33.1/networking/udhcp/d6_dhcpc.c
@@ -79,7 +79,7 @@ static const struct dhcp_optflag d6_optf
 #endif
 #if ENABLE_FEATURE_UDHCPC6_RFC5970
 	{ OPTION_STRING,                                D6_OPT_BOOT_URL },
-	{ OPTION_STRING,                                D6_OPT_BOOT_PARAM },
+	{ OPTION_STRING | OPTION_LIST,                  D6_OPT_BOOT_PARAM },
 #endif
 	{ OPTION_STRING,                                0xd1 }, /* DHCP_PXE_CONF_FILE */
 	{ OPTION_STRING,                                0xd2 }, /* DHCP_PXE_PATH_PREFIX */
@@ -214,19 +214,16 @@ static char** new_env(void)
 	return &client6_data.env_ptr[client6_data.env_idx++];
 }
 
-static char *string_option_to_env(const uint8_t *option,
-		const uint8_t *option_end)
+static const char *get_option_name(const uint8_t *option)
 {
-	const char *ptr, *name = NULL;
-	unsigned val_len;
+	const char *ptr = NULL;
 	int i;
 
 	ptr = d6_option_strings;
 	i = 0;
 	while (*ptr) {
 		if (d6_optflags[i].code == option[1]) {
-			name = ptr;
-			goto found;
+			return ptr;
 		}
 		ptr += strlen(ptr) + 1;
 		i++;
@@ -234,7 +231,19 @@ static char *string_option_to_env(const
 	bb_error_msg("can't find option name for 0x%x, skipping", option[1]);
 	return NULL;
 
- found:
+}
+
+static char *string_option_to_env(const uint8_t *option,
+		const uint8_t *option_end)
+{
+	const char *name = NULL;
+	unsigned val_len;
+
+	name = get_option_name(option);
+	if (!name) {
+		return NULL;
+	}
+
 	val_len = (option[2] << 8) | option[3];
 	if (val_len + &option[D6_OPT_DATA] > option_end) {
 		bb_simple_error_msg("option data exceeds option length");
@@ -243,6 +252,52 @@ static char *string_option_to_env(const
 	return xasprintf("%s=%.*s", name, val_len, (char*)option + 4);
 }
 
+/* parse list of variable length strings. The length of each string
+   is the first two bytes */
+static char *string_list_option_to_env(const uint8_t *option,
+        const uint8_t *option_end)
+{
+	const char *name = NULL;
+	unsigned val_len, name_len, parm_len;
+	int i;
+	char *envstr, *envptr, *envval;
+	const uint8_t *optptr;
+
+	name = get_option_name(option);
+	if (!name)
+		return NULL;
+	name_len = strlen(name);
+
+	val_len = (option[2] << 8) | option[3];
+	if (val_len + &option[D6_OPT_DATA] > option_end) {
+		bb_simple_error_msg("option data exceeds option length");
+		return NULL;
+	}
+
+	envptr = envstr = xmalloc(name_len + 1 + val_len);
+	envptr = stpcpy(envptr, name);
+	envval = envptr = stpcpy(envptr, "=");
+
+	optptr = option + D6_OPT_DATA;
+	while (optptr + 1 < option_end) {
+		parm_len = (*optptr++ << 8) | *optptr++;
+		if (optptr + parm_len > option_end) {
+			bb_simple_error_msg("option parm length overflows option length");
+			return NULL;
+		}
+		if (parm_len > 0) {
+			if (envptr > envval)
+			    *envptr++ = ' ';
+			memcpy(envptr, optptr, parm_len);
+			envptr += parm_len;
+			optptr += parm_len;
+			*envptr = '\0';
+		}
+	}
+
+	return envstr;
+}
+
 /* put all the parameters into the environment */
 static void option_to_env(const uint8_t *option, const uint8_t *option_end)
 {
@@ -407,7 +462,6 @@ static void option_to_env(const uint8_t
 			break;
 #endif
 		case D6_OPT_BOOT_URL:
-		case D6_OPT_BOOT_PARAM:
 		case 0xd1: /* DHCP_PXE_CONF_FILE */
 		case 0xd2: /* DHCP_PXE_PATH_PREFIX */
 			{
@@ -415,6 +469,38 @@ static void option_to_env(const uint8_t
 			if (tmp)
 				*new_env() = tmp;
 			break;
+			}
+		case D6_OPT_BOOT_PARAM:
+/*  0                   1                   2                   3
+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |       OPT_BOOTFILE_PARAM      |            option-len         |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | param-len 1                   |                               |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+           parameter 1         .
+ * .                                        (variable length)      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * .                                                               .
+ * .                       <multiple Parameters>                   .
+ * .                                                               .
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | param-len n                   |                               |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+           parameter n         .
+ * .                                        (variable length)      |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+			{
+			char *tmp;
+			if (option[4])
+				/* If the high byte of param-len 1 is non-zero, most
+				   likely this was caused by a non-compliant server
+				   sending the param as a single string. */
+				tmp = string_option_to_env(option, option_end);
+			else
+				tmp = string_list_option_to_env(option, option_end);
+			if (tmp)
+				*new_env() = tmp;
+			break;
 			}
 		}
 		len_m4 -= 4 + option[3];