[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: udhcpc6 expects string for bootfile-param opt(60)
From: Geoff Hanson <ghanson () arista ! com>
Date: 2022-05-12 17:44:37
Message-ID: CAHSaY_TO0WykGUpwwHweYHwH_qXdC8atWeZzkZvYZo22PTAgyQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Just wanted to follow up on this again.
If there's no further comments on the patch, could someone consider
integrating it?
I've re-attached the patch.
Thanks,
Geoff
On Wed, Feb 23, 2022 at 8:14 AM Geoff Hanson <ghanson@arista.com> wrote:
> Just following up on this patch. Are there any more comments on this?
>
> Thanks,
> Geoff
>
> On Tue, Feb 8, 2022 at 11:58 AM Geoff Hanson <ghanson@arista.com> wrote:
>
>> Any further feedback on this?
>>
>> Anything more I need to do or is what I've provided sufficient for the
>> bug report?
>>
>> Thanks,
>> Geoff
>>
>> On Tue, Feb 1, 2022 at 12:53 PM Geoff Hanson <ghanson@arista.com> wrote:
>>
>>> Hi Bernd. Can you look at my second attachment? As part of addressing
>>> the issue Xabier reported,
>>> I switched to using memcpy.
>>>
>>> Thanks,
>>> Geoff
>>>
>>> On Tue, Feb 1, 2022 at 12:36 PM Bernd Petrovitsch <
>>> bernd@petrovitsch.priv.at> wrote:
>>>
>>>> -Hi all!
>>>>
>>>> On 01.02.2022 18:12, Geoff Hanson wrote:
>>>> [...]> In most cases, there's no printf directive so this just means
>>>> it's
>>>> > copying the string.
>>>>
>>>> Using some user-provided string as a format-string opens the
>>>> possibility
>>>> ofexploits - since decades ....
>>>> > But this would cause problems in the case where the string did
>>>> contain %'s.
>>>>
>>>> So why just not only use strncpy(), strlcpy(), memcpy() or similar?
>>>>
>>>> Kind regards,
>>>> Bernd
>>>>
>>>
[Attachment #5 (text/html)]
<div dir="ltr"><div>Just wanted to follow up on this \
again.</div><div><br></div><div>If there's no further comments on the patch, \
could someone consider integrating it?</div><div><br></div><div>I've re-attached \
the patch.</div><div><br></div><div>Thanks,</div><div>Geoff<br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 23, 2022 at 8:14 AM \
Geoff Hanson <<a href="mailto:ghanson@arista.com">ghanson@arista.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div \
dir="ltr"><div>Just following up on this patch. Are there any more comments on \
this?</div><div><br></div><div>Thanks,</div><div>Geoff<br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 8, 2022 at 11:58 AM \
Geoff Hanson <<a href="mailto:ghanson@arista.com" \
target="_blank">ghanson@arista.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Any further feedback on \
this?</div><div><br></div><div>Anything more I need to do or is what I've \
provided sufficient for the bug \
report?</div><div><br></div><div>Thanks,</div><div>Geoff<br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 1, 2022 at 12:53 PM \
Geoff Hanson <<a href="mailto:ghanson@arista.com" \
target="_blank">ghanson@arista.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi Bernd. Can you look at my \
second attachment? As part of addressing the issue Xabier reported,</div><div>I \
switched to using memcpy.</div><div><br></div><div>Thanks,</div><div>Geoff<br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 1, 2022 at 12:36 PM \
Bernd Petrovitsch <<a href="mailto:bernd@petrovitsch.priv.at" \
target="_blank">bernd@petrovitsch.priv.at</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">-Hi all!<br> <br>
On 01.02.2022 18:12, Geoff Hanson wrote:<br>
[...]> In most cases, there's no printf directive so this just means \
it's<br> > copying the string.<br>
<br>
Using some user-provided string as a format-string opens the possibility <br>
ofexploits - since decades ....<br>
> But this would cause problems in the case where the string did contain \
%'s.<br> <br>
So why just not only use strncpy(), strlcpy(), memcpy() or similar?<br>
<br>
Kind regards,<br>
Bernd<br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
["bootfile-param_patch.diff.txt" (text/plain)]
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];
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic