[PATCH nft] scanner: treat invalid octal strings as strings

[PATCH nft] scanner: treat invalid octal strings as strings

[Jeremy Sowden]

The action associated with the `{numberstring}` pattern, passes `yytext`
to `strtoull` with base 0:

	errno = 0;
	yylval->val = strtoull(yytext, NULL, 0);
	if (errno != 0) {
		yylval->string = xstrdup(yytext);
		return STRING;
	}
	return NUM;

If `yytext` begins with '0', it will be parsed as octal.  However, this
has unexpected consequences if the token contains non-octal characters.
`09` will be parsed as 0; `0308` will be parsed as 24, because
`strtoull` and its siblings stop parsing as soon as they reach a
character in the input which is not valid for the base.

Replace the `{numberstring}` match with separate `{hexstring}` and
`{decstring}` matches.  For `{decstring}` set the base to 8 if the
leading character is '0', and handle an incompletely parsed token in
the same way as one that causes `strtoull` to set `errno`.

Thus, instead of:

  $ sudo nft -f - <<<'
    table x {
      chain y {
        ip saddr 0308 continue comment "parsed as 0.0.0.24/32"
      }
    }
  '
  $ sudo nft list chain x y
  table ip x {
    chain y {
      ip saddr 0.0.0.24 continue comment "parsed as 0.0.0.24/32"
    }
  }

We get:

  $ sudo ./src/nft -f - <<<'
  > table x {
  >   chain y {
  >     ip saddr 0308 continue comment "error"
  >   }
  > }
  > '
  /dev/stdin:4:14-17: Error: Could not resolve hostname: Name or service not known
      ip saddr 0308 continue comment "error"
               ^^^^

Add a test-case.

Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=932880
Link: https://bugzilla.netfilter.org/show_bug.cgi?id=1363
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/scanner.l                       | 18 +++++++++++++++---
 tests/shell/testcases/parsing/octal | 13 +++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100755 tests/shell/testcases/parsing/octal

diff --git a/src/scanner.l b/src/scanner.l
index 1371cd044b65..1540df508f78 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -118,7 +118,6 @@ digit		[0-9]
 hexdigit	[0-9a-fA-F]
 decstring	{digit}+
 hexstring	0[xX]{hexdigit}+
-numberstring	({decstring}|{hexstring})
 letter		[a-zA-Z]
 string		({letter}|[_.])({letter}|{digit}|[/\-_\.])*
 quotedstring	\"[^"]*\"
@@ -815,9 +814,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				return STRING;
 			}
 
