View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0004349unrealircdpublic2015-05-11 17:292015-05-19 02:58
Reporterdg Assigned Totmcarthur  
PrioritynormalSeverityminorReproducibilityhave not tried
Status resolvedResolutionopen 
Product Version3.2.10.4 
Fixed in Version3.2.10.5 
Summary0004349: ircd crashes with OOB read in cidr.c
DescriptionWhen running UnrealIRCd 3.2.10.4 compiled with AddressSanitizer, the ircd crashes after linking to a server with ziplinks.

AddressSanitizer is a tool to detect memory errors in C and C++ programs. I am encountering this bug every time my autoconnect fires (within seconds of ircd starting). Could this be a security issue, and if so, is it limited to servers or could a client potentially exploit it?

AddressSanitizer information containing relevant lines below:

==3791==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000134cf at pc 0x55555566ec3a bp 0x7fffffffd680 sp 0x7fffffffd67
8
READ of size 1 at 0x6020000134cf thread T0
    #0 0x55555566ec39 in parse_v4_netmask /home/irc/Unreal3.2.10.4/src/cidr.c:204
    #1 0x55555566f25d in parse_netmask /home/irc/Unreal3.2.10.4/src/cidr.c:286
    #2 0x7ffff30a3a39 in _tkl_add_line /home/irc/Unreal3.2.10.4/src/modules/m_tkl.c:1001
    #3 0x7ffff30adefe in _m_tkl /home/irc/Unreal3.2.10.4/src/modules/m_tkl.c:2064
    #4 0x5555555e1ced in parse /home/irc/Unreal3.2.10.4/src/parse.c:451
    #5 0x5555555ddd21 in dopacket /home/irc/Unreal3.2.10.4/src/packet.c:138
    #6 0x5555555a439c in read_packet /home/irc/Unreal3.2.10.4/src/s_bsd.c:1562
    #7 0x5555555a7381 in read_message /home/irc/Unreal3.2.10.4/src/s_bsd.c:2152
    0000008 0x5555555cc951 in main /home/irc/Unreal3.2.10.4/src/ircd.c:1867
    #9 0x7ffff6107b44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)
    #10 0x555555597b19 (/home/irc/Unreal3.2.10.4/src/ircd+0x43b19)

0x6020000134cf is located 1 bytes to the left of 2-byte region [0x6020000134d0,0x6020000134d2)
allocated by thread T0 here:
    #0 0x7ffff6f378a7 in __interceptor_strdup (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x328a7)
    #1 0x7ffff30a348c in _tkl_add_line /home/irc/Unreal3.2.10.4/src/modules/m_tkl.c:974
    #2 0x7ffff30adefe in _m_tkl /home/irc/Unreal3.2.10.4/src/modules/m_tkl.c:2064
    #3 0x5555555e1ced in parse /home/irc/Unreal3.2.10.4/src/parse.c:451
    #4 0x5555555ddd21 in dopacket /home/irc/Unreal3.2.10.4/src/packet.c:138
    #5 0x5555555a439c in read_packet /home/irc/Unreal3.2.10.4/src/s_bsd.c:1562
    #6 0x5555555a7381 in read_message /home/irc/Unreal3.2.10.4/src/s_bsd.c:2152
    #7 0x5555555cc951 in main /home/irc/Unreal3.2.10.4/src/ircd.c:1867
    0000008 0x7ffff6107b44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/irc/Unreal3.2.10.4/src/cidr.c:204 parse_v4_netmask
Shadow bytes around the buggy address:
  0x0c047fffa640: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffa650: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffa660: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffa670: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffa680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fffa690: fa fa fa fa fa fa 00 02 fa[fa]02 fa fa fa 00 fa
  0x0c047fffa6a0: fa fa 00 fa fa fa 00 00 fa fa 00 04 fa fa 00 00
  0x0c047fffa6b0: fa fa 00 03 fa fa 00 00 fa fa 00 03 fa fa 00 00
  0x0c047fffa6c0: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 07
  0x0c047fffa6d0: fa fa 00 00 fa fa 00 05 fa fa 00 00 fa fa 03 fa
  0x0c047fffa6e0: fa fa 00 00 fa fa 00 00 fa fa 00 fa fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable: 00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone: fa
  Heap right redzone: fb
  Freed heap region: fd
  Stack left redzone: f1
  Stack mid redzone: f2
  Stack right redzone: f3
  Stack partial redzone: f4
  Stack after return: f5
  Stack use after scope: f8
  Global redzone: f9
  Global init order: f6
  Poisoned by user: f7
  Contiguous container OOB:fc
  ASan internal: fe
==3791==ABORTING
Steps To Reproduceexport ASAN_OPTIONS="abort_on_error=1"
export CFLAGS="-fsanitize=address -ggdb"
export CXXFLAGS="-fsanitize=address -ggdb"
./Config
make
src/ircd -t
TagsNo tags attached.
3rd party modules

Activities

dg

2015-05-11 17:32

reporter   ~0018294

You may wish to add "-fno-omit-frame-pointer" to CFLAGS for a better output.

tmcarthur

2015-05-14 07:04

reporter   ~0018297

Having a look now - thanks!

syzop

2015-05-16 15:23

administrator   ~0018299

