[PATCH] var.c: check for valid variable name before printing in "export -p"

[PATCH] var.c: check for valid variable name before printing in "export -p"

[harald]

From: Harald Hoyer <harald@redhat.com>

"export -p" prints all environment variables, without checking if the
environment variable is a valid dash variable name.

IMHO, the only valid usecase for "export -p" is to eval the output.

$ eval $(export -p); echo OK
OK

Without this patch the following test does error out with:

test.py:
import os
os.environ["test-test"]="test"
os.environ["test_test"]="test"
os.execv("./dash", [ './dash', '-c', 'eval $(export -p); echo OK' ])

$ python test.py
./dash: 1: export: test-test: bad variable name

Of course the results can be more evil, if the environment variable
name is crafted, that it injects valid shell code.
---
 src/var.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/var.c b/src/var.c
index 027beff..06771d3 100644
--- a/src/var.c
+++ b/src/var.c
@@ -409,12 +409,15 @@ showvars(const char *prefix, int on, int off)
 	for (; ep < epend; ep++) {
 		const char *p;
 		const char *q;
-
+		const char *r;
+		r = endofname(*ep);
 		p = strchrnul(*ep, '=');
 		q = nullstr;
-		if (*p)
+		if (*p) {
+			if (p != r)
+				continue;
 			q = single_quote(++p);

Re: [PATCH] var.c: check for valid variable name before printing in "export -p"

[Herbert Xu]

On Tue, Feb 14, 2012 at 10:48:48AM +0000, harald@redhat.com wrote:
> From: Harald Hoyer <harald@redhat.com>
> 
> "export -p" prints all environment variables, without checking if the
> environment variable is a valid dash variable name.
> 
> IMHO, the only valid usecase for "export -p" is to eval the output.

Thanks a lot for the report and patch.

I'd prefer to fix this up at entry into the shell rather than
when we print out the variables.  So how about this patch?

commit 46d3c1a614f11f0d40a7e73376359618ff07abcd
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Sat Feb 25 15:35:18 2012 +0800

    [VAR] Sanitise environment variable names on entry
    
    On Tue, Feb 14, 2012 at 10:48:48AM +0000, harald@redhat.com wrote:
    >
    > "export -p" prints all environment variables, without checking if the
    > environment variable is a valid dash variable name.
    >
    > IMHO, the only valid usecase for "export -p" is to eval the output.
    >
    > $ eval $(export -p); echo OK
    > OK
    >
    > Without this patch the following test does error out with:
    >
    > test.py:
    > import os
    > os.environ["test-test"]="test"
    > os.environ["test_test"]="test"
    > os.execv("./dash", [ './dash', '-c', 'eval $(export -p); echo OK' ])
    >
    > $ python test.py
    > ./dash: 1: export: test-test: bad variable name
    >
    > Of course the results can be more evil, if the environment variable
    > name is crafted, that it injects valid shell code.
    
    This patch fixes the issue by sanitising all environment variable names
    upon entry into the shell.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index d1e84c9..8686332 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2012-02-25  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Sanitise environment variable names on entry.
+
 2011-08-17  David S. Miller <davem@davemloft.net>
 
 	* Allow building without LINEO support.
diff --git a/src/var.c b/src/var.c
index 027beff..dc90249 100644
--- a/src/var.c
+++ b/src/var.c
@@ -136,7 +136,8 @@ INIT {
 
 	initvar();
 	for (envp = environ ; *envp ; envp++) {
-		if (strchr(*envp, '=')) {
+		p = endofname(*envp);
+		if (p != *envp && *p == '=') {
 			setvareq(*envp, VEXPORT|VTEXTFIXED);
 		}
 	}

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] var.c: check for valid variable name before printing in "export -p"

[Jilles Tjoelker]

On Sat, Feb 25, 2012 at 06:36:36PM +1100, Herbert Xu wrote:
> On Tue, Feb 14, 2012 at 10:48:48AM +0000, harald@redhat.com wrote:
> > From: Harald Hoyer <harald@redhat.com>

> > "export -p" prints all environment variables, without checking if the
> > environment variable is a valid dash variable name.

> > IMHO, the only valid usecase for "export -p" is to eval the output.

> Thanks a lot for the report and patch.

> I'd prefer to fix this up at entry into the shell rather than
> when we print out the variables.  So how about this patch?

Such a change would change other things than just "set" and "export -p".
It would also not propagate environment variables with invalid names to
child processes. For example:

env -i not-a-name=1 PATH="$PATH" dash -c env | grep not-a-name

Most shells pass the environment variable through, such as bash, zsh,
ksh93 and most ash derivatives. However, the original Bourne shell and
pdksh/mksh do not.

I think it is best to pass them through so that executing a utility with
and without the shell are as similar as possible (most versions of
make(1) assume this is the case by skipping the shell if the command is
simple enough) and particularly for dash for historical/compatibility
reasons.

I did something similar to the OP's patch in FreeBSD (SVN r223183):
"set" and "export -p" skip variables with invalid names.

Note that this problem cannot occur with "readonly -p" because only
variables with a proper name can be marked read-only.

-- 
Jilles Tjoelker

Re: [PATCH] var.c: check for valid variable name before printing in "export -p"

[Herbert Xu]

On Sat, Feb 25, 2012 at 03:30:04PM +0100, Jilles Tjoelker wrote:
>
> Most shells pass the environment variable through, such as bash, zsh,
> ksh93 and most ash derivatives. However, the original Bourne shell and
> pdksh/mksh do not.

Do you know of any genuine uses of such environment variables?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] var.c: check for valid variable name before printing in "export -p"

[Eric Blake]

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

On 02/25/2012 07:31 AM, Herbert Xu wrote:
> On Sat, Feb 25, 2012 at 03:30:04PM +0100, Jilles Tjoelker wrote:
>>
>> Most shells pass the environment variable through, such as bash, zsh,
>> ksh93 and most ash derivatives. However, the original Bourne shell and
>> pdksh/mksh do not.
> 
> Do you know of any genuine uses of such environment variables?

POSIX states that applications must not rely on such pass-through:
http://austingroupbugs.net/view.php?id=168

So while it might indeed be useful to pass through invalid names, such
an application is broken for expecting it to work, and I'm okay with
this patch as-is.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [PATCH] var.c: check for valid variable name before printing in "export -p"

[Herbert Xu]

On Sat, Feb 25, 2012 at 07:53:24AM -0700, Eric Blake wrote:
>
> POSIX states that applications must not rely on such pass-through:
> http://austingroupbugs.net/view.php?id=168
> 
> So while it might indeed be useful to pass through invalid names, such
> an application is broken for expecting it to work, and I'm okay with
> this patch as-is.

Great.  From a security point of view I'd rather not pass them
through.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] var.c: check for valid variable name before printing in "export -p"

[Jilles Tjoelker]

On Sun, Feb 26, 2012 at 01:31:55AM +1100, Herbert Xu wrote:
> On Sat, Feb 25, 2012 at 03:30:04PM +0100, Jilles Tjoelker wrote:

> > Most shells pass the environment variable through, such as bash,
> > zsh, ksh93 and most ash derivatives. However, the original Bourne
> > shell and pdksh/mksh do not.

> Do you know of any genuine uses of such environment variables?

No, but I know that I do not know. I recommend noting the (small)
potential for breakage in the changelog. In a large body of software,
subtle features are often depended on somewhere, be it inadvertently.

-- 
Jilles Tjoelker