Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Potential heap buffer overflow in modbus.c #365

Closed
tinajohnson opened this issue Jun 30, 2022 · 2 comments · Fixed by #366
Closed

Potential heap buffer overflow in modbus.c #365

tinajohnson opened this issue Jun 30, 2022 · 2 comments · Fixed by #366
Labels
bug

Comments

@tinajohnson
Copy link
Contributor

Hi,

There's a heap buffer overflow vulnerability in the create_tag_object function of src/protocols/mb/modbus.c. The variable elem_count is user input and thus can be a negative value. When elem_count is a negative value, data_size variable will also be a negative value and the allocation of tag object at line 365 will result in an object of size lesser than the sizeof(struct modbus_tag_t). Since tag object is expected to have a minimum size of sizeof(struct modbus_tag_t), there will be heap buffer overflows when there are writes to certain struct elements that were not allocated. For example, the stack trace below shows a crash produced by AddressSanitizer at line 375 where there is a write to (*tag)->reg_base and elem_count is -627.

Stack trace:

=================================================================
==1366675==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c0000000e4 at pc 0x7f7dac436ad0 bp 0x7ffca33d1870 sp 0x7ffca33d1868
WRITE of size 2 at 0x60c0000000e4 thread T0
    #0 0x7f7dac436acf in create_tag_object /home/iunknown/libplctag/src/protocols/mb/modbus.c:375:22
    #1 0x7f7dac436acf in mb_tag_create /home/iunknown/libplctag/src/protocols/mb/modbus.c:279:10
    #2 0x7f7dac34768c in plc_tag_create /home/iunknown/libplctag/src/lib/lib.c:830:11
    #3 0x4cad69 in process_line /home/iunknown/libplctag/src/examples/data_dumper.c:206:29
    #4 0x4cc0d3 in read_config /home/iunknown/libplctag/src/examples/data_dumper.c:270:18
    #5 0x4cec44 in main /home/iunknown/libplctag/src/examples/data_dumper.c:515:14
    #6 0x7f7dabefa0b2 in __libc_start_main /build/glibc-YbNSs7/glibc-2.31/csu/../csu/libc-start.c:308:16
    #7 0x41d4ad in _start (/home/iunknown/libplctag/build/bin_dist/data_dumper 0x41d4ad)

0x60c0000000e4 is located 41 bytes to the right of 123-byte region [0x60c000000040,0x60c0000000bb)
allocated by thread T0 here:
    #0 0x498882 in calloc (/home/iunknown/libplctag/build/bin_dist/data_dumper 0x498882)
    #1 0x7f7dac45ce67 in rc_alloc_impl /home/iunknown/libplctag/src/util/rc.c:115:10
    #2 0x7f7dac434602 in create_tag_object /home/iunknown/libplctag/src/protocols/mb/modbus.c:365:26
    #3 0x7f7dac434602 in mb_tag_create /home/iunknown/libplctag/src/protocols/mb/modbus.c:279:10
    #4 0x7f7dac34768c in plc_tag_create /home/iunknown/libplctag/src/lib/lib.c:830:11
    #5 0x4cad69 in process_line /home/iunknown/libplctag/src/examples/data_dumper.c:206:29
    #6 0x4cc0d3 in read_config /home/iunknown/libplctag/src/examples/data_dumper.c:270:18
    #7 0x4cec44 in main /home/iunknown/libplctag/src/examples/data_dumper.c:515:14
    #8 0x7f7dabefa0b2 in __libc_start_main /build/glibc-YbNSs7/glibc-2.31/csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/iunknown/libplctag/src/protocols/mb/modbus.c:375:22 in create_tag_object
Shadow bytes around the buggy address:
  0x0c187fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c187fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c187fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c187fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c187fff8000: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x0c187fff8010: 00 00 00 00 00 00 00 03 fa fa fa fa[fa]fa fa fa
  0x0c187fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c187fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c187fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c187fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c187fff8060: 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
==1366675==ABORTING
tinajohnson added a commit to tinajohnson/libplctag that referenced this issue Jun 30, 2022
@tinajohnson
Add condition to fail tag creation when elem_count is a negative value.
dd0cb8c 
Fixes libplctag#365
@tinajohnson tinajohnson mentioned this issue Jun 30, 2022
Merged
kyle-github pushed a commit that referenced this issue Jul 1, 2022
@tinajohnson
Add condition to fail tag creation when elem_count is a negative valu…
d1d3cb4 
…e. (#366)

Fixes #365
@kyle-github kyle-github added the bug label Jul 1, 2022
@kyle-github
Copy link
Member

Thanks for reporting this and the PR! Did you use some tooling to find these issues?

I need to go through my code again as I am certain I had checks on some of this stuff before.

I have made a release with your fixes and another fix that was identified when I tried to merge your PRs.

@tinajohnson
Copy link
Contributor Author

tinajohnson commented Jul 1, 2022 •
edited
Loading

Hi Kyle,

You are welcome! No tools were used for finding these vulnerabilities, it was mostly code review and ASan. If you are interested in fuzzing, checkout AFL, honggfuzz, Jackalope.

Thank you for the merges!

kyle-github added a commit that referenced this issue Sep 24, 2022
@kyle-github @tinajohnson
Fix 371, fix 378, fix 381 - string termination, extra delay, and inco…
8835d56 
…rrect comparison (#383)

* Add condition to fail tag creation when elem_count is a negative value. (#366)

Fixes #365

* Change CI build to use Visual Studio 17 2022.  Apparently version 16 is gone on Github?

* Add checks to prevent out-of-bounds write on tag->encoded_name (#368)

Fixes #367

* Update stable vs. unstable versions.

* Bump version for next release.

* Fix 378.   Refactor condition var signal into common exit path of state SESSION_OPEN_SOCKET_START.

* Fix 381. Incorrect comparison in loop guarding cond_wait from spurious wakeups.

* Add null terminator to tag->name string (#372)

Fixes #371

Co-authored-by: Tina Johnson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add condition to fail tag creation when elem_count is a negative value. tinajohnson/libplctag
2 participants
@kyle-github @tinajohnson