Last edited: 2015-05-16 15:54

 Just glancing at the source just now. Error is at:

        if (*(p + 1) || n == 0 || *(p - 1) != '.') /* Error: * is not at the end
                                * or not its own section */

and the description is:

0x6020000134cf is located 1 bytes to the left of 2-byte region [0x6020000134d0,0x6020000134d2)

Possibly the *(p - 1) is the problem, it's reading 1 byte before 'p' so if 'p' points to the very 1st byte then it's reading the byte BEFORE the actual string. It had a 'n == 0' check, probably to prevent this, but a number of lines up it does 'digits[n++] = text;' so n would be 1 and not 0 at the start of the string.

Perhaps change:
        if (*(p + 1) || n == 0 || *(p - 1) != '.') /* Error: * is not at the end
                                * or not its own section */
To:
        if (*(p + 1) || n == 1 || *(p - 1) != '.') /* Error: * is not at the end
                                * or not its own section */

Well.. probably should take a bit more in-depth look :D

I would theorise that a string to trigger this would be just "*"

(EDIT: removed statement regarding "not a netmask")

dg

2015-05-17 15:34

reporter   ~0018300

Do you want me to try changing n == 0 to n == 1 and see if it fixes it? It's possible some more errors might come up in separate places.

ASAN holds no prisoners and dies when it finds the first error, so there may more laying dormant to fix after this one is resolved.

I'm happy to change, report back, and so on. Thanks for your speedy replies, guys! Let me know if I can be of any more assistance.

tmcarthur

2015-05-17 18:24

reporter   ~0018301

Fixed in 5d42ac844fe79dcad001972a6f34014955775332 - just verifying security impact, but it doesn't look like a big problem.

tmcarthur

2015-05-17 18:26

reporter   ~0018302

Moving to has patch until security verification finished

dg

2015-05-17 23:27

reporter   ~0018303

Confirmed that 5d42ac844fe79dcad001972a6f34014955775332 fixes the crash. Let us know if it is a security concern.

I'll continue to test Unreal against various tools and keep you guys abreast of any reports. I may at some point try the alpha, but I currently run stable on my network.

Thanks again for being so friendly and quick to fix.

syzop

2015-05-18 11:52

administrator   ~0018304

After having a chance to read the function a bit more I decided to change the fix a bit, see https://github.com/unrealircd/unrealircd/commit/af0e823116fbaa5b9d58826027bcf33cf597dbdd
I basically made an explicit (p == text) check rather than using 'n'.
And note that I did the same fix in the parse...v6 function as well.

This fixes the OOB read (what this issue is all about) but to be honest I'm not entirely sure if it fixed the entire function to do what the comments in the function actually said. In any case, I don't think I broke it more and it seems to have worked for so many years so it's probably OK :D. It mostly falls through to NM_HOST anyway.

Just be sure to make it a QA item for (whatever) next release.

Travis: you may want to port it to 3.4.x also.

As for the security impact: it's an out of bounds read of 1 byte before the actual string. It is unlikely that it would ever try to access invalid memory regions (and crash) as if it's on the stack.. all is okay.. if it's on the heap then even if it's at the very beginning of a malloc()'d region then you still have some malloc internals before that (probably at least 8 bytes). It's also just a 1 byte read, so no write or anything that can be used to gain privileges.

Thanks for the report dg! Bugs like this should always be fixed anyway.

syzop

2015-05-18 11:54

administrator   ~0018305

I'm marking this bug as resolved and changing view status from private to public now that there's no risk. (and it's called from config- and *LINE routines anyway)

Oh and I did re-indent the function a bit in 3.2.x as it was damn confusing.. a whole block of code was indented the same as the for () statement itself even though it belonged WITHIN that statement. Both with tab size 4 and tab 8. That's just confusing :D

Travis: well, see previous comment ;)

tmcarthur

2015-05-19 02:58

reporter   ~0018318

Thanks! Will have a look.

Issue History

Date Modified Username Field Change
2015-05-11 17:29 dg New Issue
2015-05-11 17:32 dg Note Added: 0018294
2015-05-14 07:03 tmcarthur Assigned To => tmcarthur
2015-05-14 07:03 tmcarthur Status new => assigned
2015-05-14 07:04 tmcarthur Note Added: 0018297
2015-05-16 15:23 syzop Note Added: 0018299
2015-05-16 15:54 syzop Note Edited: 0018299
2015-05-16 15:55 syzop Summary ircd crashes with a heap-buffer-overflow in cidr.c => ircd crashes with OOB read in cidr.c
2015-05-17 15:34 dg Note Added: 0018300
2015-05-17 18:24 tmcarthur Note Added: 0018301
2015-05-17 18:26 tmcarthur Note Added: 0018302
2015-05-17 18:26 tmcarthur Status assigned => has patch
2015-05-17 23:27 dg Note Added: 0018303
2015-05-18 11:52 syzop Note Added: 0018304
2015-05-18 11:54 syzop Note Added: 0018305
2015-05-18 11:54 syzop Status has patch => resolved
2015-05-18 11:54 syzop Fixed in Version => 3.2.10.5
2015-05-18 11:54 syzop View Status private => public
2015-05-19 02:58 tmcarthur Note Added: 0018318