GNU bug report logs - #72756
Potential Bug/Vuln in test.c

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 72756 in the body.
You can then email your comments to 72756 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nathan Hays <nathan.hays <at> nccgroup.com>
To: "bug-coreutils <at> gnu.org" <bug-coreutils <at> gnu.org>
Subject: Potential Bug/Vuln in test.c
Date: Wed, 21 Aug 2024 17:29:27 +0000

# ENV #

Ubuntu 22.04 LTS 
COREUTILS version 8.32-4.1ubuntu1.2

*Note could be specific to Ubuntu but I believe I confirmed in source*

# Observed Behavior #

Expectation:
`var=''; [ -n $var ]; echo $?` should NOT return `0`
Reality:
`var=''; [ -n $var ]; echo $?` returns `0`

*Note that this behavior does not persist when variable is quoted* 

Test:
~~~bash
testvar=''
[ -n $testvar ]
echo -n unquote exit $?
[ -n "$testvar" ]
echo -n quote exit $?
[ -z $testvar ]
echo -z unquote exit $?
[ -z "$testvar" ]
echo -z quote exit $?
~~~

Result:
~~~bash
+ testvar=
+ '[' -n ']'
+ echo -n unquote exit 0
unquote exit 0+ '[' -n '' ']'
+ echo -n quote exit 1
quote exit 1+ '[' -z ']'
+ echo -z unquote exit 0
-z unquote exit 0
+ '[' -z '' ']'
+ echo -z quote exit 0
-z quote exit 0
~~~

# Relevant Code #

From `coreutils/src/test.c`:

lines 106-111:
~~~C
static void
unary_advance (void)
{
  advance (true);
  ++pos;
}
~~~

lines 512-514:
~~~C
    case 'n':			/* True if arg has some length. */
      unary_advance ();
      return argv[pos - 1][0] != 0;
~~~


# What it looks to me that it's doing #

Based on my interpretation of the relevant code sections, which could be completely wrong:
 
`'[' -n ']'` moves to `']'` and checks if `']'` has a length of `[0]` and returns `0` if not (so always `0`, always `TRUE`)

This would explain why the behavior is not continued when the var is quoted: 

`'[' -n '' ']'` moves to `''` and checks if `''` has a length of `[0]` and returns (not zero) correctly.

Oddly though, the `-z` argument appears to handle both situations correct despite the code being similar. 


# Word Around # 

When using `-n` with `test` (or `[`), ensure variables are always quoted to avoid a false positive when the variable is unset but unquoted.
Use `-z` exclusively instead.

This is my first time reporting anything like this so apologies in advance if formatting or other ways on how to pass on the information could be improved. 

Nathan Hays | Principal Security Consultant | NCC Group PLC

Message #8 received at 72756 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Nathan Hays <nathan.hays <at> nccgroup.com>
Cc: 72756 <at> debbugs.gnu.org
Subject: Re: bug#72756: Potential Bug/Vuln in test.c
Date: Wed, 21 Aug 2024 23:21:07 -0700

I don't see a bug there. 'test' is behaving as documented.

> Expectation:
> `var=''; [ -n $var ]; echo $?` should NOT return `0`

This expectation is incorrect, because that command should exit with 
status 0. This is because the command '[ -n $var ]' is equivalent to:

  [ -n ]

which is equivalent to:

  test -n

and this is is defined to exit successfully, because '-n' is not the 
empty string.

The presence of the shell variable is irrelevant, because the shell 
omits the expansion of $var in the context you give.

Admittedly the usage of "test" is confusing, but it's been standardized 
for decades and it's not something we'd lightly change. You can see the 
current standard here:

https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html

and the relevant text says:

  1 argument:
      Exit true (0) if $1 is not null; otherwise, exit false.

Message #11 received at 72756 <at> debbugs.gnu.org (full text, mbox):

From: "Philip Rowlands" <phr+coreutils <at> dimebar.com>
To: "Nathan Hays" <nathan.hays <at> nccgroup.com>, 72756 <at> debbugs.gnu.org
Subject: Re: bug#72756: Potential Bug/Vuln in test.c
Date: Thu, 22 Aug 2024 07:54:35 +0100

On Wed, 21 Aug 2024, at 18:29, Nathan Hays via GNU coreutils Bug Reports wrote:
> Test:
> ~~~bash
> testvar=''
> [ -n $testvar ]
> echo -n unquote exit $?
> [ -n "$testvar" ]
> echo -n quote exit $?
> [ -z $testvar ]
> echo -z unquote exit $?
> [ -z "$testvar" ]
> echo -z quote exit $?
> ~~~

For this code snippet, the shellcheck linter reports

[ -n $testvar ]
     ^------^ SC2070: -n doesn't work with unquoted arguments. Quote or use [[ ]].
     ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
[ -n "$testvar" ]

For more information:
  https://www.shellcheck.net/wiki/SC2070 -- -n doesn't work with unquoted arg...



Cheers,
Phil

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thomas Wolff <towo <at> towo.net>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#72756: Potential Bug/Vuln in test.c
Date: Thu, 22 Aug 2024 09:29:28 +0200

[Message part 1 (text/plain, inline)]

Am 22.08.2024 um 08:54 schrieb Philip Rowlands:
> On Wed, 21 Aug 2024, at 18:29, Nathan Hays via GNU coreutils Bug Reports wrote:
>> Test:
>> ~~~bash
>> testvar=''
>> [ -n $testvar ]
>> echo -n unquote exit $?
>> [ -n "$testvar" ]
>> echo -n quote exit $?
>> [ -z $testvar ]
>> echo -z unquote exit $?
>> [ -z "$testvar" ]
>> echo -z quote exit $?
>> ~~~
> For this code snippet, the shellcheck linter reports
>
> [ -n $testvar ]
>       ^------^ SC2070: -n doesn't work with unquoted arguments. Quote or use [[ ]].
>       ^------^ SC2086: Double quote to prevent globbing and word splitting.
>
> Did you mean:
> [ -n "$testvar" ]
>
> For more information:
>    https://www.shellcheck.net/wiki/SC2070  -- -n doesn't work with unquoted arg...
It's a similar confusing syntax glitch as in shell:
# cond=true # commented out with the assumption the below condition will
not be true then
if $cond
then echo surprise # but it is
fi

Cheers
Thomas

> Cheers,
> Phil
>

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.