GNU bug report logs - #30683
[PATCH] build: add a configure flag to force --sandbox

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 30683 in the body.
You can then email your comments to 30683 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: Mike Frysinger <vapier <at> gentoo.org>
To: bug-sed <at> gnu.org
Subject: [PATCH] build: add a configure flag to force --sandbox
Date: Fri,  2 Mar 2018 17:28:15 -0500

From: Mike Frysinger <vapier <at> chromium.org>

When building systems that integrate code scripts from a variety of
sources, it's hard to guarantee all users of sed are robust, and it's
not easy to make sure everyone uses --sandbox all the time.  Lets add
a configure option so people can easily build a GNU sed that always
enforces --sandbox mode.  This makes sure sed stays a dumb text tool
and can't be used as an avenue for code injection.

Consider a "benign" argument controlled by the user to a script that
is inlined as a match in a sed script.  Yes, the argument should have
been properly checked and/or sanitized, but the overall integrity of
the system shouldn't suffer because of these common mistakes.

* configure.ac: Add --enable-forced-sandbox option, and define
ENABLE_FORCED_SANDBOX when enabled.
* sed/sed.c (sandbox): Set to true when ENABLE_FORCED_SANDBOX,
else set to false.
---
 configure.ac | 7 +++++++
 sed/sed.c    | 7 ++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 4c57d682f976..8531fc2f0fe8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -123,6 +123,13 @@ fi
 AM_CONDITIONAL([TEST_SYMLINKS],
           [test "$ac_cv_func_lstat:$ac_cv_func_readlink" = yes:yes])
 
+AC_ARG_ENABLE([forced-sandbox],
+  [AS_HELP_STRING([--enable-forced-sandbox)],
+     [always run with --sandbox enabled])])
+if test "$enable_forced_sandbox" = "yes"; then
+  AC_DEFINE([ENABLE_FORCED_SANDBOX], , [Always enabled --sandbox mode])
+fi
+
 AC_ARG_ENABLE(i18n,
 [  --disable-i18n          disable internationalization (default=enabled)], ,
 enable_i18n=yes)
diff --git a/sed/sed.c b/sed/sed.c
index 65bcab5ac58a..9d4a7a888c54 100644
--- a/sed/sed.c
+++ b/sed/sed.c
@@ -55,7 +55,12 @@ bool separate_files = false;
 bool follow_symlinks = false;
 
 /* If set, opearate in 'sandbox' mode */
-bool sandbox = false;
+bool sandbox =
+#ifdef ENABLE_FORCED_SANDBOX
+  true;
+#else
+  false;
+#endif
 
 /* How do we edit files in-place? (we don't if NULL) */
 char *in_place_extension = NULL;
-- 
2.16.1

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 30683 <at> debbugs.gnu.org
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Fri, 2 Mar 2018 16:07:45 -0700

tag 30683 notabug
close 30683
stop

Hello Mike,

On Fri, Mar 02, 2018 at 05:28:15PM -0500, Mike Frysinger wrote:
> * configure.ac: Add --enable-forced-sandbox option, and define
> ENABLE_FORCED_SANDBOX when enabled.

Conceptually I like your idea (since I've added the original
--sandbox option to both gnu sed and gawk).

