Re: [PATCH] gnulib-tool: do not use $(top_srcdir) unquoted; may be tainted

[Jim Meyering]

Ralf Wildenhues <address@hidden> wrote:
> Hi Jim,
>
> * Jim Meyering wrote on Mon, Nov 24, 2008 at 09:02:51PM CET:
>> [ along the way I noticed that my gnulib-tool patch is wrong,
>>   since single quotes in the context of a Makefile dependency
>>   list are interpreted literally ]
>
> Indeed.
>
>> But I did find a bug to fix... in automake.
>
>> While $(top_srcdir) values usually look like ../src,
>> but sometimes they are full, absolute names.  It's the latter
>> case I was trying to protect against with the gnulib-tool patch.
>>
>> For the record, you can cause trouble by building with a
>> source directory name containing e.g., a space, and invoking
>> configure via an absolute name.
>
> Been there before, tried to push the exact patch you propose.
> It doesn't help but cover up.  If the corresponding code complains,
> the user used a path he shouldn't have used.  "Don't do that when
> it hurts."
>
> It would be an improvement if the sanity.m4 code produced a more
> helpful error, though.

Hi Ralf,

I think it's a fine idea to disallow bogus (and potentially dangerous)
absolute names in automake's sanity.m4, but the existing code is not
sufficient.

To demonstrate (as non-root), create a directory named 'a;$(halt);'
cd into it, unpack an automake/autoconf-using tarball, then run
"$PWD/configure".

When I just did that with coreutils.tar.gz, I got this:

  $ "$PWD/configure"
  checking for a BSD-compatible install... /p/bin/install -c
  checking whether build environment is sane... yes
  /bin/sh: /t/am-test/a: No such file or directory
  halt: Need to be root
  /t/am-test/a;$(halt);/coreutils-7.0.76-cb90a/configure: line 3275: 
/coreutils-7.0.76-cb90a/build-aux/missing: No such file or directory
  configure: WARNING: `missing' script is too old or missing
  checking for a thread-safe mkdir -p... /p/bin/mkdir -p
  ...
  configure: creating ./config.status
  halt: Need to be root
  ...
  config.status: creating Makefile
  config.status: creating doc/Makefile
  config.status: creating po/Makefile
  $

To my surprise, I found that it invokes halt even when
invoked via the usual "./configure":

  $ ./configure
  checking for a BSD-compatible install... /p/bin/install -c
  checking whether build environment is sane... yes
  /bin/sh: /t/am-test/a: No such file or directory
  halt: Need to be root
  ./configure: line 3275: /coreutils-7.0.70-186b6/build-aux/missing: No such 
file or directory
  configure: WARNING: `missing' script is too old or missing
  checking for a thread-safe mkdir -p... /p/bin/mkdir -p

That's due to the eval of "$MISSING...".

And that's only in the relative safety of the configure script.
I'd expect many more problems in arbitrary Makefile.am files.

This is why I care about unprotected uses of $(top_srcdir)

Here's another patch, just for discussion:

[ hmm. just noticed that the autoconf-set $as_cr_* variables
  are not documented, so we'd have to spell them out.  Actually,
  you might argue that this sanity check belongs in autoconf itself,
  in which case it _could_ use them.  Also, I'm not confident that
  the list of acceptable bytes is right. ]

>From 3888a6645ca10093ca26373c9d22b49181f5bed3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 25 Nov 2008 10:53:02 +0100
Subject: [PATCH] sanity.m4: disallow risky $PWD

* m4/sanity.m4 (AM_SANITY_CHECK): Abort configure if `pwd` contains
whitespace or a shell meta-character.
---
 m4/sanity.m4 |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/m4/sanity.m4 b/m4/sanity.m4
index 2e76b10..e75b08d 100644
--- a/m4/sanity.m4
+++ b/m4/sanity.m4
@@ -1,13 +1,13 @@
 # Check to make sure that the build environment is sane.    -*- Autoconf -*-

-# Copyright (C) 1996, 1997, 2000, 2001, 2003, 2005
+# Copyright (C) 1996, 1997, 2000, 2001, 2003, 2005, 2008
 # Free Software Foundation, Inc.
 #
 # This file is free software; the Free Software Foundation
 # gives unlimited permission to copy and/or distribute it,
 # with or without modifications, as long as this notice is preserved.

-# serial 4
+# serial 5

 # AM_SANITY_CHECK
 # ---------------
@@ -16,6 +16,14 @@ AC_DEFUN([AM_SANITY_CHECK],
 # Just in case
 sleep 1
 echo timestamp > conftest.file
+
+# If the absolute working directory name contains any byte that
+# would be interpreted as a non-literal by the shell, reject it.
+case `pwd` in
+  [^$as_cr_letters$as_cr_LETTERS$as_cr_digits._,+:/@%=-])
+    AC_MSG_ERROR([unsafe absolute working directory name]);;
+esac
+
 # Do `set' in a subshell so we don't clobber the current shell's
 # arguments.  Must try -L first in case configure is actually a
 # symlink; some systems play weird games with the mod time of symlinks
--
1.6.0.4.1044.g77718