| Message ID | 20200504204858.15009-1-michael-dev@fami-braun.de |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | rule: fix out of memory write if num_stmts is too low | expand |
On Mon, May 04, 2020 at 10:48:58PM +0200, Michael Braun wrote: > Running bridge/vlan.t with ASAN, results in the following error. > This patch fixes this > > flush table bridge test-bridge > add rule bridge test-bridge input vlan id 1 ip saddr 10.0.0.1 Thanks for your patch. Probably this patch instead?
Am 05.05.2020 14:17, schrieb Pablo Neira Ayuso: > On Mon, May 04, 2020 at 10:48:58PM +0200, Michael Braun wrote: >> Running bridge/vlan.t with ASAN, results in the following error. >> This patch fixes this >> >> flush table bridge test-bridge >> add rule bridge test-bridge input vlan id 1 ip saddr 10.0.0.1 > > Thanks for your patch. Probably this patch instead? That fixes the testcase for me as well. Though there are some more places that call list_add / list_add_tail on rule->stmts, so I'm unsure if this patch catches all similar cases, e.g: src/evaluate.c: list_add(&nstmt->list, &ctx->rule->stmts); src/evaluate.c: list_add(&nstmt->list, &ctx->rule->stmts); src/netlink_delinearize.c: list_add_tail(&stmt->list, &ctx->rule->stmts); src/netlink_delinearize.c: list_add_tail(&stmt->list, &ctx->rule->stmts); src/netlink_delinearize.c: list_add_tail(&ctx->stmt->list, &ctx->rule->stmts); src/parser_json.c: list_add_tail(&stmt->list, &rule->stmts); src/parser_json.c: list_add_tail(&stmt->list, &rule->stmts); src/xt.c: list_add_tail(&stmt->list, &ctx->rule->stmts); src/xt.c: list_add_tail(&stmt->list, &ctx->rule->stmts); Regards, M. Braun
On Tue, May 05, 2020 at 03:22:07PM +0200, michael-dev wrote: > Am 05.05.2020 14:17, schrieb Pablo Neira Ayuso: > > On Mon, May 04, 2020 at 10:48:58PM +0200, Michael Braun wrote: > > > Running bridge/vlan.t with ASAN, results in the following error. > > > This patch fixes this > > > > > > flush table bridge test-bridge > > > add rule bridge test-bridge input vlan id 1 ip saddr 10.0.0.1 > > > > Thanks for your patch. Probably this patch instead? > > That fixes the testcase for me as well. Thanks for confirming. > Though there are some more places that call list_add / list_add_tail on > rule->stmts, so I'm unsure if this patch catches all similar cases, e.g: > > src/evaluate.c: list_add(&nstmt->list, &ctx->rule->stmts); > src/evaluate.c: list_add(&nstmt->list, &ctx->rule->stmts); > src/netlink_delinearize.c: list_add_tail(&stmt->list, > &ctx->rule->stmts); > src/netlink_delinearize.c: list_add_tail(&stmt->list, > &ctx->rule->stmts); > src/netlink_delinearize.c: list_add_tail(&ctx->stmt->list, > &ctx->rule->stmts); > src/parser_json.c: list_add_tail(&stmt->list, &rule->stmts); > src/parser_json.c: list_add_tail(&stmt->list, &rule->stmts); > src/xt.c: list_add_tail(&stmt->list, &ctx->rule->stmts); > src/xt.c: list_add_tail(&stmt->list, &ctx->rule->stmts); Right, this is inconsistent. I sent a few patches for this. BTW, did you update tests/py to run it under ASAN, a patch for this would be great. Thanks.
Am 05.05.2020 21:27, schrieb Pablo Neira Ayuso: > BTW, did you update tests/py to run it under ASAN, a patch for this > would be great. I was running ./configure CFLAGS="-g -O0 -fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer" LDFLAGS="-lasan" so libnftables was compiled with ASAN. But when running the python test, it complained that libasan wasn't loaded as the first library, so I used LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.5 ./nft-test.py -d bridge/vlan.t to fix that. Some python leaks or all leaks can be suppressed as described here: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer This doesn't really look to me like it can be patched easily into the python test scripts, as it might need a reconfigure/rebuild. Regards, M. Braun
diff --git a/src/rule.c b/src/rule.c index 23b1cbfc..8b17ada0 100644 --- a/src/rule.c +++ b/src/rule.c @@ -2830,6 +2830,18 @@ static void payload_do_merge(struct stmt *sa[], unsigned int n) } } +static unsigned int payload_count_stmts(const struct rule *rule) +{ + struct stmt *stmt, *next; + unsigned int count = 0; + + list_for_each_entry_safe(stmt, next, &rule->stmts, list) { + count++; + } + + return count; +} + /** * payload_try_merge - try to merge consecutive payload match statements * @@ -2843,7 +2855,7 @@ static void payload_do_merge(struct stmt *sa[], unsigned int n) */ static void payload_try_merge(const struct rule *rule) { - struct stmt *sa[rule->num_stmts]; + struct stmt *sa[payload_count_stmts(rule)]; struct stmt *stmt, *next; unsigned int idx = 0;
Running bridge/vlan.t with ASAN, results in the following error. This patch fixes this flush table bridge test-bridge add rule bridge test-bridge input vlan id 1 ip saddr 10.0.0.1 rule.c:2870:5: runtime error: index 2 out of bounds for type 'stmt *[*]' ================================================================= ==1043==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffdd69c1350 at pc 0x7f1036f53330 bp 0x7ffdd69c1300 sp 0x7ffdd69c12f8 WRITE of size 8 at 0x7ffdd69c1350 thread T0 #0 0x7f1036f5332f in payload_try_merge /home/mbr/nftables/src/rule.c:2870 #1 0x7f1036f534b7 in rule_postprocess /home/mbr/nftables/src/rule.c:2885 #2 0x7f1036fb2785 in rule_evaluate /home/mbr/nftables/src/evaluate.c:3744 #3 0x7f1036fb627b in cmd_evaluate_add /home/mbr/nftables/src/evaluate.c:3982 #4 0x7f1036fbb9e9 in cmd_evaluate /home/mbr/nftables/src/evaluate.c:4462 #5 0x7f10370652d2 in nft_evaluate /home/mbr/nftables/src/libnftables.c:414 #6 0x7f1037065ba1 in nft_run_cmd_from_buffer /home/mbr/nftables/src/libnftables.c:447 #7 0x7f10380098ed in ffi_call_unix64 (/usr/lib/x86_64-linux-gnu/libffi.so.6+0x68ed) #8 0x7f10380092be in ffi_call (/usr/lib/x86_64-linux-gnu/libffi.so.6+0x62be) #9 0x7f10375214a6 in _ctypes_callproc (/usr/lib/python2.7/lib-dynload/_ctypes.x86_64-linux-gnu.so+0x114a6) #10 0x7f1037520e8e (/usr/lib/python2.7/lib-dynload/_ctypes.x86_64-linux-gnu.so+0x10e8e) #11 0x562dca596882 in PyObject_Call (/usr/bin/python2.7+0xd2882) #12 0x562dca5ba501 in PyEval_EvalFrameEx (/usr/bin/python2.7+0xf6501) #13 0x562dca5ba3b9 in PyEval_EvalFrameEx (/usr/bin/python2.7+0xf63b9) #14 0x562dca5b2865 in PyEval_EvalCodeEx (/usr/bin/python2.7+0xee865) #15 0x562dca5ba64d in PyEval_EvalFrameEx (/usr/bin/python2.7+0xf664d) #16 0x562dca5b2865 in PyEval_EvalCodeEx (/usr/bin/python2.7+0xee865) #17 0x562dca5babd7 in PyEval_EvalFrameEx (/usr/bin/python2.7+0xf6bd7) #18 0x562dca5b2865 in PyEval_EvalCodeEx (/usr/bin/python2.7+0xee865) #19 0x562dca5babd7 in PyEval_EvalFrameEx (/usr/bin/python2.7+0xf6bd7) #20 0x562dca5b2865 in PyEval_EvalCodeEx (/usr/bin/python2.7+0xee865) #21 0x562dca5babd7 in PyEval_EvalFrameEx (/usr/bin/python2.7+0xf6bd7) #22 0x562dca5b2865 in PyEval_EvalCodeEx (/usr/bin/python2.7+0xee865) #23 0x562dca5b21f8 in PyEval_EvalCode (/usr/bin/python2.7+0xee1f8) #24 0x562dca5e4e2e (/usr/bin/python2.7+0x120e2e) #25 0x562dca5dfd1f in PyRun_FileExFlags (/usr/bin/python2.7+0x11bd1f) #26 0x562dca5df6c9 in PyRun_SimpleFileExFlags (/usr/bin/python2.7+0x11b6c9) #27 0x562dca580187 in Py_Main (/usr/bin/python2.7+0xbc187) #28 0x7f103ab0109a in __libc_start_main ../csu/libc-start.c:308 #29 0x562dca57fae9 in _start (/usr/bin/python2.7+0xbbae9) Address 0x7ffdd69c1350 is located in stack of thread T0 SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow /home/mbr/nftables/src/rule.c:2870 in payload_try_merge Shadow bytes around the buggy address: 0x10003ad30210: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad30220: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad30230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad30240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad30250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x10003ad30260: 00 00 00 00 ca ca ca ca 00 00[cb]cb cb cb cb cb 0x10003ad30270: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad30280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad30290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad302a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10003ad302b0: 00 00 00 00 f1 f1 f1 f1 00 00 00 00 00 00 00 00 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 ==1043==ABORTING Signed-off-by: Michael Braun <michael-dev@fami-braun.de> --- src/rule.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)