However,
Adding such "--enable" options to "./configure" goes against the gnu coding standards,
which say:

   No ‘--enable’ option should ever cause one feature to replace another.
   No ‘--enable’ option should ever substitute one useful behavior for
   another useful behavior. The only proper use for ‘--enable’ is for
   questions of whether to build part of the program or exclude it.
   (source: https://www.gnu.org/prep/standards/html_node/Configuration.html)

As such, I'm closing this as not a bug, but discussion can continue
by replying to this thread.

regards,
 - assaf

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

From: Eric Blake <eblake <at> redhat.com>
To: Assaf Gordon <assafgordon <at> gmail.com>, Mike Frysinger <vapier <at> gentoo.org>
Cc: 30683 <at> debbugs.gnu.org
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Fri, 2 Mar 2018 17:20:07 -0600

On 03/02/2018 05:07 PM, Assaf Gordon wrote:

> On Fri, Mar 02, 2018 at 05:28:15PM -0500, Mike Frysinger wrote:
>> * configure.ac: Add --enable-forced-sandbox option, and define
>> ENABLE_FORCED_SANDBOX when enabled.
> 
> Conceptually I like your idea (since I've added the original
> --sandbox option to both gnu sed and gawk).
> 
> However,
> Adding such "--enable" options to "./configure" goes against the gnu coding standards,
> which say:
> 
>     No ‘--enable’ option should ever cause one feature to replace another.
>     No ‘--enable’ option should ever substitute one useful behavior for
>     another useful behavior. The only proper use for ‘--enable’ is for
>     questions of whether to build part of the program or exclude it.
>     (source: https://www.gnu.org/prep/standards/html_node/Configuration.html)

Would a different spelling, such as 
'--with-forced-sandbox-default=on/off' be better?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

From: Mike Frysinger <vapier <at> gentoo.org>
To: Assaf Gordon <assafgordon <at> gmail.com>
Cc: 30683 <at> debbugs.gnu.org
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Fri, 2 Mar 2018 18:27:37 -0500

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

On 02 Mar 2018 16:07, Assaf Gordon wrote:
> tag 30683 notabug
> close 30683
> stop
> 
> Hello Mike,
> 
> On Fri, Mar 02, 2018 at 05:28:15PM -0500, Mike Frysinger wrote:
> > * configure.ac: Add --enable-forced-sandbox option, and define
> > ENABLE_FORCED_SANDBOX when enabled.
> 
> Conceptually I like your idea (since I've added the original
> --sandbox option to both gnu sed and gawk).
> 
> However,
> Adding such "--enable" options to "./configure" goes against the gnu coding standards,
> which say:
> 
>    No ‘--enable’ option should ever cause one feature to replace another.
>    No ‘--enable’ option should ever substitute one useful behavior for
>    another useful behavior. The only proper use for ‘--enable’ is for
>    questions of whether to build part of the program or exclude it.
>    (source: https://www.gnu.org/prep/standards/html_node/Configuration.html)

frankly, i consider this a bikeshed aspect, and i have no interest in
debating what would be acceptable.  so if you tell me what name you
want, i'll happily adjust the patch/code.
-mike

[signature.asc (application/pgp-signature, inline)]

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 30683 <at> debbugs.gnu.org, Mike Frysinger <vapier <at> gentoo.org>
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Fri, 2 Mar 2018 16:44:40 -0700

Hello Eric, Mike,

(replying to both recent messages)

On Fri, Mar 02, 2018 at 05:20:07PM -0600, Eric Blake wrote:
> On 03/02/2018 05:07 PM, Assaf Gordon wrote:
> 
> >Adding such "--enable" options to "./configure" goes against the gnu coding standards,
>
> Would a different spelling, such as '--with-forced-sandbox-default=on/off'
> be better?

On Fri, Mar 2, 2018 at 4:27 PM, Mike Frysinger <vapier <at> gentoo.org> wrote:
> [...] so if you tell me what name you
> want, i'll happily adjust the patch/code.

I generally do not object to this feature (regardless of the name).

However when I previously suggested something similar for coreutils [1]
(a ./configure flag to change the default 'ls' quoting style),
it was explained that such things should be avoided [2].

[1] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00057.html
[2] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00058.html

Perhaps there's no issue in adding it to sed - in which case I'm happy to commit it.

Jim - what do you think?

regards,
 - assaf

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

From: Jim Meyering <jim <at> meyering.net>
To: Assaf Gordon <assafgordon <at> gmail.com>
Cc: Mike Frysinger <vapier <at> gentoo.org>, 30683 <at> debbugs.gnu.org,
 Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Fri, 2 Mar 2018 17:34:18 -0800

On Fri, Mar 2, 2018 at 3:44 PM, Assaf Gordon <assafgordon <at> gmail.com> wrote:
> Hello Eric, Mike,
>
> (replying to both recent messages)
>
> On Fri, Mar 02, 2018 at 05:20:07PM -0600, Eric Blake wrote:
>> On 03/02/2018 05:07 PM, Assaf Gordon wrote:
>>
>> >Adding such "--enable" options to "./configure" goes against the gnu coding standards,
>>
>> Would a different spelling, such as '--with-forced-sandbox-default=on/off'
>> be better?
>
> On Fri, Mar 2, 2018 at 4:27 PM, Mike Frysinger <vapier <at> gentoo.org> wrote:
>> [...] so if you tell me what name you
>> want, i'll happily adjust the patch/code.
>
> I generally do not object to this feature (regardless of the name).
>
> However when I previously suggested something similar for coreutils [1]
> (a ./configure flag to change the default 'ls' quoting style),
> it was explained that such things should be avoided [2].
>
> [1] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00057.html
> [2] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00058.html
>
> Perhaps there's no issue in adding it to sed - in which case I'm happy to commit it.
>
> Jim - what do you think?

Hi Assaf,

I think you made the right call by declining this change. The trouble
with any such configure-time option is that then any script that
requires a legitimate use of those sed commands would fail.

Mike, it sounds like you want an environment in which every sed use
would resolve to a specially-built sed binary. Given that you can do
that, can't you interpose a wrapper that invokes the real sed with
--sandbox?

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

From: Mike Frysinger <vapier <at> gentoo.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: 30683 <at> debbugs.gnu.org, Assaf Gordon <assafgordon <at> gmail.com>,
 Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Sun, 4 Mar 2018 01:19:20 -0500

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

On 02 Mar 2018 17:34, Jim Meyering wrote:
> On Fri, Mar 2, 2018 at 3:44 PM, Assaf Gordon <assafgordon <at> gmail.com> wrote:
> > Hello Eric, Mike,
> >
> > (replying to both recent messages)
> >
> > On Fri, Mar 02, 2018 at 05:20:07PM -0600, Eric Blake wrote:
> >> On 03/02/2018 05:07 PM, Assaf Gordon wrote:
> >>
> >> >Adding such "--enable" options to "./configure" goes against the gnu coding standards,
> >>
> >> Would a different spelling, such as '--with-forced-sandbox-default=on/off'
> >> be better?
> >
> > On Fri, Mar 2, 2018 at 4:27 PM, Mike Frysinger <vapier <at> gentoo.org> wrote:
> >> [...] so if you tell me what name you
> >> want, i'll happily adjust the patch/code.
> >
> > I generally do not object to this feature (regardless of the name).
> >
> > However when I previously suggested something similar for coreutils [1]
> > (a ./configure flag to change the default 'ls' quoting style),
> > it was explained that such things should be avoided [2].
> >
> > [1] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00057.html
> > [2] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00058.html
> >
> > Perhaps there's no issue in adding it to sed - in which case I'm happy to commit it.
> >
> > Jim - what do you think?
> 
> I think you made the right call by declining this change. The trouble
> with any such configure-time option is that then any script that
> requires a legitimate use of those sed commands would fail.

those scripts should fail on these systems

> Mike, it sounds like you want an environment in which every sed use
> would resolve to a specially-built sed binary. Given that you can do
> that, can't you interpose a wrapper that invokes the real sed with
> --sandbox?

that would add useless (to me) overhead on the system and simultaneously
not [satisfactorily] resolve the issue.  we want to kill all such escapes
on the system.

what about a configure option to disable these commands entirely at compile
time ?

i don't really understand the argument of "adding a wrapper `sed` that adds
--sandbox all the time is OK, but changing `sed` to always use --sandbox is
not OK".  how is this any different to "legitimate" scripts ?
-mike

[signature.asc (application/pgp-signature, inline)]

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

From: Jim Meyering <jim <at> meyering.net>
To: Jim Meyering <jim <at> meyering.net>, Assaf Gordon <assafgordon <at> gmail.com>, 
 Eric Blake <eblake <at> redhat.com>, 30683 <at> debbugs.gnu.org
Subject: Re: bug#30683: [PATCH] build: add a configure flag to force --sandbox
Date: Sun, 4 Mar 2018 11:21:51 -0800

On Sat, Mar 3, 2018 at 10:19 PM, Mike Frysinger <vapier <at> gentoo.org> wrote:
> On 02 Mar 2018 17:34, Jim Meyering wrote:
>> On Fri, Mar 2, 2018 at 3:44 PM, Assaf Gordon <assafgordon <at> gmail.com> wrote:
>> > Hello Eric, Mike,
>> >
>> > (replying to both recent messages)
>> >
>> > On Fri, Mar 02, 2018 at 05:20:07PM -0600, Eric Blake wrote:
>> >> On 03/02/2018 05:07 PM, Assaf Gordon wrote:
>> >>
>> >> >Adding such "--enable" options to "./configure" goes against the gnu coding standards,
>> >>
>> >> Would a different spelling, such as '--with-forced-sandbox-default=on/off'
>> >> be better?
>> >
>> > On Fri, Mar 2, 2018 at 4:27 PM, Mike Frysinger <vapier <at> gentoo.org> wrote:
>> >> [...] so if you tell me what name you
>> >> want, i'll happily adjust the patch/code.
>> >
>> > I generally do not object to this feature (regardless of the name).
>> >
>> > However when I previously suggested something similar for coreutils [1]
>> > (a ./configure flag to change the default 'ls' quoting style),
>> > it was explained that such things should be avoided [2].
>> >
>> > [1] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00057.html
>> > [2] https://lists.gnu.org/archive/html/bug-coreutils/2016-02/msg00058.html
>> >
>> > Perhaps there's no issue in adding it to sed - in which case I'm happy to commit it.
>> >
>> > Jim - what do you think?
>>
>> I think you made the right call by declining this change. The trouble
>> with any such configure-time option is that then any script that
>> requires a legitimate use of those sed commands would fail.
>
> those scripts should fail on these systems

Maybe it would help to explain what your intended use case is. I.e.,
why do you want this?

Do you want to do something similar for perl, awk, python, etc?

>> Mike, it sounds like you want an environment in which every sed use
>> would resolve to a specially-built sed binary. Given that you can do
>> that, can't you interpose a wrapper that invokes the real sed with
>> --sandbox?
>
> that would add useless (to me) overhead on the system and simultaneously
> not [satisfactorily] resolve the issue.  we want to kill all such escapes
> on the system.

Sure, there would be overhead for the interpreter and interposed
"exec". Other than that, what is not satisfactory?

> what about a configure option to disable these commands entirely at compile
> time ?

That runs afoul of the same guideline: it changes the language that sed accepts.

> i don't really understand the argument of "adding a wrapper `sed` that adds
> --sandbox all the time is OK, but changing `sed` to always use --sandbox is
> not OK".  how is this any different to "legitimate" scripts ?

Currently, any GNU sed binary can be expected to provide certain
fundamental features.
If we provide the suggested configure option, that implies that robust
scripts may want to (or have to) detect the new variant and do
something differently when it is found. But this is just the first
such option. If we make it easy to add a second such configure-time
option, there would then be four possible variants. Clearly that would
not scale.

Previous Next

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