darrylb123 / usbrelay

Control usb relay - based on hidapi
GNU General Public License v2.0
310 stars 98 forks source link

Fix heap overflow #70

Closed wentasah closed 3 years ago

wentasah commented 3 years ago

Memory pointed to by relay_boards variable is allocated only for "known relays". When iterating the devices, we have to skip "unknown relays" not to go out of bounds.

This fixes the following error produced by address sanitizer:

==93537==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000002468 at pc 0x7f6f3b3a37d1 bp 0x7ffffabaf710 sp 0x7ffffabaf708
WRITE of size 4 at 0x603000002468 thread T0
    #0 0x7f6f3b3a37d0 in enumerate_relay_boards /home/wsh/src/usbrelay/libusbrelay.c:75
    #1 0x4029ac in main /home/wsh/src/usbrelay/usbrelay.c:175
    #2 0x7f6f3b1fb77f in __libc_start_main (/nix/store/9bh3986bpragfjmr32gay8p95k91q4gy-glibc-2.33-47/lib/libc.so.6+0x2777f)
    #3 0x402239 in _start (/home/wsh/src/usbrelay/usbrelay+0x402239)

0x603000002468 is located 16 bytes to the right of 24-byte region [0x603000002440,0x603000002458)
allocated by thread T0 here:
    #0 0x7f6f3b453ff7 in __interceptor_calloc (/nix/store/0kiykyrnrpfhmjwxwx89kxr20hmf5304-gcc-10.3.0-lib/lib/libasan.so.6+0xacff7)
    #1 0x7f6f3b3a379e in enumerate_relay_boards /home/wsh/src/usbrelay/libusbrelay.c:69
darrylb123 commented 3 years ago

Still there on mine. I need to learn to decode the sanitize error

$ sudo usbrelay -d ZAQ12_2=1

==3556==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000036 at pc 0x7f0b684cbc77 bp 0x7ffdde1482b0 sp 0x7ffdde147a60 READ of size 7 at 0x602000000036 thread T0

0 0x7f0b684cbc76 in printf_common(void, char const, __va_list_tag*) (/lib64/libasan.so.6+0x5bc76)

#1 0x7f0b684cc52e in __interceptor_vfprintf (/lib64/libasan.so.6+0x5c52e)
#2 0x7f0b684cc61e in fprintf (/lib64/libasan.so.6+0x5c61e)
#3 0x40187f in main /home/dbond/src/usbrelay/usbrelay.c:165
#4 0x7f0b682b9b74 in __libc_start_main (/lib64/libc.so.6+0x27b74)
#5 0x40123d in _start (/usr/bin/usbrelay+0x40123d)

0x602000000036 is located 0 bytes to the right of 6-byte region [0x602000000030,0x602000000036) allocated by thread T0 here:

0 0x7f0b6851e93f in __interceptor_malloc (/lib64/libasan.so.6+0xae93f)

#1 0x401713 in main /home/dbond/src/usbrelay/usbrelay.c:145

SUMMARY: AddressSanitizer: heap-buffer-overflow (/lib64/libasan.so.6+0x5bc76) in printf_common(void, char const, __va_list_tag*) Shadow bytes around the buggy address: 0x0c047fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c047fff8000: fa fa 00 00 fa fa[06]fa fa fa fa fa fa fa fa fa 0x0c047fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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 Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==3556==ABORTING

wentasah commented 3 years ago

Decoding is not that hard. Just find the first location from your code in the backtrace (above /home/dbond/src/usbrelay/usbrelay.c:165) - this is (usually) where the problem is. You, or more precisely fprintf, are reading from some memory where you shouldn't read. Perhaps you gave fprintf some wrong pointer. And this memory was allocated (either via malloc or as local or global variable) as shown it the next part of the report: /home/dbond/src/usbrelay/usbrelay.c:145.

The address map shown at the end of the report is usually not needed. It might be useful in some tricky cases, but I've never needed to decode it.

darrylb123 commented 3 years ago

Yes, just noticed the sanitize error is in usbrelay.c and only occurs when using the -d flag. There is also a memory leak when not using th e-d flag. Can't find that either. Thanks for your description.

darrylb123 commented 3 years ago

Your patch seems to have fixed the original problam.

darrylb123 commented 3 years ago

Found both errors! strncpy doesn't insert a null byte at the end of the copy and the loop to free the serials started at 1.