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: check if uri is long enough to even contain a :// #15930

Merged
merged 1 commit into from Feb 5, 2021

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Feb 4, 2021

Contribution description

Before attempting to access the :// part of the URI it is necessary to check that the URI is long enough to actually contain this string and data following it. Otherwise, a read outside the bounds of the provided buffer is performed if the buffer is too small.

Testing procedure

Application code:

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

#define BUFFER_SIZE 5

static char buf[] = { 112, 80, 58, 47, 47, };

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

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

Makefile:

APPLICATION = uri
DEVELHELP = 1

BOARD = native

USEMODULE += uri_parser

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

Run:

$ make -C examples/uri all-asan
$ make -C examples/uri term
main(): This is RIOT! (Version: 2021.04-devel-440-g4cc04)
=================================================================
==24892==ERROR: AddressSanitizer: global-buffer-overflow on address 0x5664f105 at pc 0x56645e58 bp 0x56654718 sp 0x5665470c
READ of size 1 at 0x5664f105 thread T0
    #0 0x56645e57 in _consume_authority /root/RIOT/sys/uri_parser/uri_parser.c:132
    #1 0x5664661a in _parse_absolute /root/RIOT/sys/uri_parser/uri_parser.c:224
    #2 0x56646ae9 in uri_parser_process /root/RIOT/sys/uri_parser/uri_parser.c:282
    #3 0x5663cbcb in main /root/RIOT/examples/uri/main.c:15
    #4 0x5663d0ff in main_trampoline /root/RIOT/core/init.c:58
    #5 0xf773853a in makecontext (/lib/i386-linux-gnu/libc.so.6+0x4153a)

0x5664f105 is located 0 bytes to the right of global variable 'buf' defined in '/root/RIOT/examples/uri/main.c:8:13' (0x5664f100) of size 5
SUMMARY: AddressSanitizer: global-buffer-overflow /root/RIOT/sys/uri_parser/uri_parser.c:132 in _consume_authority
Shadow bytes around the buggy address:
  0x2acc9dd0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acc9de0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acc9df0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acc9e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acc9e10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x2acc9e20:[05]f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x2acc9e30: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x2acc9e40: f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acc9e50: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acc9e60: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acc9e70: 00 00 00 00 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
==24892==ABORTING
make: *** [/root/RIOT/examples/uri/../../Makefile.include:725: term] Error 1
make: Leaving directory '/root/RIOT/examples/uri'

Issues/PRs references

This is similar to #15927 but not addressed in #15929.

sys/uri_parser/uri_parser.c Outdated Show resolved Hide resolved
sys/uri_parser/uri_parser.c Outdated Show resolved Hide resolved
@nmeum nmeum force-pushed the pr/uri_parser_scheme branch 2 times, most recently from 95aeb76 to e2cad36 Compare February 4, 2021 16:03
Before attempting to access these characters. This fixes an
out-of-bounds read on the provided URI buffer.
@nmeum nmeum requested a review from miri64 as a code owner February 5, 2021 10:16
@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 5, 2021
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as is. I think we need another check after consuming the scheme whether there is still space in buffer to consume (similar to here: https://github.com/RIOT-OS/RIOT/pull/15929/files#diff-05fd96efad40b0211dd1f6c15d061bc4c810f3b9f0c18eb5d33b054413aa6d75R235). But we can do that in the other PR. I'll just rebase my PR onto your's once we merged it.

@cgundogan cgundogan added Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 5, 2021
@cgundogan cgundogan merged commit 07f1254 into RIOT-OS:master Feb 5, 2021
3 checks passed
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants