diff mbox

[nft,1/4] evaluate: Fix datalen checks in expr_evaluate_string()

Message ID 1471017610-3473-2-git-send-email-phil@nwl.cc
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Phil Sutter Aug. 12, 2016, 4 p.m. UTC 
This part of the code is pretty weird due to suboptimal variable name
choice: 'data', 'len', 'datalen', 'data_len'.

But even without understanding all of it, the code checking 'datalen - 1
>= 0' assumes 'datalen - 1' may actually become negative, which is not
true since it is unsigned. So make 'datalen' a signed integer instead.

Another issue is the check for "data[datalen] != '*'" which will access
unallocated memory if 'strlen(data) == 0'. So make sure 'datalen >= 0'
before using it as array index.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Pablo Neira Ayuso Aug. 17, 2016, 2:41 p.m. UTC | #1 
On Fri, Aug 12, 2016 at 06:00:07PM +0200, Phil Sutter wrote:
> This part of the code is pretty weird due to suboptimal variable name
> choice: 'data', 'len', 'datalen', 'data_len'.
> 
> But even without understanding all of it, the code checking 'datalen - 1
> >= 0' assumes 'datalen - 1' may actually become negative, which is not
> true since it is unsigned. So make 'datalen' a signed integer instead.
> 
> Another issue is the check for "data[datalen] != '*'" which will access
> unallocated memory if 'strlen(data) == 0'. So make sure 'datalen >= 0'
> before using it as array index.

We don't allow empty strings from our flex scanner as string, so we
assume the string is at least 1.

You can probably add an assert() here instead.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/src/evaluate.c b/src/evaluate.c
index 87f5a6d77d485..523eedabe84ac 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -211,8 +211,9 @@  static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 {
 	struct expr *expr = *exprp;
-	unsigned int len = div_round_up(expr->len, BITS_PER_BYTE), datalen;
+	unsigned int len = div_round_up(expr->len, BITS_PER_BYTE);
 	struct expr *value, *prefix;
+	int datalen;
 	int data_len = ctx->ectx.len > 0 ? ctx->ectx.len : len + 1;
 	char data[data_len];
 
@@ -228,7 +229,8 @@  static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 	mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len);
 
 	datalen = strlen(data) - 1;
-	if (data[datalen] != '*') {
+	if (datalen >= 0 &&
+	    data[datalen] != '*') {
 		/* We need to reallocate the constant expression with the right
 		 * expression length to avoid problems on big endian.
 		 */