Concatenate multiple sub-options with the same code sent by the server #404
Conversation
There was a problem hiding this comment.
This patch doesn't validate input properly so it introduces things like out-of-bounds reads, out-of-bounds writes and double-frees that can be triggered by incoming DHCP replies. For example
host0: rebinding lease of 192.168.1.128
host0: leased 192.168.1.128 for 25960 seconds
host0: adding route to 192.168.1.0/24
host0: dhcp_envoption 119: Numerical result out of range
=================================================================
==230134==ERROR: AddressSanitizer: attempting double-free on 0x502000002990 in thread T0:
#0 0x0000004a131a in free (/dhcpcd/src/dhcpcd+0x4a131a) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0)
#1 0x00000050dc66 in dhcp_envoption /dhcpcd/src/dhcp-common.c:990:7
#2 0x000000521247 in dhcp_env /dhcpcd/src/dhcp.c:1479:3
#3 0x0000005114b0 in make_env /dhcpcd/src/script.c:480:7
#4 0x0000005101a5 in script_runreason /dhcpcd/src/script.c:759:16
#5 0x0000005371c9 in ipv4_applyaddr /dhcpcd/src/ipv4.c:826:3
#6 0x0000005239e4 in dhcp_bind /dhcpcd/src/dhcp.c:2511:6
#7 0x0000005316cc in dhcp_handledhcp /dhcpcd/src/dhcp.c:3504:2
#8 0x00000052b2ca in dhcp_handlebootp /dhcpcd/src/dhcp.c:3641:2
#9 0x00000052b2ca in dhcp_packet /dhcpcd/src/dhcp.c:3711:2
#10 0x000000532e0e in dhcp_readbpf /dhcpcd/src/dhcp.c:3736:3
#11 0x0000004f370d in eloop_run_ppoll /dhcpcd/src/eloop.c:1106:5
#12 0x0000004f370d in eloop_start /dhcpcd/src/eloop.c:1228:11
#13 0x0000004edec4 in main /dhcpcd/src/dhcpcd.c:2650:6
#14 0x7f9309c1e247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 8f0d04c433960a3a1d01ec9c4612545d44a9a405)
#15 0x7f9309c1e30a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 8f0d04c433960a3a1d01ec9c4612545d44a9a405)
#16 0x000000401784 in _start (/dhcpcd/src/dhcpcd+0x401784) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0)
0x502000002990 is located 0 bytes inside of 1-byte region [0x502000002990,0x502000002991)
freed by thread T0 here:
#0 0x0000004a19d0 in realloc (/dhcpcd/src/dhcpcd+0x4a19d0) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0)
#1 0x00000050d8d0 in dhcp_envoption /dhcpcd/src/dhcp-common.c:988:13
#2 0x000000521247 in dhcp_env /dhcpcd/src/dhcp.c:1479:3
#3 0x0000005114b0 in make_env /dhcpcd/src/script.c:480:7
#4 0x0000005101a5 in script_runreason /dhcpcd/src/script.c:759:16
#5 0x0000005371c9 in ipv4_applyaddr /dhcpcd/src/ipv4.c:826:3
#6 0x0000005239e4 in dhcp_bind /dhcpcd/src/dhcp.c:2511:6
#7 0x0000005316cc in dhcp_handledhcp /dhcpcd/src/dhcp.c:3504:2
#8 0x00000052b2ca in dhcp_handlebootp /dhcpcd/src/dhcp.c:3641:2
#9 0x00000052b2ca in dhcp_packet /dhcpcd/src/dhcp.c:3711:2
#10 0x000000532e0e in dhcp_readbpf /dhcpcd/src/dhcp.c:3736:3
#11 0x0000004f370d in eloop_run_ppoll /dhcpcd/src/eloop.c:1106:5
#12 0x0000004f370d in eloop_start /dhcpcd/src/eloop.c:1228:11
#13 0x0000004edec4 in main /dhcpcd/src/dhcpcd.c:2650:6
#14 0x7f9309c1e247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 8f0d04c433960a3a1d01ec9c4612545d44a9a405)
#15 0x7f9309c1e30a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 8f0d04c433960a3a1d01ec9c4612545d44a9a405)
#16 0x000000401784 in _start (/dhcpcd/src/dhcpcd+0x401784) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0)
previously allocated by thread T0 here:
#0 0x0000004a19d0 in realloc (/dhcpcd/src/dhcpcd+0x4a19d0) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0)
#1 0x00000050d8d0 in dhcp_envoption /dhcpcd/src/dhcp-common.c:988:13
#2 0x000000521247 in dhcp_env /dhcpcd/src/dhcp.c:1479:3
#3 0x0000005114b0 in make_env /dhcpcd/src/script.c:480:7
#4 0x0000005101a5 in script_runreason /dhcpcd/src/script.c:759:16
#5 0x0000005371c9 in ipv4_applyaddr /dhcpcd/src/ipv4.c:826:3
#6 0x0000005239e4 in dhcp_bind /dhcpcd/src/dhcp.c:2511:6
#7 0x0000005316cc in dhcp_handledhcp /dhcpcd/src/dhcp.c:3504:2
#8 0x00000052b2ca in dhcp_handlebootp /dhcpcd/src/dhcp.c:3641:2
#9 0x00000052b2ca in dhcp_packet /dhcpcd/src/dhcp.c:3711:2
#10 0x000000532e0e in dhcp_readbpf /dhcpcd/src/dhcp.c:3736:3
#11 0x0000004f370d in eloop_run_ppoll /dhcpcd/src/eloop.c:1106:5
#12 0x0000004f370d in eloop_start /dhcpcd/src/eloop.c:1228:11
#13 0x0000004edec4 in main /dhcpcd/src/dhcpcd.c:2650:6
#14 0x7f9309c1e247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 8f0d04c433960a3a1d01ec9c4612545d44a9a405)
#15 0x7f9309c1e30a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 8f0d04c433960a3a1d01ec9c4612545d44a9a405)
#16 0x000000401784 in _start (/dhcpcd/src/dhcpcd+0x401784) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0)
SUMMARY: AddressSanitizer: double-free (/dhcpcd/src/dhcpcd+0x4a131a) (BuildId: ec5ebf273c5372cd40c225db2ef0c334eaa53be0) in free
==230134==ABORTING
dhcpcd_fork_cb: dhcpcd manager hungup is triggered by the following BOOTP reply
>>> hexdump(p[BOOTP])
0000 02 01 06 00 E1 66 53 BD 00 00 00 00 C0 A8 01 01 .....fS.........
0010 C0 A8 01 80 C0 A8 01 01 C0 A8 01 01 FE 67 11 DB .............g..
0020 5A F6 00 00 00 00 00 00 00 00 00 00 0E 00 1C 00 Z...............
0030 1C 00 00 00 00 00 00 00 00 00 00 00 00 1C 04 00 ................
0040 1C 00 1C 00 77 26 00 00 00 00 00 00 00 00 20 10 ....w&........ .
0050 6C 6F 63 61 6C 68 6F 73 74 01 02 00 20 00 00 00 localhost... ...
0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 35 01 ..............5.
0070 02 36 04 05 C0 FF FF 7A 33 04 00 04 00 29 00 00 .6.....z3....)..
0080 00 00 00 00 00 00 00 00 5E 00 00 00 00 00 00 00 ........^.......
0090 00 00 00 00 00 00 00 00 00 00 FF 05 00 00 00 13 ................
00a0 00 41 00 1C 00 31 01 23 E7 00 1C 00 1C 00 1C 04 .A...1.#........
00b0 00 1C 00 01 23 00 00 21 04 EF FB FF 7E 33 04 00 ....#..!....~3..
00c0 00 65 68 72 00 00 32 01 02 29 00 11 03 DD A3 3D .ehr..2..).....=
00d0 00 00 86 00 FF FF 7A 03 00 E0 00 03 3F A3 55 54 ......z.....?.UT
00e0 43 6C 05 12 FF FF FF FF 2D 00 FC 00 63 82 53 63 Cl......-...c.Sc
00f0 35 01 05 34 01 23 FF 5..4.#.
>>> p[DHCP]
<DHCP options=[message-type=ack dhcp-option-overload=35 end] |>
>>> p[BOOTP].file
b'\x00\x005\x01\x026\x04\x05\xc0\xff\xffz3\x04\x00\x04\x00)\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00^\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\x05\x00\x00\x00\x13\x00A\x00\x1c\x001\x01#\xe7\x00\x1c\x00\x1c\x00\x1c\x04\x00\x1c\x00\x01#\x00\x00!\x04\xef\xfb\xff~3\x04\x00\x00ehr\x00\x002\x01\x02)\x00\x11\x03\xdd\xa3=\x00\x00\x86\x00\xff\xffz\x03\x00\xe0\x00\x03?\xa3UTCl\x05\x12\xff\xff\xff\xff-\x00\xfc\x00'
>>> p[BOOTP].sname
b'\x0e\x00\x1c\x00\x1c\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x1c\x04\x00\x1c\x00\x1c\x00w&\x00\x00\x00\x00\x00\x00\x00\x00 \x10localhost\x01\x02\x00 \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'The compiler complains about the places where negative values like -1 aren't handled properly:
dhcp-common.c:891:8: warning: implicit conversion changes signedness: 'ssize_t' (aka 'long') to 'size_t' (aka 'unsigned long') [-Wsign-conversion]
891 | ol = dhcp_optlen(opt, ol);
| ~ ^~~~~~~~~~~~~~~~~~~~
dhcp-common.c:975:13: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
975 | eod = dgetopt(ctx, &eos, &eoc, &eol, od, ol, &oopt);
| ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
dhcp-common.c:975:13: note: place parentheses around the assignment to silence this warning
975 | eod = dgetopt(ctx, &eos, &eoc, &eol, od, ol, &oopt);
| ^
| ( )
dhcp-common.c:975:13: note: use '==' to turn this assignment into an equality comparison
975 | eod = dgetopt(ctx, &eos, &eoc, &eol, od, ol, &oopt);
| ^
| ==
dhcp-common.c:987:12: warning: implicit conversion changes signedness: 'ssize_t' (aka 'long') to 'size_t' (aka 'unsigned long') [-Wsign-conversion]
987 | eol = dhcp_optlen(eopt, eol);
| ~ ^~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
|
Could this be rebased with master? |
I think this should be a bit more optimized as @evverx mentioned |
|
After another review, I don't think we need this as we already concatenate the options via the Everyone agree or anyone disagree? @spoljak-ent @evverx |
|
@rsmarples I looked at this patch because there was a request downstream where I am to backport it and I just fuzz stuff before looking at patches. As far as I understand get_option concatenates options according to RFC 3396 and it works. This patch is supposed to also concatenate suboptions as well but I have to admit I don't remember the details. |
I don't think this one is related to RFC 3396 Basically, as it's outlined in the issue linked above, if we have 2 or more sub options in say, DHCPv4 Option 125 with the same sub-option number, we will concatenate them rather than use only the last one. For example: Now, with this patch we'd have an option 125 environment variable Last time I tested, it didn't work without this patch. |
I don't think so either. I replied to the comment where https://github.com/NetworkConfiguration/dhcpcd/blob/master/src/dhcp.c#L240 was mentioned and it does what RFC 3396 says. I don't think there are RFCs saying how suboptions should be concatenated (or whether they should be concatenated at all). Personally I think generally it would be great if patches came along with tests showing what they do and making sure they work. |
|
For the record I pulled this patch and sent Option: (125) V-I Vendor-specific Information
Length: 11
Enterprise: Unknown (185470479)
Length: 6
Option 125 Suboption: 1
Length: 1
Data: 01
Option 125 Suboption: 1
Length: 1
Data: 02I haven't been able to get I also tried to specify that the length should be 2 and it led to ==2779==ERROR: AddressSanitizer: requested allocation size 0xffffffffffffffff (0x800 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0)
#0 0x557dc45a5610 in realloc (/dhcpcd/src/dhcpcd+0x1c3610) (BuildId: d0a7682fe8c9ae6e53a35f0320853624f09f84a4)
#1 0x557dc464b7c7 in dhcp_envoption /dhcpcd/src/dhcp-common.c:988:13
#2 0x557dc46756c2 in dhcp_env /dhcpcd/src/dhcp.c:1504:3
#3 0x557dc46532b5 in make_env /dhcpcd/src/script.c:480:7
#4 0x557dc4651368 in script_runreason /dhcpcd/src/script.c:759:16
#5 0x557dc467b726 in dhcp_bind /dhcpcd/src/dhcp.c:2447:3
#6 0x557dc46a43fd in dhcp_handledhcp /dhcpcd/src/dhcp.c:3503:2
#7 0x557dc468724e in dhcp_packet /dhcpcd/src/dhcp.c:3710:2
#8 0x557dc46a8b59 in dhcp_readbpf /dhcpcd/src/dhcp.c:3735:3
#9 0x557dc4611b2e in eloop_run_ppoll /dhcpcd/src/eloop.c:1106:5
#10 0x557dc4611b2e in eloop_start /dhcpcd/src/eloop.c:1228:11
#11 0x557dc46008d8 in main /dhcpcd/src/dhcpcd.c:2650:6
#12 0x7f8a3c352ca7 (/lib/x86_64-linux-gnu/libc.so.6+0x29ca7) (BuildId: def5460e3cee00bfee25b429c97bcc4853e5b3a8)With new_yolo_125=0b0e0e0f06010101010102All in all it doesn't seem this patch does what it's supposed to do (and introduces memory corruptions triggered by incoming packets) so to me it seems it can be safely closed and whatever #74 is about can be discussed there probably. |
4 participants
Apologies, a bit of a branch cleanup :)
Original pull request #378
Same patch in issue #74