[PATCH v2] configure: fix out of tree build; check unsafe characters in srcdir

[PATCH v2] configure: fix out of tree build; check unsafe characters in srcdir

[Tomi Ollila]

While check for GMime session key extraction support... was made
out of tree build compatible, related (and some unrelated) unsafe
characters are now checked in notmuch source directory path.

The known unsafe characters in NOTMUCH_SRCDIR are:

- Single quote (') -- NOTMUCH_SRCDIR='${NOTMUCH_SRCDIR}'
  is written to sh.config in configure line 1328.

- Double quote (") -- configure line 521 *now* writes "$srcdir"
  into generated c source file ($NOTMUCH_SRCDIR includes $srcdir).

- Backslash (\) could also be problematic in configure line 521.

- The added $ and ` are potentially unsafe -- inside double quotes
  in shell script those have special meaning.

  Other characters don't expand inside double quoted strings.
---

V2: added missing \ to the case pattern

 configure | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 1e7b9f7a..ef81e71b 100755
--- a/configure
+++ b/configure
@@ -26,6 +26,11 @@ readonly DEFAULT_IFS="$IFS"
 srcdir=$(dirname "$0")
 NOTMUCH_SRCDIR=$(cd "$srcdir" && pwd)
 
+case $NOTMUCH_SRCDIR in ( *\'* | *['\"`$']* )
+	echo "Definitely unsafe characters in source path '$NOTMUCH_SRCDIR'".
+	exit 1
+esac
+
 subdirs="util compat lib parse-time-string completion doc emacs"
 subdirs="${subdirs} performance-test test test/test-databases"
 subdirs="${subdirs} bindings"
@@ -513,7 +518,7 @@ int main () {
 
     g_mime_init ();
     parser = g_mime_parser_new ();
-    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("test/corpora/crypto/basic-encrypted.eml", "r", &error));
+    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("$srcdir/test/corpora/crypto/basic-encrypted.eml", "r", &error));
     if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/crypto/basic-encrypted.eml\n");
 
     body = GMIME_MULTIPART_ENCRYPTED(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
@@ -533,7 +538,7 @@ EOF
         printf 'No.\nCould not make tempdir for testing session-key support.\n'
         errors=$((errors + 1))
     elif ${CC} ${CFLAGS} ${gmime_cflags} _check_session_keys.c ${gmime_ldflags} -o _check_session_keys \
-           && GNUPGHOME=${TEMP_GPG} gpg --batch --quiet --import < test/gnupg-secret-key.asc \
+           && GNUPGHOME=${TEMP_GPG} gpg --batch --quiet --import < "$srcdir"/test/gnupg-secret-key.asc \
            && SESSION_KEY=$(GNUPGHOME=${TEMP_GPG} ./_check_session_keys) \
            && [ $SESSION_KEY = 9:0BACD64099D1468AB07C796F0C0AC4851948A658A15B34E803865E9FC635F2F5 ]
     then
-- 
2.21.0

Re: [PATCH v2] configure: fix out of tree build; check unsafe characters in srcdir

[Daniel Kahn Gillmor]

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

On Mon 2019-08-26 20:03:46 +0300, Tomi Ollila wrote:
> While check for GMime session key extraction support... was made
> out of tree build compatible, related (and some unrelated) unsafe
> characters are now checked in notmuch source directory path.

LGTM.   Thanks, Tomi.

> The known unsafe characters in NOTMUCH_SRCDIR are:
>
> - Single quote (') -- NOTMUCH_SRCDIR='${NOTMUCH_SRCDIR}'
>   is written to sh.config in configure line 1328.
>
> - Double quote (") -- configure line 521 *now* writes "$srcdir"
>   into generated c source file ($NOTMUCH_SRCDIR includes $srcdir).
>
> - Backslash (\) could also be problematic in configure line 521.
>
> - The added $ and ` are potentially unsafe -- inside double quotes
>   in shell script those have special meaning.

This is a great list of concerns to have enumerated.  How did you
generate it?

Are these things that we can pick off one by one?  It'd be great to be
robust against being built in weirdly named paths in the filesystem, and
it has always bothered me that so much of our tooling is brittle in that
way.

        --dkg

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

Re: [PATCH v2] configure: fix out of tree build; check unsafe characters in srcdir

[Tomi Ollila]

On Thu, Aug 29 2019, Daniel Kahn Gillmor wrote:

> On Mon 2019-08-26 20:03:46 +0300, Tomi Ollila wrote:
>> While check for GMime session key extraction support... was made
>> out of tree build compatible, related (and some unrelated) unsafe
>> characters are now checked in notmuch source directory path.
>
> LGTM.   Thanks, Tomi.
>
>> The known unsafe characters in NOTMUCH_SRCDIR are:
>>
>> - Single quote (') -- NOTMUCH_SRCDIR='${NOTMUCH_SRCDIR}'
>>   is written to sh.config in configure line 1328.
>>
>> - Double quote (") -- configure line 521 *now* writes "$srcdir"
>>   into generated c source file ($NOTMUCH_SRCDIR includes $srcdir).
>>
>> - Backslash (\) could also be problematic in configure line 521.
>>
>> - The added $ and ` are potentially unsafe -- inside double quotes
>>   in shell script those have special meaning.
>
> This is a great list of concerns to have enumerated.  How did you
> generate it?

The enumerated cases are what I think takes care what gets expanded 
inside single quotes (none there), double quotes (and heredocs when
delimiter is not in ''s)

(and if there ever were any encoding conversions...uhh forget it now ;)

The ' was simple. Inside single quotes nothing gets expanded, second '
ends it...

Inside double quotes (and abovementioned here documents) of none of the
characters in \ " ` $ is present, nothing gets expanded. I think I 
exhausted the options. See e.g. QUOTING in bash(1) namual page for
more information...

... In that section it also mentioned ! as history expansion, In 
non-interactive shell scripts it is not on. Compare:

$ : previous command :
$ echo "'!!'"
# bash -c 'echo "!!"'

> Are these things that we can pick off one by one?  It'd be great to be
> robust against being built in weirdly named paths in the filesystem, and
> it has always bothered me that so much of our tooling is brittle in that
> way.

IMO we can pick more of these one by one if we encounter more cases...

When I picked the characters to be excluced I deliberately left out $IFS
characters (space tab newline) and would have guessed that configuring
and building would fail (and pre-planned to look that in near/far future).

To my surprise mkdir 'build dir'; cd 'build dir'; ../notmuch/configure
and then `make` just worked. :D

I did not dare to run tests -- I'm pretty sure that would fail...

>
>         --dkg

Tomi

Re: [PATCH v2] configure: fix out of tree build; check unsafe characters in srcdir

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:
> To my surprise mkdir 'build dir'; cd 'build dir'; ../notmuch/configure
> and then `make` just worked. :D
>
> I did not dare to run tests -- I'm pretty sure that would fail...
>

20-ish tests currently fail looking for json_check_nodes.py

But yeah, many other tests explode if the out of tree directory has a
space in it.

d

Re: [PATCH v2] configure: fix out of tree build; check unsafe characters in srcdir

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> While check for GMime session key extraction support... was made
> out of tree build compatible, related (and some unrelated) unsafe
> characters are now checked in notmuch source directory path.

pushed.


Note that while notmuch builds out of tree, the test suite is currently
broken, and even more broken with spaces in the build path.

d