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

Fix potential read buffer overflow in PACKET_strndup() #399

Closed
wants to merge 1 commit into from
Closed

Fix potential read buffer overflow in PACKET_strndup() #399

wants to merge 1 commit into from

Conversation

ghedo
Copy link
Contributor

@ghedo ghedo commented Sep 16, 2015

Due to its use of BUF_strndup() (which in turn uses BUF_strlcpy()) the
strlen() function will get called on the input data. However, since the
input data may not end in a NUL-byte, strlen() will read from invalid
memory.

Here's the address sanitizer report for the packettest test (which is how
I found the problem):

==2296==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdbf63923a at pc 0x7f470fe2f92b bp 0x7ffdbf639130 sp 0x7ffdbf6388e0
READ of size 5 at 0x7ffdbf63923a thread T0
    #0 0x7f470fe2f92a in strlen (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x6d92a)
    #1 0x40a969 in BUF_strlcpy /home/ghedo/devel/openssl/crypto/buffer/buf_str.c:121
    #2 0x40a798 in BUF_strndup /home/ghedo/devel/openssl/crypto/buffer/buf_str.c:93
    #3 0x402abd in PACKET_strndup ../ssl/packet_locl.h:398
    #4 0x404e6b in test_PACKET_strndup /home/ghedo/devel/openssl/test/packettest.c:269
    #5 0x406370 in main /home/ghedo/devel/openssl/test/packettest.c:444
    #6 0x7f470f836b44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)
    #7 0x4019e8  (/home/ghedo/devel/openssl/test/packettest+0x4019e8)

Address 0x7ffdbf63923a is located in stack of thread T0 at offset 106 in frame
    #0 0x404d3a in test_PACKET_strndup /home/ghedo/devel/openssl/test/packettest.c:259

  This frame has 4 object(s):
    [32, 40) 'data'
    [96, 106) 'buf' <== Memory access at offset 106 overflows this variable
    [160, 170) 'buf2'
    [224, 248) 'pkt'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow ??:0 strlen
Shadow bytes around the buggy address:
  0x100037ebf1f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100037ebf200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100037ebf210: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100037ebf220: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100037ebf230: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4
=>0x100037ebf240: f4 f4 f2 f2 f2 f2 00[02]f4 f4 f2 f2 f2 f2 00 02
  0x100037ebf250: f4 f4 f2 f2 f2 f2 00 00 00 f4 f3 f3 f3 f3 00 00
  0x100037ebf260: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4
  0x100037ebf270: f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2 00 00 00 00
  0x100037ebf280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100037ebf290: 00 00 00 00 00 00 00 00 00 00 00 07 f3 f3 f3 f3
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
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
==2296==ABORTING

@kaduk
Copy link
Contributor

kaduk commented Sep 16, 2015

Why is it better to fix the issue here than in BUF_strndup itself? The siz = BUF_strnlen(str, siz) idiom is already present there, and it could be implemented using strncpy insted of strlcpy and always set the (siz+1)th byte to NUL.

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Because I didn't think of it... indeed it may be simpler, I'll look into that.

@ekasper
Copy link
Contributor

ekasper commented Sep 16, 2015

Thanks for the report! Mea culpa. Turns out the standard strndup contract doesn't seem to say anything about reading out of the given bound, and BUF_strndup just does what strndup promises to do, and nothing more.

That said, fixing BUF_strndup, and making it part of its contract is the safe thing to do. ghedo@, would you like to have a go yourself or shall I?

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

@ekasper I just pushed a new commit fixing BUF_strndup(). I'm currently running the test to check that everythin is ok.

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Pushed the correct fix now (there was a mistake in the old one).

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Looks good. You can merge if you want.

@kaduk
Copy link
Contributor

kaduk commented Sep 16, 2015

That said, fixing BUF_strndup, and making it part of its contract is the safe thing to do.

Can we document its contract at the same time? I didn't see a comment or manual page describing the current contract.

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Ok, so I just added a brief description of BUF_strndup().

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Of course the description was wrong -.-", fixed it now.

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Though it might make more sense to put it in include/openssl/buffer.h...

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Though it might make more sense to put it in include/openssl/buffer.h...

Done that now (but I can revert the change if needed).

@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

So, I also added the overflow check based on INT_MAX (since OPENSSL_malloc takes an int), not sure if that's what you had in mind.

@ghedo
Fix potential read buffer overflow in BUF_strndup()
cd6dd0d 
Due to its use of BUF_strlcpy(), the strlen() function will get called
on the input data. However, since the input data may not end in a
NUL-byte, strlen() will read from invalid memory.

This also adds a check for siz overflow and some brief documentation
for BUF_strndup().
@ghedo
Copy link
Contributor Author

ghedo commented Sep 16, 2015

Tests went good as well, so unless there are other comments I think this can be merged.

@ghedo
Copy link
Contributor Author

ghedo commented Sep 18, 2015

@ekasper ping?

@ekasper
Copy link
Contributor

ekasper commented Sep 18, 2015

Looks good, I am waiting for a second team member to review this.

@ghedo
Copy link
Contributor Author

ghedo commented Sep 23, 2015

Merged in 110f7b3, so this can be closed.

@ghedo ghedo closed this Sep 23, 2015
@ghedo ghedo deleted the packet_overflow branch September 25, 2015 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ghedo @kaduk @ekasper