[PATCH] rule: fix out of memory write if num_stmts is too low

[PATCH] rule: fix out of memory write if num_stmts is too low

[Michael Braun]

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(-)

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;
 
-- 
2.20.1

Re: [PATCH] rule: fix out of memory write if num_stmts is too low

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

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?

[-- Attachment #2: fix-num-stmts.patch --]
[-- Type: text/x-diff, Size: 1635 bytes --]

diff --git a/src/evaluate.c b/src/evaluate.c
index 597141317000..26dfba2c6a74 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -645,6 +645,12 @@ static bool proto_is_dummy(const struct proto_desc *desc)
 	return desc == &proto_inet || desc == &proto_netdev;
 }
 
+static void ctx_stmt_add(struct eval_ctx *ctx, struct stmt *nstmt)
+{
+	list_add_tail(&nstmt->list, &ctx->stmt->list);
+	ctx->rule->num_stmts++;
+}
+
 static int resolve_protocol_conflict(struct eval_ctx *ctx,
 				     const struct proto_desc *desc,
 				     struct expr *payload)
@@ -659,7 +665,7 @@ static int resolve_protocol_conflict(struct eval_ctx *ctx,
 		if (err < 0)
 			return err;
 
-		list_add_tail(&nstmt->list, &ctx->stmt->list);
+		ctx_stmt_add(ctx, nstmt);
 	}
 
 	assert(base <= PROTO_BASE_MAX);
@@ -673,7 +679,7 @@ static int resolve_protocol_conflict(struct eval_ctx *ctx,
 		return 1;
 
 	payload->payload.offset += ctx->pctx.protocol[base].offset;
-	list_add_tail(&nstmt->list, &ctx->stmt->list);
+	ctx_stmt_add(ctx, nstmt);
 
 	return 0;
 }
@@ -698,7 +704,8 @@ static int __expr_evaluate_payload(struct eval_ctx *ctx, struct expr *expr)
 	if (desc == NULL) {
 		if (payload_gen_dependency(ctx, payload, &nstmt) < 0)
 			return -1;
-		list_add_tail(&nstmt->list, &ctx->stmt->list);
+
+		ctx_stmt_add(ctx, nstmt);
 	} else {
 		/* No conflict: Same payload protocol as context, adjust offset
 		 * if needed.
@@ -841,7 +848,7 @@ static int ct_gen_nh_dependency(struct eval_ctx *ctx, struct expr *ct)
 
 	nstmt = expr_stmt_alloc(&dep->location, dep);
 
-	list_add_tail(&nstmt->list, &ctx->stmt->list);
+	ctx_stmt_add(ctx, nstmt);
 	return 0;
 }
 

Re: [PATCH] rule: fix out of memory write if num_stmts is too low

[michael-dev]

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

Re: [PATCH] rule: fix out of memory write if num_stmts is too low

[Pablo Neira Ayuso]

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.

Re: [PATCH] rule: fix out of memory write if num_stmts is too low

[michael-dev]

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