clif: After incrementing pos, make sure it is still in bounds

[nmeum]

Contribution description

While the for-loop condition does contain a bounds check, the pointer is independently increment in the for-loop body. This increment therefore requires a separate bounds check. Otherwise, the parsing loop may access data outside the given buffer boundaries.

Testing procedure

Sample application code:

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

static uint8_t buf[] = { 59, 196, 198, 196, 196, 196, 196, 196, 196, 61 };

int main(void)
{
        char *input = (char *)&buf[0];
	clif_attr_t attr;
	clif_get_attr(input, sizeof(buf), &attr);
	return 0;
}

Minimal Makefile:

APPLICATION = clif

BOARD = native

USEMODULE += clif

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

Invoke as:

$ make -C examples/clif all-asan
$ make -C examples/clif term
main(): This is RIOT! (Version: 2021.04-devel-461-g50cf93)
=================================================================
==6533==ERROR: AddressSanitizer: global-buffer-overflow on address 0x565cb0ea at pc 0x565bb00a bp 0x565d08d8 sp 0x565d08cc
READ of size 1 at 0x565cb0ea thread T0
    #0 0x565bb009 in clif_get_attr /root/RIOT/sys/clif/clif.c:281
    #1 0x565bab5b in main /root/RIOT/examples/clif/main.c:12
    #2 0x565bb501 in main_trampoline /root/RIOT/core/init.c:58
    #3 0xf76ca53a in makecontext (/lib/i386-linux-gnu/libc.so.6+0x4153a)

0x565cb0ea is located 0 bytes to the right of global variable 'buf' defined in '/root/RIOT/examples/clif/main.c:6:16' (0x565cb0e0) of size 10
SUMMARY: AddressSanitizer: global-buffer-overflow /root/RIOT/sys/clif/clif.c:281 in clif_get_attr
Shadow bytes around the buggy address:
  0x2acb95c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acb95d0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acb95e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acb95f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x2acb9600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x2acb9610: 00 00 00 00 00 00 00 00 00 00 00 00 00[02]f9 f9
  0x2acb9620: f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acb9630: 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x2acb9640: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acb9650: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x2acb9660: 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
==6533==ABORTING

With this patch applied, no error is reported.

Issues/PRs references

None.

[leandrolanzieri]

Thanks @nmeum for the fix! Just a couple of comments

[leandrolanzieri]

If it gets to this condition, it would be a malformed attribute right? (e.g. foo=)

Suggested change

	break;
	return CLIF_NOT_FOUND;

[nmeum]

Right, but what about foo="? That doesn't seem to return CLIF_NOT_FOUND either at the moment or am I missing something here?

[leandrolanzieri]

Yeah, we should check for that, and also a possible foo="ba I guess, and return CLIF_NOT_FOUND

[nmeum]

Ok, maybe it makes sense to implement this separately? The changes I propose here handle foo= in the same way as foo=" is handled presently.

[leandrolanzieri]

Ok. Could you add your proposed test to the unit test application?

[nmeum]

Done. Keep in mind though that the tests are not run with ASAN enabled. So even if this bug would be reintroduced they would not necessarily detect it.

[leandrolanzieri]

ACK.