-{numberstring}		{
+{hexstring}		{
 				errno = 0;
-				yylval->val = strtoull(yytext, NULL, 0);
+				yylval->val = strtoull(yytext, NULL, 16);
 				if (errno != 0) {
 					yylval->string = xstrdup(yytext);
 					return STRING;
@@ -825,6 +824,19 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				return NUM;
 			}
 
+{decstring}		{
+				int base = yytext[0] == '0' ? 8 : 10;
+				char *end;
+
+				errno = 0;
+				yylval->val = strtoull(yytext, &end, base);
+				if (errno != 0 || *end) {
+					yylval->string = xstrdup(yytext);
+					return STRING;
+				}
+				return NUM;
+			}
+
 {classid}/[ \t\n:\-},]	{
 				yylval->string = xstrdup(yytext);
 				return STRING;
diff --git a/tests/shell/testcases/parsing/octal b/tests/shell/testcases/parsing/octal
new file mode 100755
index 000000000000..09ac26e76144
--- /dev/null
+++ b/tests/shell/testcases/parsing/octal
@@ -0,0 +1,13 @@
+#!/bin/bash
+
+$NFT add table t || exit 1
+$NFT add chain t c || exit 1
+$NFT add rule t c 'ip saddr 01 continue comment "0.0.0.1"' || exit 1
+$NFT add rule t c 'ip saddr 08 continue comment "error"' && {
+  echo "'"ip saddr 08"'" not rejected 1>&2
+  exit 1
+}
+$NFT delete table t || exit 1
+
+exit 0
+
-- 
2.35.1

Re: [PATCH nft] scanner: treat invalid octal strings as strings

[Pablo Neira Ayuso]

Hi Jeremy,

On Fri, Dec 16, 2022 at 08:27:14PM +0000, Jeremy Sowden wrote:
> The action associated with the `{numberstring}` pattern, passes `yytext`
> to `strtoull` with base 0:
> 
> 	errno = 0;
> 	yylval->val = strtoull(yytext, NULL, 0);
> 	if (errno != 0) {
> 		yylval->string = xstrdup(yytext);
> 		return STRING;
> 	}
> 	return NUM;
> 
> If `yytext` begins with '0', it will be parsed as octal.  However, this
> has unexpected consequences if the token contains non-octal characters.
> `09` will be parsed as 0; `0308` will be parsed as 24, because
> `strtoull` and its siblings stop parsing as soon as they reach a
> character in the input which is not valid for the base.
> 
> Replace the `{numberstring}` match with separate `{hexstring}` and
> `{decstring}` matches.  For `{decstring}` set the base to 8 if the
> leading character is '0', and handle an incompletely parsed token in
> the same way as one that causes `strtoull` to set `errno`.
> 
> Thus, instead of:
> 
>   $ sudo nft -f - <<<'
>     table x {
>       chain y {
>         ip saddr 0308 continue comment "parsed as 0.0.0.24/32"
>       }
>     }
>   '
>   $ sudo nft list chain x y
>   table ip x {
>     chain y {
>       ip saddr 0.0.0.24 continue comment "parsed as 0.0.0.24/32"
>     }
>   }
> 
> We get:
> 
>   $ sudo ./src/nft -f - <<<'
>   > table x {
>   >   chain y {
>   >     ip saddr 0308 continue comment "error"
>   >   }
>   > }
>   > '
>   /dev/stdin:4:14-17: Error: Could not resolve hostname: Name or service not known
>       ip saddr 0308 continue comment "error"
>                ^^^^
> 
> Add a test-case.

Applied, thanks.

I am sorry I missed this patch before the release.

Re: [PATCH nft] scanner: treat invalid octal strings as strings

[Pablo Neira Ayuso]

On Thu, Dec 22, 2022 at 11:38:39AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Dec 16, 2022 at 08:27:14PM +0000, Jeremy Sowden wrote:
[...]
> > We get:
> > 
> >   $ sudo ./src/nft -f - <<<'
> >   > table x {
> >   >   chain y {
> >   >     ip saddr 0308 continue comment "error"
> >   >   }
> >   > }
> >   > '
> >   /dev/stdin:4:14-17: Error: Could not resolve hostname: Name or service not known
> >       ip saddr 0308 continue comment "error"
> >                ^^^^
> > 
> > Add a test-case.
> 
> Applied, thanks.
> 
> I am sorry I missed this patch before the release.

Hm. I thought this patch just fixes the parsing of octals.

iptables and iproute seem to support for octals?

Re: [PATCH nft] scanner: treat invalid octal strings as strings

[Jeremy Sowden]

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

On 2022-12-22, at 11:52:49 +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 22, 2022 at 11:38:39AM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Dec 16, 2022 at 08:27:14PM +0000, Jeremy Sowden wrote:
> [...]
> > > We get:
> > > 
> > >   $ sudo ./src/nft -f - <<<'
> > >   > table x {
> > >   >   chain y {
> > >   >     ip saddr 0308 continue comment "error"
> > >   >   }
> > >   > }
> > >   > '
> > >   /dev/stdin:4:14-17: Error: Could not resolve hostname: Name or service not known
> > >       ip saddr 0308 continue comment "error"
> > >                ^^^^
> > > 
> > > Add a test-case.
> > 
> > Applied, thanks.
> > 
> > I am sorry I missed this patch before the release.
> 
> Hm. I thought this patch just fixes the parsing of octals.
>
> iptables and iproute seem to support for octals?

So does nft.  However, 0308 is not valid octal, and nft was silently
truncating it to 030.

For hex and decimal, we know that the entire number string is valid in
the base and only have to worry whether it is too long and may result in
a out-of-range error.  For octal, there is also the possibility that the
string may contain 8 or 9.  This patch adds a check for this and if the
check fails the failure is handled as an error in the same way it would
be if strtoull had reported `ERANGE`.

I did consider adding an `{octalstring}` match to handle octal
separately from decimal, but in the end the solution in this patch
seemed simpler.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH nft] scanner: treat invalid octal strings as strings

[Pablo Neira Ayuso]

On Thu, Dec 22, 2022 at 11:02:20AM +0000, Jeremy Sowden wrote:
> On 2022-12-22, at 11:52:49 +0100, Pablo Neira Ayuso wrote:
> > On Thu, Dec 22, 2022 at 11:38:39AM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Dec 16, 2022 at 08:27:14PM +0000, Jeremy Sowden wrote:
> > [...]
> > > > We get:
> > > > 
> > > >   $ sudo ./src/nft -f - <<<'
> > > >   > table x {
> > > >   >   chain y {
> > > >   >     ip saddr 0308 continue comment "error"
> > > >   >   }
> > > >   > }
> > > >   > '
> > > >   /dev/stdin:4:14-17: Error: Could not resolve hostname: Name or service not known
> > > >       ip saddr 0308 continue comment "error"
> > > >                ^^^^
> > > > 
> > > > Add a test-case.
> > > 
> > > Applied, thanks.
> > > 
> > > I am sorry I missed this patch before the release.
> > 
> > Hm. I thought this patch just fixes the parsing of octals.
> >
> > iptables and iproute seem to support for octals?
> 
> So does nft.  However, 0308 is not valid octal, and nft was silently
> truncating it to 030.
> 
> For hex and decimal, we know that the entire number string is valid in
> the base and only have to worry whether it is too long and may result in
> a out-of-range error.  For octal, there is also the possibility that the
> string may contain 8 or 9.  This patch adds a check for this and if the
> check fails the failure is handled as an error in the same way it would
> be if strtoull had reported `ERANGE`.
> 
> I did consider adding an `{octalstring}` match to handle octal
> separately from decimal, but in the end the solution in this patch
> seemed simpler.

Oh well, thanks for explaining, patch is applied.