- Notifications
You must be signed in to change notification settings - Fork137
Concatenate multiple sub-options with the same code sent by the server#404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
evverx left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.128for 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 0x502000002990in 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])000002010600E16653BD00000000C0A80101 .....fS.........0010C0A80180C0A80101C0A80101FE6711DB .............g..00205AF6000000000000000000000E001C00Z...............00301C0000000000000000000000001C0400 ................00401C001C00772600000000000000002010 ....w&........ .00506C6F63616C686F737401020020000000localhost... ...006000000000000000000000000000003501 ..............5.007002360405C0FFFF7A3304000400290000.6.....z3....)..008000000000000000005E00000000000000 ........^.......009000000000000000000000FF0500000013 ................00a00041001C00310123E7001C001C001C04 .A...1.#........00b0001C00012300002104EFFBFF7E330400 ....#..!....~3..00c000656872000032010229001103DDA33D .ehr..2..).....=00d000008600FFFF7A0300E000033FA35554 ......z.....?.UT00e0436C0512FFFFFFFF2D00FC0063825363Cl......-...c.Sc00f0350105340123FF5..4.#.>>>p[DHCP]<DHCPoptions=[message-type=ackdhcp-option-overload=35end]|>>>>p[BOOTP].fileb'\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].snameb'\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.
perkelix commentedNov 22, 2025
Could this be rebased with master? |
spoljak-ent commentedNov 24, 2025
I think this should be a bit more optimized as@evverx mentioned |
rsmarples commentedNov 24, 2025
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 |
evverx commentedNov 24, 2025
@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. |
spoljak-ent commentedNov 24, 2025
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. |
evverx commentedNov 24, 2025
I don't think so either. I replied to the comment wherehttps://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. |
evverx commentedNov 25, 2025
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: 02 I 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 adjustmentsfor 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=0b0e0e0f06010101010102 All 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. |
Apologies, a bit of a branch cleanup :)
Original pull request#378
Same patch in issue#74