[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Reminder: $EDITOR with spaces?

From: Mark Benedetto King <bking_at_answerfriend.com>
Date: 2002-04-29 18:42:22 CEST

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
Received on Mon Apr 29 18:46:07 2002

This is an archived mail posted to the Subversion Dev mailing list.