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

sock_dns: Fix incorrect buffer bounds check #15345

Merged
merged 1 commit into from Oct 31, 2020

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Oct 30, 2020

Contribution description

The following bounds check performed in sock_dns is IMHO incorrect:

if ((bufpos + RR_TYPE_LENGTH + RR_CLASS_LENGTH + RR_TTL_LENGTH) >= buflim) {
return -EBADMSG;
}

It does not take into account that after the bufpos is advanced by RR_TYPE_LENGTH, RR_CLASS_LENGTH, and RR_TTL_LENGTH two bytes are read unconditionally using the _get_short function in the following line:

unsigned addrlen = ntohs(_get_short(bufpos));

This is currently not taken into account by the bounds check, thus resulting in a potential out-of-bounds buffer access by a maximum of two bytes.

Testing procedure

The easiest way to confirm this issue is using the following application:

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

#include <net/af.h>

extern int _parse_dns_reply(uint8_t *buf, size_t len, void *addr_out, int family);

static uint8_t buf[] = {
	0, 0, 0, 0, 0, 0, 255, 0, 0, 0, 0, 0, 192, 0, 0, 0, 0, 0, 0, 0, 0, 0,
	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
	0, 0, 0, 0, 0,
};

int main(void)
{
    void *addr;
    uint8_t addr_buf[16];

    addr = &addr_buf[0];
    _parse_dns_reply(buf, sizeof(buf), addr, AF_UNSPEC);

    exit(EXIT_SUCCESS);
    return 0;
}

Attention: This application passes data directly to _parse_dns_reply, for this reason the static keyword must be removed from the _parse_dns_reply function in sys/net/application_layer/dns/dns.c.

Afterwards, compile the application with:

$ make BOARD=native -C examples/sock_dns/ all-asan

And execute it with:

$ make BOARD=native -C examples/sock_dns/ term

This will result in the following error message:

==15536==ERROR: AddressSanitizer: global-buffer-overflow on address 0x56655151 at pc 0x56640049 bp 0x5665b798 sp 0x5665b78c
READ of size 2 at 0x56655151 thread T0
    #0 0x56640048 in _get_short /root/RIOT/sys/net/application_layer/dns/dns.c:77
    #1 0x566403b9 in _parse_dns_reply /root/RIOT/sys/net/application_layer/dns/dns.c:141
    #2 0x56616bf0 in main /root/RIOT/examples/sock_dns/main.c:22
    #3 0x56616f5d in main_trampoline /root/RIOT/core/init.c:57
    #4 0xf767453a in makecontext (/lib/i386-linux-gnu/libc.so.6+0x4153a)

0x56655152 is located 0 bytes to the right of global variable 'buf' defined in '/root/RIOT/examples/sock_dns/main.c:10:16' (0x56655120) of size 50
SUMMARY: AddressSanitizer: global-buffer-overflow /root/RIOT/sys/net/application_layer/dns/dns.c:77 in _get_short
Shadow bytes around the buggy address:
  0x2acca9d0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acca9e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acca9f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2accaa00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2accaa10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x2accaa20: 00 00 00 00 00 00 00 00 00 00[02]f9 f9 f9 f9 f9
  0x2accaa30: 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2accaa40: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x2accaa50: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2accaa60: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x2accaa70: 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
==15536==ABORTING

With the proposed patch applied no error is detected by ASAN. I think the _get_short function should also be renamed to _get_u16 and the issue could be avoided all together by passing buffer bounds to _get_u16 and checking these bounds before performing the access in this function.

Issues/PRs references

The incorrect check was introduced in #10740 for fixing #10739.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Change looks sensible, will test. In the mean time please apply the following style nit.

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved
@miri64 miri64 added Area: network Area: Networking 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) labels Oct 30, 2020
@miri64
Copy link
Member

miri64 commented Oct 30, 2020

ACK. I can reproduce the steps the testing procedures in master and confirm that the ASAN error disappears when this PR is applied. For posterity, here is the minimal Makefile I used for the provided C-code (which I put in a sub-directory of tests/):

include ../Makefile.tests_common

USEMODULE += sock_dns
USEMODULE += sock_udp
USEMODULE += gnrc_nettype_ipv6

include $(RIOTBASE)/Makefile.include

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 30, 2020
@miri64
Copy link
Member

miri64 commented Oct 30, 2020

Please squash

@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 30, 2020
@nmeum
Copy link
Member Author

nmeum commented Oct 30, 2020

Thank you for testing! :)

Please squash

Done.

Apart from advancing the buffer by RR_TYPE_LENGTH, RR_CLASS_LENGTH,
and RR_TTL_LENGTH the code also attempts to read a two byte unsigned
integer using _get_short(bufpos):

	unsigned addrlen = ntohs(_get_short(bufpos));

The bounds check must therefore ensure that the given buffer is large
enough to contain two more bytes after advancing the buffer.
@nmeum
Copy link
Member Author

nmeum commented Oct 31, 2020

Seems to have passed on CI, is there anything else I can do in order to get this merged?

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

My ACK was missing ;-). I tested it and the fix fixes, what it is supposed to do. ACK

@miri64 miri64 merged commit 44741ff into RIOT-OS:master Oct 31, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch 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

2 participants