You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Stenberg <da...@haxx.se> on 2002/04/28 20:33:37 UTC
Reminder: $EDITOR with spaces?
My patch that makes svn support $EDITOR containing spaces wasn't exactly
turned down but it didn't get much support either.
Let me put the question differently now, does anyone object to my solution?
- Normal spaces are used as separators
- Escaped spaces ("\ ") are used for embedding spaces in paths/commands
--
Daniel Stenberg - http://daniel.haxx.se - +46-705-44 31 77
ech`echo xiun|tr nu oc|sed 'sx\([sx]\)\([xoi]\)xo un\2\1 is xg'`ol
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Zack Weinberg <za...@codesourcery.com>.
On Sun, Apr 28, 2002 at 10:48:51PM +0200, Daniel Stenberg wrote:
> system() isn't portable, and it isn't exactly the safest function in
> the world.
Portability is a red herring. The function is guaranteed to exist by
the C standard. Portability of the *argument* (i.e. the value of
$EDITOR) is the responsibility of the user.
IMO safety is also a red herring -- as someone else said downthread,
the user can _already_ set $EDITOR to any value and have svn execute a
program based on that.
One remaining concern is quoting the filename argument. However, as I
understand it, $EDITOR is invoked only to generate log messages with,
which means Subversion controls the filename and can simply avoid
using shell metacharacters there.
zw
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Mark Benedetto King <bk...@answerfriend.com>.
On Mon, Apr 29, 2002 at 12:25:37AM -0700, David Mankin wrote:
> Good point, Ben. We can (should? must?) do something like Perl taint
> checking
> does: refuse to invoke an $EDITOR if we're running setuid. Even if we
> did escape all {semicolon,pipe,tick,etc.}, setuid binaries could still
> be exploited with:
>
> export EDITOR="my-script-to-do-something-nasty.sh"
>
> setuidcommit any.file
>
Well, my point was that the person building the setuid wrapper around
svn might be smart enough to do *some* input validation but not smart
enough to do *enough*.
The taint idea is great, but I'm not sure what APR can tell us
about whether we're running with elevated priveledges. Maybe
it's best to put "caveat emptor" in the fine print and be
done with it.
> When not running setuid, I still think system() is the best way to go
> because it provides the same semantics for $EDITOR as other programs
> which share the *very same variable*. If we want to do our own parsing
> for safety/cross-platformness/etc to the $SVN_EDITOR variable, that's
> fine, because that's not breaking compatibility with other programs.
>
> I haven't come up with a good example, but here's a contrived example of
> how doing our own parsing (different from the shell) could be bad:
>
> export EDITOR="vim -n"
> cvs commit somefile.x
> svn commit somefile.y
>
> If I have a "vim -n" executable somewhere in my path, then it will be
> called by svn but not cvs. Not what I would expect as a longtime CVS
> user. (See, I told you it was contrived.)
>
> Anyway, I say don't muck with the standard meaning of $EDITOR which
> seems to be "pass it to system". (Though I don't know anything about
> how this applies on Windows.)
This is a very compelling argument.
>
> And never invoke system() on any tainted (user specified) data when
> running setuid. (Or open output files whose names are derived from
> tainted data. Or system()/exec() any program when $PATH hasn't been set
> to an untainted value. Or run any world-writable executable. Or any
> executable in a world writable directory. Or... there are probably a few
> dozen things to watch out for when running setuid.) See the perlsec man
> page for more.
>
> Being safe sounds like a lot of work. Instead, maybe we should bail out
> if running setuid? Or loudly warn a whole lot? Or should we just put a
> warning somewhere in small print at the bottom of INSTALL?
>
I like this "warning in install document" idea more and more...
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
Mark Benedetto King <bk...@answerfriend.com> writes:
> > > and will eventuall call, as uid=svn:
> > >
> > > EDITOR="/path/to/editor foo" svn commit
> > >
> > > Let's not try to understand *why* the administrator might
> > > want to do this, just that it is possible. :-)
> >
> > Here, the problem is not with svn using system(). Instead, the
> > problem is with the setuid program mycommit building a command line
> > from user input without proper quoting. A big no-no in any setuid
>
> The problem is not with mycommit or with system(). It's with
> the way they're (hypothetically) interacting. mycommit is
> expecting execl() semantics and system() doesn't give those
> semantics. This has bitten many programmers many times.
No it isn't. Expecting execl() semantics would be expecting svn to
execute the binary "editor foo" (with a space in the filename) in the
directory "/path/to", because that's what execl() would have done.
> It might be a "can't win, don't use system()" type scenario,
> though. Look at gets(). It's so evil, that it has been
> thoroughly deprecrated. Is gets() secure? Sure, if it never
> runs with elevated privileges or if the stdin has been sufficient
> constrained so as to ensure that lines will always be short enough.
> Most people have decided, at least for gets(), that the answer
> is "don't use gets()".
Now you're confusing things. There is a perfectly good reason for not
using gets(): Using gets() will cause your program to crash on some
input. That is undesirable even if you are not concerned about
security aspects.
> > need to reset $EDITOR regardless. It doesn't take any "craftily
> > constructed $EDITOR variables" to exploit a setuid program that calls
> > $EDITOR. You can even leave it set to "emacs", just do M-x shell when
>
> Sure it does, if it the user expects execl() semantics
> and gets system() semantics.
If the program uses a $EDITOR entirely provided by the user, then
nothing crafty is needed to exploit it. If the program tries to
construct a $EDITOR using user input for some kind of argument, then
it isn't expecting execl() semantics, as that would not allow for any
arguments at all.
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Mark Benedetto King <bk...@answerfriend.com>.
On Mon, Apr 29, 2002 at 01:13:54PM +0200, Marcus Comstedt wrote:
>
> Mark Benedetto King <bk...@answerfriend.com> writes:
>
> > Historically, many vulnerabilities have resulted from insufficient
> > input validation around system() calls. Considering the complexity
> > of /bin/sh, this is not suprising.
>
> Certainly. However, they are not really relevant here.
>
>
> > This will seem a little contrived, but it is an example:
> >
> > Let's say an administrator wants to build s setuid-svn executable
> > that is run as follows:
> >
> > mycommit foo
> >
> > and will eventuall call, as uid=svn:
> >
> > EDITOR="/path/to/editor foo" svn commit
> >
> > Let's not try to understand *why* the administrator might
> > want to do this, just that it is possible. :-)
>
> Here, the problem is not with svn using system(). Instead, the
> problem is with the setuid program mycommit building a command line
> from user input without proper quoting. A big no-no in any setuid
The problem is not with mycommit or with system(). It's with
the way they're (hypothetically) interacting. mycommit is
expecting execl() semantics and system() doesn't give those
semantics. This has bitten many programmers many times.
> program. You can not expect svn to guard you against vulnerabilities
> in _other_ programs. That's a "can't win, don't try" type scenario.
It might be a "can't win, don't use system()" type scenario,
though. Look at gets(). It's so evil, that it has been
thoroughly deprecrated. Is gets() secure? Sure, if it never
runs with elevated privileges or if the stdin has been sufficient
constrained so as to ensure that lines will always be short enough.
Most people have decided, at least for gets(), that the answer
is "don't use gets()".
>
>
> > So, that means before we call system, either we need
> > to escape all semicolons (and pipes and backticks and ...)
>
> No. That would defeat the main purpose of using system().
I agree. And it would lose if system() used a /bin/sh with
different semantics than those expected. *that* is a "can't
win, don't try" problem. :-) I was presenting it as the
"naive approach to be shot down later in my argument".
> > or we need to warn the user "$EDITOR is passed verbatim
> > to system; you are probably *not* smart enough to protect
> > yourself from craftily constructed $EDITOR variables, so
> > we recommend that you don't build setuid applications that
> > can trigger $EDITOR."
>
> Somebody who needs that warning probably wasn't fit to build setuid
> applications in the first place... Anyway, if you do a setuid app you
You're right, but they do. That's why, as has been pointed out,
perl has taint.
> need to reset $EDITOR regardless. It doesn't take any "craftily
> constructed $EDITOR variables" to exploit a setuid program that calls
> $EDITOR. You can even leave it set to "emacs", just do M-x shell when
Sure it does, if it the user expects execl() semantics
and gets system() semantics.
Considering that the huge install-base of programs that use
$EDITOR seem to all give system() semantics, it shouldn't
be too much of a surprise. However, that discrepancy
is the source of the "security problems with system".
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
cmpilato@collab.net writes:
> Philip Martin <ph...@codematters.co.uk> writes:
>
> > It turns out that cvs doesn't call system(). It does a fork/execvp.
>
> *Ding!* Didn't I mention this already a week ago? Maybe that got
> edited out of my post. Our APR-using functionality is equivalent to
> CVS's functionality. IIRC, they use strtok() to break up the EDITOR
> line into a command and args, and pass that off to execvp() ... just
> like we were doing.
So this is yet another opportunity to do better than CVS, which is the
whole purpose of Subversion. ;-)
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> Marcus Comstedt <ma...@mc.pp.se> writes:
>
> > > All that said, +1 on using system() from me, too, because
> > > it *is* a lot more powerful than execl(), and because it's
> > > what people expect from $EDITOR.
> >
> > Exactly. You are more likely to break peoples expectations by trying
> > to play big brother than by simply doing what everybody else does.
>
> It turns out that cvs doesn't call system(). It does a fork/execvp.
*Ding!* Didn't I mention this already a week ago? Maybe that got
edited out of my post. Our APR-using functionality is equivalent to
CVS's functionality. IIRC, they use strtok() to break up the EDITOR
line into a command and args, and pass that off to execvp() ... just
like we were doing.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Philip Martin <ph...@codematters.co.uk>.
Marcus Comstedt <ma...@mc.pp.se> writes:
> > All that said, +1 on using system() from me, too, because
> > it *is* a lot more powerful than execl(), and because it's
> > what people expect from $EDITOR.
>
> Exactly. You are more likely to break peoples expectations by trying
> to play big brother than by simply doing what everybody else does.
It turns out that cvs doesn't call system(). It does a fork/execvp.
http://ccvs.cvshome.org/source/browse/ccvs/src/run.c?rev=1.33&content-type=text/x-cvsweb-markup
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
Mark Benedetto King <bk...@answerfriend.com> writes:
> No, you're wrong. The semantics are different. If the administrator
> expects execl() semantics they will be very suprised (and perhaps
> compromised) when they get system() semantics.
"execl() semantics" would mean not treating the space character as a
special character at all. So either you'd have to pass an array (not
possible with environment variables; they are just strings) or not be
able to give parameters to the program at all. The "split on space"
thing that has been suggested is neither "execl() semantics", nor
"system() semantics". So if somebody was really expecting execl()
semantics (which I doubt), they'd still get a surprise.
> All that said, +1 on using system() from me, too, because
> it *is* a lot more powerful than execl(), and because it's
> what people expect from $EDITOR.
Exactly. You are more likely to break peoples expectations by trying
to play big brother than by simply doing what everybody else does.
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Mark Benedetto King <bk...@answerfriend.com>.
On Mon, Apr 29, 2002 at 12:25:12PM -0500, Karl Fogel wrote:
> > So, let's say the user runs
> >
> > mycommit "foo; cp /bin/sh /tmp/; chmod 4777 /tmp/sh"
>
> Invoking an $EDITOR by *any* means is the same as invoking arbitrary
> programs (for example, if it's Emacs, then the user can just start up
> a shell inside the editor, no need to get fancy). Bottom line: if you
> can run `svn' as user jrandom, then you can run any program as user
> jrandom. If someone has installed a setuid svn client binary, they
> deserve what they get -- the safe thing to do is make sure only
> jrandom can run svn as jrandom.
The case that I'm pointing out is where a insufficiently paranoid
operator has done enough input validation to verify the the first
white-space-separated component of $EDITOR is what he wants. The
case in which the semantics of execl() are different from system().
This has happened again and again. It should, however be sufficient
to warn in the man page that $EDITOR will be passed to system(),
rather than execl().
I guess.
>
> Remember, this isn't just about $EDITOR. It also holds when we
> provide a mechanism for invoking a user-specified diff or merge
> program; and inevitably there will be other circumstances where svn
> invokes subprocesses. We need to put a warning in the documentation,
> but otherwise let people shoot themselves in the foot if they choose.
>
I agree.
> So far, the security implications of system() don't seem any worse
> than using APR/execl; system() is effectively as portable (it's ANSI
> C, and the rest of the portability issues boil down to the argument
> passed); and using system() will get us some expected behaviors that
> we're not getting with APR.
>
No, you're wrong. The semantics are different. If the administrator
expects execl() semantics they will be very suprised (and perhaps
compromised) when they get system() semantics.
> +1 on using system(). I'm probably going to make this change unless
> someone (Daniel?) beats me to it... :-)
All that said, +1 on using system() from me, too, because
it *is* a lot more powerful than execl(), and because it's
what people expect from $EDITOR.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Daniel Stenberg <da...@haxx.se>.
On 29 Apr 2002, Karl Fogel wrote:
> +1 on using system(). I'm probably going to make this change unless
> someone (Daniel?) beats me to it... :-)
*done*
--
Daniel Stenberg - http://daniel.haxx.se - +46-705-44 31 77
ech`echo xiun|tr nu oc|sed 'sx\([sx]\)\([xoi]\)xo un\2\1 is xg'`ol
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
> So, let's say the user runs
>
> mycommit "foo; cp /bin/sh /tmp/; chmod 4777 /tmp/sh"
Invoking an $EDITOR by *any* means is the same as invoking arbitrary
programs (for example, if it's Emacs, then the user can just start up
a shell inside the editor, no need to get fancy). Bottom line: if you
can run `svn' as user jrandom, then you can run any program as user
jrandom. If someone has installed a setuid svn client binary, they
deserve what they get -- the safe thing to do is make sure only
jrandom can run svn as jrandom.
Remember, this isn't just about $EDITOR. It also holds when we
provide a mechanism for invoking a user-specified diff or merge
program; and inevitably there will be other circumstances where svn
invokes subprocesses. We need to put a warning in the documentation,
but otherwise let people shoot themselves in the foot if they choose.
So far, the security implications of system() don't seem any worse
than using APR/execl; system() is effectively as portable (it's ANSI
C, and the rest of the portability issues boil down to the argument
passed); and using system() will get us some expected behaviors that
we're not getting with APR.
+1 on using system(). I'm probably going to make this change unless
someone (Daniel?) beats me to it... :-)
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
Mark Benedetto King <bk...@answerfriend.com> writes:
> Historically, many vulnerabilities have resulted from insufficient
> input validation around system() calls. Considering the complexity
> of /bin/sh, this is not suprising.
Certainly. However, they are not really relevant here.
> This will seem a little contrived, but it is an example:
>
> Let's say an administrator wants to build s setuid-svn executable
> that is run as follows:
>
> mycommit foo
>
> and will eventuall call, as uid=svn:
>
> EDITOR="/path/to/editor foo" svn commit
>
> Let's not try to understand *why* the administrator might
> want to do this, just that it is possible. :-)
Here, the problem is not with svn using system(). Instead, the
problem is with the setuid program mycommit building a command line
from user input without proper quoting. A big no-no in any setuid
program. You can not expect svn to guard you against vulnerabilities
in _other_ programs. That's a "can't win, don't try" type scenario.
> So, that means before we call system, either we need
> to escape all semicolons (and pipes and backticks and ...)
No. That would defeat the main purpose of using system().
> or we need to warn the user "$EDITOR is passed verbatim
> to system; you are probably *not* smart enough to protect
> yourself from craftily constructed $EDITOR variables, so
> we recommend that you don't build setuid applications that
> can trigger $EDITOR."
Somebody who needs that warning probably wasn't fit to build setuid
applications in the first place... Anyway, if you do a setuid app you
need to reset $EDITOR regardless. It doesn't take any "craftily
constructed $EDITOR variables" to exploit a setuid program that calls
$EDITOR. You can even leave it set to "emacs", just do M-x shell when
Emacs starts and you'll be able to use the privileges for anything.
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Mark Benedetto King <bk...@answerfriend.com>.
On Mon, Apr 29, 2002 at 03:26:54AM -0400, Tim Waugh wrote:
> Isn't it enough to pass "$EDITOR" to the shell and get _it_ to expand
> it? Like:
>
> execl("/bin/sh","sh","-c","$EDITOR",0);
>
> Tim.
> */
It doesn't really matter who expands it; the problem is that the
semantics of the above are significantly different from
execl($EDITOR,"svn-editor",0);
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Tim Waugh <tw...@redhat.com>.
On Mon, Apr 29, 2002 at 02:40:22AM -0400, Mark Benedetto King wrote:
> One might think that, presuming that the executable
> at /path/to/editor was capable of handling arbitrary
> parameters, everything would be hunky-dory. But wait!
> We're calling system()! That means $EDITOR gets passed
> verbatim as, basically,
>
> execl("/bin/sh","sh","-c",$EDITOR,0);
>
> So, let's say the user runs
>
> mycommit "foo; cp /bin/sh /tmp/; chmod 4777 /tmp/sh"
>
> You can see where this goes wrong.
>
> So, that means before we call system, either we need
> to escape all semicolons (and pipes and backticks and ...)
> or we need to warn the user "$EDITOR is passed verbatim
> to system; you are probably *not* smart enough to protect
> yourself from craftily constructed $EDITOR variables, so
> we recommend that you don't build setuid applications that
> can trigger $EDITOR."
Isn't it enough to pass "$EDITOR" to the shell and get _it_ to expand
it? Like:
execl("/bin/sh","sh","-c","$EDITOR",0);
Tim.
*/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by David Mankin <ma...@ants.com>.
>
> So, that means before we call system, either we need
> to escape all semicolons (and pipes and backticks and ...)
> or we need to warn the user "$EDITOR is passed verbatim
> to system; you are probably *not* smart enough to protect
> yourself from craftily constructed $EDITOR variables, so
> we recommend that you don't build setuid applications that
> can trigger $EDITOR."
>
>
> --ben
>
Good point, Ben. We can (should? must?) do something like Perl taint
checking
does: refuse to invoke an $EDITOR if we're running setuid. Even if we
did escape all {semicolon,pipe,tick,etc.}, setuid binaries could still
be exploited with:
export EDITOR="my-script-to-do-something-nasty.sh"
setuidcommit any.file
When not running setuid, I still think system() is the best way to go
because it provides the same semantics for $EDITOR as other programs
which share the *very same variable*. If we want to do our own parsing
for safety/cross-platformness/etc to the $SVN_EDITOR variable, that's
fine, because that's not breaking compatibility with other programs.
I haven't come up with a good example, but here's a contrived example of
how doing our own parsing (different from the shell) could be bad:
export EDITOR="vim -n"
cvs commit somefile.x
svn commit somefile.y
If I have a "vim -n" executable somewhere in my path, then it will be
called by svn but not cvs. Not what I would expect as a longtime CVS
user. (See, I told you it was contrived.)
Anyway, I say don't muck with the standard meaning of $EDITOR which
seems to be "pass it to system". (Though I don't know anything about
how this applies on Windows.)
And never invoke system() on any tainted (user specified) data when
running setuid. (Or open output files whose names are derived from
tainted data. Or system()/exec() any program when $PATH hasn't been set
to an untainted value. Or run any world-writable executable. Or any
executable in a world writable directory. Or... there are probably a few
dozen things to watch out for when running setuid.) See the perlsec man
page for more.
Being safe sounds like a lot of work. Instead, maybe we should bail out
if running setuid? Or loudly warn a whole lot? Or should we just put a
warning somewhere in small print at the bottom of INSTALL?
-David Mankin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Mark Benedetto King <bk...@answerfriend.com>.
On Sun, Apr 28, 2002 at 11:28:56PM +0200, Marcus Comstedt wrote:
>
> As I've said before, the unsafe/exploit point is moot. We're running
> any binary/script of the users choice. That in itself is as big an
> "exploit" as you could possibly hope to get. system() in itself adds
> nothing in this department. If I wan't "echo >/etc/passwd" to be
> executed, I can just put that in a script and use the name of the
> script as $EDITOR.
>
Historically, many vulnerabilities have resulted from insufficient
input validation around system() calls. Considering the complexity
of /bin/sh, this is not suprising.
This will seem a little contrived, but it is an example:
Let's say an administrator wants to build s setuid-svn executable
that is run as follows:
mycommit foo
and will eventuall call, as uid=svn:
EDITOR="/path/to/editor foo" svn commit
Let's not try to understand *why* the administrator might
want to do this, just that it is possible. :-)
One might think that, presuming that the executable
at /path/to/editor was capable of handling arbitrary
parameters, everything would be hunky-dory. But wait!
We're calling system()! That means $EDITOR gets passed
verbatim as, basically,
execl("/bin/sh","sh","-c",$EDITOR,0);
So, let's say the user runs
mycommit "foo; cp /bin/sh /tmp/; chmod 4777 /tmp/sh"
You can see where this goes wrong.
So, that means before we call system, either we need
to escape all semicolons (and pipes and backticks and ...)
or we need to warn the user "$EDITOR is passed verbatim
to system; you are probably *not* smart enough to protect
yourself from craftily constructed $EDITOR variables, so
we recommend that you don't build setuid applications that
can trigger $EDITOR."
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
Daniel Stenberg <da...@haxx.se> writes:
> <quote>
> It's not just a "better option" but a requirement :-) system() isn't
> portable, and it isn't exactly the safest function in the world. Granted,
> this is all client stuff, but I'd still not like to see system-based
> exploits in the code.
> </quote>
As I've said before, the unsafe/exploit point is moot. We're running
any binary/script of the users choice. That in itself is as big an
"exploit" as you could possibly hope to get. system() in itself adds
nothing in this department. If I wan't "echo >/etc/passwd" to be
executed, I can just put that in a script and use the name of the
script as $EDITOR.
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Philip Martin <ph...@codematters.co.uk>.
Alan Shutko <at...@acm.org> writes:
> Philip Martin <ph...@codematters.co.uk> writes:
>
> > Perhaps we need an APR wrapper for system()?
>
> Sure, if the wrapper knows how to quote filenames for all the shells
> out there, so that operating on files with spaces, returns, colons,
> etc works on all the systems and all the shells out there.
What? The wrapper just passes the raw string to system(). It just
works.
>
> Ok, I'm revising my old proposal. Forget $EDITOR. Don't use it at
> all. It's worthless because people insist on doing stupid stuff like
>
> > EDITOR='LD_LIBRARY_PATH=/some/lib /some/program'
>
> in it.
Works fine with other programs.
> Use a new $SVNEDITOR which is defined as the name of a single
> program. No switches, no environment setting, no tricks. If a user
> wants tricks, they can put it in a script.
>
> Choosing between system() and svn_io_run_cmd() is basically a choice
> between allowing stupid $EDITOR tricks and making sure $EDITOR will
> actually work on all files. Me, I'd take correct behaviour on every
> file over worrying about edge cases whipped up by twisted minds.
A gratuitous incompatibility with other Unix programs is better?
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
Alan Shutko <at...@acm.org> writes:
> Ok, I'm revising my old proposal. Forget $EDITOR. Don't use it at
> all. It's worthless because people insist on doing stupid stuff like
>
> > EDITOR='LD_LIBRARY_PATH=/some/lib /some/program'
If somebody does that, then it's because they're on a system where
they know that this works (and probably that just '/some/program'
doesn't), so exactly how is it stupid? Environment variables are
system local. There are no portability problems with somebody setting
their $EDITOR like that. And all that's needed to make it work is to
use system(). Which, as zw pointed out, is a function that by
definition exists in every C implementation.
> Choosing between system() and svn_io_run_cmd() is basically a choice
> between allowing stupid $EDITOR tricks and making sure $EDITOR will
> actually work on all files. Me, I'd take correct behaviour on every
> file over worrying about edge cases whipped up by twisted minds.
As long as it actually works on every file _subversion start $EDITOR
on_, it should be ok, no? As zw also pointed out, svn doesn't _have_
to use strange characters in the filenames of the log message files.
Don't try to solve a larger class of problems than we're actually
faced with.
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Alan Shutko <at...@acm.org>.
Philip Martin <ph...@codematters.co.uk> writes:
> Perhaps we need an APR wrapper for system()?
Sure, if the wrapper knows how to quote filenames for all the shells
out there, so that operating on files with spaces, returns, colons,
etc works on all the systems and all the shells out there.
Ok, I'm revising my old proposal. Forget $EDITOR. Don't use it at
all. It's worthless because people insist on doing stupid stuff like
> EDITOR='LD_LIBRARY_PATH=/some/lib /some/program'
in it. Use a new $SVNEDITOR which is defined as the name of a single
program. No switches, no environment setting, no tricks. If a user
wants tricks, they can put it in a script.
Choosing between system() and svn_io_run_cmd() is basically a choice
between allowing stupid $EDITOR tricks and making sure $EDITOR will
actually work on all files. Me, I'd take correct behaviour on every
file over worrying about edge cases whipped up by twisted minds.
--
Alan Shutko <at...@acm.org> - In a variety of flavors!
Most public domain software is free, at least at first glance.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Can anyone say exactly what isn't portable about system()?
I'd prefer to use the APR way just for consistency, but if we're
losing functionality, then hmmm...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Marcus Comstedt <ma...@mc.pp.se>.
Philip Martin <ph...@codematters.co.uk> writes:
> Yes, the patch provides some of the expected behaviour on Unix. I
> don't know enough about Windows to say what would be expected there,
> what is the conventional behaviour on that platform?
I expect that it is to put double quotes around paths with spaces in
them. This is how it's done on the command line and in the registry
at least. Using backslash as an escape character would probably be
considered confusing since backslash is the directory separator on
w*ndows. Although it wouldn't actually cause any clashes as long as
only `\ ' is recognized as an escape, and no directories have names
starting with space...
Being able to use double quotes for escaping spaces is probably part
of the expected behaviour on UNIX as well, since system() allows this,
so using them in favour of \ is probably a better match on the whole.
Although personally I'd rather see it use system() so that it
automatically conforms to the OS's conventions, whatever they happen
to be.
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Philip Martin <ph...@codematters.co.uk>.
Daniel Stenberg <da...@haxx.se> writes:
> On Sun, 28 Apr 2002, Zack Weinberg wrote:
>
> > -0: I don't see what's wrong with just calling system() in this case, like
> > everyone else who handles $EDITOR.
>
> Because using system() for this was discussed and turned down when I first
> started messing with $EDITOR support, back in January:
>
> [ said regarding using svn_io_run_cmd() instead of system() ]
> <quote>
> It's not just a "better option" but a requirement :-) system() isn't
> portable, and it isn't exactly the safest function in the world. Granted,
> this is all client stuff, but I'd still not like to see system-based
> exploits in the code.
> </quote>
> http://subversion.tigris.org/servlets/ReadMsg?msgId=55026&listName=dev
>
> Of course, matters and opinions change. This may not be valid anymore...
Well the security problem is a red herring, the whole point of EDITOR
is that we are letting the user specify the program to be run.
Yes, the patch provides some of the expected behaviour on Unix. I
don't know enough about Windows to say what would be expected there,
what is the conventional behaviour on that platform?
The patch doesn't provide all the behaviour I would expect on Unix,
using system() would allow
EDITOR='LD_LIBRARY_PATH=/some/lib /some/program'
Perhaps we need an APR wrapper for system()?
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Daniel Stenberg <da...@haxx.se>.
On Sun, 28 Apr 2002, Zack Weinberg wrote:
> -0: I don't see what's wrong with just calling system() in this case, like
> everyone else who handles $EDITOR.
Because using system() for this was discussed and turned down when I first
started messing with $EDITOR support, back in January:
[ said regarding using svn_io_run_cmd() instead of system() ]
<quote>
It's not just a "better option" but a requirement :-) system() isn't
portable, and it isn't exactly the safest function in the world. Granted,
this is all client stuff, but I'd still not like to see system-based
exploits in the code.
</quote>
http://subversion.tigris.org/servlets/ReadMsg?msgId=55026&listName=dev
Of course, matters and opinions change. This may not be valid anymore...
--
Daniel Stenberg - http://daniel.haxx.se - +46-705-44 31 77
ech`echo xiun|tr nu oc|sed 'sx\([sx]\)\([xoi]\)xo un\2\1 is xg'`ol
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by Zack Weinberg <za...@codesourcery.com>.
On Sun, Apr 28, 2002 at 10:33:37PM +0200, Daniel Stenberg wrote:
> My patch that makes svn support $EDITOR containing spaces wasn't exactly
> turned down but it didn't get much support either.
>
> Let me put the question differently now, does anyone object to my solution?
>
> - Normal spaces are used as separators
> - Escaped spaces ("\ ") are used for embedding spaces in paths/commands
-0: I don't see what's wrong with just calling system() in this case,
like everyone else who handles $EDITOR.
zw
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reminder: $EDITOR with spaces?
Posted by cm...@collab.net.
Daniel Stenberg <da...@haxx.se> writes:
> My patch that makes svn support $EDITOR containing spaces wasn't exactly
> turned down but it didn't get much support either.
>
> Let me put the question differently now, does anyone object to my solution?
>
> - Normal spaces are used as separators
> - Escaped spaces ("\ ") are used for embedding spaces in paths/commands
Sorry, Daniel. I was under the impression that the spirit of your
patch was pretty much accepted -- folks just haven't gotten around to
reviewing it. I have it saved in my inbox for review later if no one
else gets to it, but that hasn't been my foremost priority as of late.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org