From 5625f5bc36954d644cb80adf8de47854c65d91c3 Mon Sep 17 00:00:00 2001 From: Hauke Mehrtens Date: Wed, 28 Oct 2020 00:16:38 +0100 Subject: [PATCH] uci: Backport security fixes This packports two security fixes from master. Signed-off-by: Hauke Mehrtens (cherry picked from commit f9005d4f80dee3dcc257d4613cbc46668faad094) --- package/system/uci/Makefile | 2 +- ...arse_package-fix-heap-use-after-free.patch | 51 ++++++++ ...-file-Check-buffer-size-after-strtok.patch | 112 ++++++++++++++++++ 3 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 package/system/uci/patches/001-file-uci_parse_package-fix-heap-use-after-free.patch create mode 100644 package/system/uci/patches/002-file-Check-buffer-size-after-strtok.patch diff --git a/package/system/uci/Makefile b/package/system/uci/Makefile index ebaf0cf161..4ee0d4ed73 100644 --- a/package/system/uci/Makefile +++ b/package/system/uci/Makefile @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk PKG_NAME:=uci -PKG_RELEASE:=1 +PKG_RELEASE:=2 PKG_SOURCE_URL=$(PROJECT_GIT)/project/uci.git PKG_SOURCE_PROTO:=git diff --git a/package/system/uci/patches/001-file-uci_parse_package-fix-heap-use-after-free.patch b/package/system/uci/patches/001-file-uci_parse_package-fix-heap-use-after-free.patch new file mode 100644 index 0000000000..47ea4e0ee6 --- /dev/null +++ b/package/system/uci/patches/001-file-uci_parse_package-fix-heap-use-after-free.patch @@ -0,0 +1,51 @@ +From a3e650911f5e6f67dcff09974df3775dfd615da6 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Petr=20=C5=A0tetiar?= +Date: Sat, 3 Oct 2020 01:29:21 +0200 +Subject: [PATCH] file: uci_parse_package: fix heap use after free +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Fixes following issue which is caused by usage of pointer which pointed +to a reallocated address: + + ERROR: AddressSanitizer: heap-use-after-free on address 0x619000000087 at pc 0x000000509aa7 bp 0x7ffd6b9c3c40 sp 0x7ffd6b9c3400 + READ of size 2 at 0x619000000087 thread T0 + #0 0x509aa6 in strdup (test-fuzz+0x509aa6) + #1 0x7fc36d2a1636 in uci_strdup util.c:60:8 + #2 0x7fc36d29e1ac in uci_alloc_generic list.c:55:13 + #3 0x7fc36d29e241 in uci_alloc_package list.c:253:6 + #4 0x7fc36d2a0ba3 in uci_switch_config file.c:375:18 + #5 0x7fc36d2a09b8 in uci_parse_package file.c:397:2 + #6 0x7fc36d2a09b8 in uci_parse_line file.c:513:6 + #7 0x7fc36d2a09b8 in uci_import file.c:681:4 + + 0x619000000087 is located 7 bytes inside of 1024-byte region [0x619000000080,0x619000000480) + freed by thread T0 here: + #0 0x51daa9 in realloc (test-fuzz+0x51daa9) + #1 0x7fc36d2a1612 in uci_realloc util.c:49:8 + + previously allocated by thread T0 here: + #0 0x51daa9 in realloc (test-fuzz+0x51daa9) + #1 0x7fc36d2a1612 in uci_realloc util.c:49:8 + +Reported-by: Jeremy Galindo +Signed-off-by: Petr Å tetiar +--- + file.c | 2 +- + ...sig-06,src-000079,time-22005942,op-ext_AO,pos-8 | Bin 0 -> 56 bytes + 2 files changed, 1 insertion(+), 1 deletion(-) + create mode 100644 tests/fuzz/corpus/id-000000,sig-06,src-000079,time-22005942,op-ext_AO,pos-8 + +--- a/file.c ++++ b/file.c +@@ -387,8 +387,8 @@ static void uci_parse_package(struct uci + pctx->pos += strlen(pctx_cur_str(pctx)) + 1; + + ofs_name = next_arg(ctx, true, true, true); +- name = pctx_str(pctx, ofs_name); + assert_eol(ctx); ++ name = pctx_str(pctx, ofs_name); + if (single) + return; + diff --git a/package/system/uci/patches/002-file-Check-buffer-size-after-strtok.patch b/package/system/uci/patches/002-file-Check-buffer-size-after-strtok.patch new file mode 100644 index 0000000000..463bb3adf0 --- /dev/null +++ b/package/system/uci/patches/002-file-Check-buffer-size-after-strtok.patch @@ -0,0 +1,112 @@ +From eae126f66663e5c73e5d290b8e3134449489340f Mon Sep 17 00:00:00 2001 +From: Hauke Mehrtens +Date: Sun, 4 Oct 2020 17:14:49 +0200 +Subject: [PATCH] file: Check buffer size after strtok() +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This fixes a heap overflow in the parsing of the uci line. + +The line which is parsed and put into pctx->buf is null terminated and +stored on the heap. In the uci_parse_line() function we use strtok() to +split this string in multiple parts after divided by a space or tab. +strtok() replaces these characters with a NULL byte. If the next byte is +NULL we assume that this NULL byte was added by strtok() and try to +parse the string after this NULL byte. If this NULL byte was not added +by strtok(), but by fgets() to mark the end of the string we would read +over this end of the string in uninitialized memory and later over the +allocated buffer. + +Fix this problem by storing how long the line we read was and check if +we would read over the end of the string here. + +This also adds the input which detected this crash to the corpus of the +fuzzer. + +Signed-off-by: Hauke Mehrtens +[fixed merge conflict in tests] +Signed-off-by: Petr Å tetiar +--- + file.c | 19 ++++++++++++++++--- + tests/cram/test-san_uci_import.t | 1 + + tests/cram/test_uci_import.t | 1 + + .../2e18ecc3a759dedc9357b1298e9269eccc5c5a6b | 1 + + uci_internal.h | 1 + + 5 files changed, 20 insertions(+), 3 deletions(-) + create mode 100644 tests/fuzz/corpus/2e18ecc3a759dedc9357b1298e9269eccc5c5a6b + +--- a/file.c ++++ b/file.c +@@ -28,6 +28,7 @@ + #include + #include + #include ++#include + + #include "uci.h" + #include "uci_internal.h" +@@ -63,6 +64,7 @@ __private void uci_getln(struct uci_cont + return; + + ofs += strlen(p); ++ pctx->buf_filled = ofs; + if (pctx->buf[ofs - 1] == '\n') { + pctx->line++; + return; +@@ -120,6 +122,15 @@ static inline void addc(struct uci_conte + *pos_src += 1; + } + ++static int uci_increase_pos(struct uci_parse_context *pctx, size_t add) ++{ ++ if (pctx->pos + add > pctx->buf_filled) ++ return -EINVAL; ++ ++ pctx->pos += add; ++ return 0; ++} ++ + /* + * parse a double quoted string argument from the command line + */ +@@ -384,7 +395,8 @@ static void uci_parse_package(struct uci + char *name; + + /* command string null-terminated by strtok */ +- pctx->pos += strlen(pctx_cur_str(pctx)) + 1; ++ if (uci_increase_pos(pctx, strlen(pctx_cur_str(pctx)) + 1)) ++ uci_parse_error(ctx, "package without name"); + + ofs_name = next_arg(ctx, true, true, true); + assert_eol(ctx); +@@ -416,7 +428,8 @@ static void uci_parse_config(struct uci_ + } + + /* command string null-terminated by strtok */ +- pctx->pos += strlen(pctx_cur_str(pctx)) + 1; ++ if (uci_increase_pos(pctx, strlen(pctx_cur_str(pctx)) + 1)) ++ uci_parse_error(ctx, "config without name"); + + ofs_type = next_arg(ctx, true, false, false); + type = pctx_str(pctx, ofs_type); +@@ -466,7 +479,8 @@ static void uci_parse_option(struct uci_ + uci_parse_error(ctx, "option/list command found before the first section"); + + /* command string null-terminated by strtok */ +- pctx->pos += strlen(pctx_cur_str(pctx)) + 1; ++ if (uci_increase_pos(pctx, strlen(pctx_cur_str(pctx)) + 1)) ++ uci_parse_error(ctx, "option without name"); + + ofs_name = next_arg(ctx, true, true, false); + ofs_value = next_arg(ctx, false, false, false); +--- a/uci_internal.h ++++ b/uci_internal.h +@@ -33,6 +33,7 @@ struct uci_parse_context + const char *name; + char *buf; + int bufsz; ++ size_t buf_filled; + int pos; + }; + #define pctx_pos(pctx) ((pctx)->pos) -- 2.30.2