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.

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

uri_parser: out-of-bounds read #15927

Closed
nmeum opened this issue Feb 4, 2021 · 2 comments · Fixed by #15929
Closed

uri_parser: out-of-bounds read #15927

nmeum opened this issue Feb 4, 2021 · 2 comments · Fixed by #15929
Assignees
Labels
Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@nmeum
Copy link
Member

nmeum commented Feb 4, 2021

Description

I did some testing of the uri_parser module as provided in sys/uri_parser. I believe I discovered an edge-case where the parser performs an out-of-bounds read of the provided buffer. The code causing this is:

result->userinfo = uri;
result->userinfo_len = userinfo_end - uri;
/* shift host part beyond userinfo and '@' */
result->host += result->userinfo_len + 1;
result->host_len -= result->userinfo_len + 1;

which advances result->host without a bounds check and even if userinfo_len is zero result->host is still advanced by one byte.

Steps to reproduce the issue

Application code:

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <uri_parser.h>

#define BUFFER_SIZE 5

static char buf[] = { 65, 58, 47, 47, 64 };

int main(void)
{
    size_t len = sizeof(buf);
    uri_parser_result_t result;

    uri_parser_process(&result, buf, len);
    return 0;
}

Minimal Makefile:

APPLICATION = uri
DEVELHELP = 1

BOARD = native

USEMODULE += uri_parser

RIOTBASE ?= $(CURDIR)/../..
include $(RIOTBASE)/Makefile.include

Afterwards, compile as:

$ make -C examples/uri all-asan

And run the application using:

$ make -C examples/uri term

Expected results

The application shouldn't crash.

Actual results

main(): This is RIOT! (Version: 2021.04-devel-440-g4cc04)
=================================================================
==24259==ERROR: AddressSanitizer: global-buffer-overflow on address 0x565e6105 at pc 0x565dce2b bp 0x565eb6f8 sp 0x565eb6ec
READ of size 1 at 0x565e6105 thread T0
    #0 0x565dce2a in _consume_authority /root/RIOT/sys/uri_parser/uri_parser.c:130
    #1 0x565dd5ed in _parse_absolute /root/RIOT/sys/uri_parser/uri_parser.c:222
    #2 0x565ddabc in uri_parser_process /root/RIOT/sys/uri_parser/uri_parser.c:280
    #3 0x565d3bcb in main /root/RIOT/examples/uri/main.c:15
    #4 0x565d40ed in main_trampoline /root/RIOT/core/init.c:58
    #5 0xf766353a in makecontext (/lib/i386-linux-gnu/libc.so.6+0x4153a)

0x565e6105 is located 0 bytes to the right of global variable 'buf' defined in '/root/RIOT/examples/uri/main.c:8:13' (0x565e6100) of size 5
SUMMARY: AddressSanitizer: global-buffer-overflow /root/RIOT/sys/uri_parser/uri_parser.c:130 in _consume_authority
Shadow bytes around the buggy address:
  0x2acbcbd0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acbcbe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acbcbf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acbcc00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acbcc10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x2acbcc20:[05]f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x2acbcc30: f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x2acbcc40: 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acbcc50: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acbcc60: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x2acbcc70: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
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
==24259==ABORTING
make: *** [/root/RIOT/examples/uri/../../Makefile.include:725: term] Error 1
make: Leaving directory '/root/RIOT/examples/uri'

Versions

I don't think this is needed, if you need more information let me know.

@miri64 miri64 added Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 4, 2021
nmeum pushed a commit to nmeum/RIOT that referenced this issue Feb 4, 2021
nmeum pushed a commit to nmeum/RIOT that referenced this issue Feb 4, 2021
This is intended as a hotfix for RIOT-OS#15927. Not sure if I would be
preferable to return an error in this case instead.

Fixes RIOT-OS#15927.
@cgundogan
Copy link
Member

thanks for this detailed report (: I was able to reproduce on native and while going through the code I also adjusted the unit tests for the case you presented "A://@". That's correct, right?

@nmeum
Copy link
Member Author

nmeum commented Feb 4, 2021

That's correct, right?

Yes, that's correct. I also found a separate issue while testing my fix for this issue. I documented this separately in #15930. Hope that's the correct way to do this. Thanks for your quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
3 participants