Hook scripts start with an empty environment

[Tim Starling <ts...@wikimedia.org>]

Hook scripts start with an empty environment instead of inheriting it
from svnserve or whatever.

This is inconvenient, not least for the case where you want to commit
something to an svn+ssh server via a local pushmi mirror on file:/// and
your SSH_AUTH_SOCK is lost so you have to type your passphrase all the time.

There's no comment in the code explaining why the environment has to be
empty, so I assume it was just done like that on a whim. Trivial patch
attached.

-- Tim Starling

Re: Hook scripts start with an empty environment

[Greg Hudson <gh...@MIT.EDU>]

On Wed, 2010-03-24 at 16:45 -0400, Tim Starling wrote:
> But like I said, I'm happy with it being configurable. Do you want a
> patch for that too? It's a fair bit more complicated than the one I
> already did so I didn't want to try it without at least in-principle
> approval.

I haven't been active in Subversion development for some time, so you
may want to wait for an in-principle approval from someone more likely
to be able to conveniently integrate the patch.

Re: Hook scripts start with an empty environment

[Tim Starling <ts...@wikimedia.org>]

Greg Hudson wrote:
> It might be reasonable to have said from the start, "if you're in the
> third situation, then your hook scripts should clear their own
> environments," but we can't start saying that in release 1.7.  We can
> detect a setuid or setgid bit, but we cannot detect a restricted shell
> situation (such as when .ssh/authorized-keys contains a "command"
> directive), so we can't really intuit when it's safe to propagate the
> environment.
>   

If the .ssh/authorized_keys has a command directive, the only way the
user could set environment variables in OpenSSH is if the server has a
set of potentially malicious variable names in the AcceptEnv
configuration variable. It accepts no variables by default and the
manual warns "that some environment variables could be used to bypass
restricted user environments".

But like I said, I'm happy with it being configurable. Do you want a
patch for that too? It's a fair bit more complicated than the one I
already did so I didn't want to try it without at least in-principle
approval.

-- Tim Starling

Re: Hook scripts start with an empty environment

[Greg Hudson <gh...@MIT.EDU>]

Although I've always been aware of the design intent behind empty hook
script environments, I'll echo Tim's complaint that it's sometimes
inconvenient.  The problem most commonly crops up in svn+ssh:// or
file:// deployments where you want to run some action with user
credentials: updating a bug database, updating a shared read-only
working copy, updating a snapshot of the repository, etc..  If the
credentials are pointed to by environment variables (e.g. Kerberos
tickets), then the operation fails.  If they are pointed to by other
means (e.g. AFS tokens), then the operation may succeed anyway.

In some cases it may be possible to perform the action with server host
credentials instead, but that is not always a good option.

The design intent behind clearing the environment has two bases:
security and consistency.  In evaluating the security basis, you have to
consider the type of deployment:

  * http:// and svn:// access, where the client has little or no control
over the environment.  In this case propagating the environment carries
no risk because the environment is controlled by the admin.

  * file:// and svn+ssh:// access where the calling user has
unrestricted shell priveleges and there is no setuid or setgid bit on
the svn/svnserve binary.  In this case propagating the environment
carries no risk because the svn code is not executing with elevated
privilege.

  * file:// and svn+ssh:// access where the calling user has restricted
shell access (can only run svn/svnserve for some reason) or there is a
setuid or setgid bit on the svn or svnserve binary.  In this case
propagating the environment could open the door to unintended access.

It might be reasonable to have said from the start, "if you're in the
third situation, then your hook scripts should clear their own
environments," but we can't start saying that in release 1.7.  We can
detect a setuid or setgid bit, but we cannot detect a restricted shell
situation (such as when .ssh/authorized-keys contains a "command"
directive), so we can't really intuit when it's safe to propagate the
environment.

It would be reasonable to make this controlled by repository
configuration, if there's a convenient place to put that bit.

Re: Hook scripts start with an empty environment

[Tim Starling <ts...@wikimedia.org>]

Bert Huijben wrote:
>   
>> -----Original Message-----
>> From: Tim Starling [mailto:tstarling@wikimedia.org]
>> Sent: woensdag 24 maart 2010 7:19
>> To: dev@subversion.apache.org
>> Subject: Hook scripts start with an empty environment
>>
>> Hook scripts start with an empty environment instead of inheriting it
>> from svnserve or whatever.
>>
>> This is inconvenient, not least for the case where you want to commit
>> something to an svn+ssh server via a local pushmi mirror on file:/// and
>> your SSH_AUTH_SOCK is lost so you have to type your passphrase all the
>> time.
>>
>> There's no comment in the code explaining why the environment has to be
>> empty, so I assume it was just done like that on a whim. Trivial patch
>> attached.
>>     
>
> 	Sorry,
>
> This behavior is by design. 
>
> Repository hooks run as the 'repository owner' and clearing the environment is part of the security around that feature.
>   

You mean if the repository is accessed via the svn or http transports?
That may be true, but the file and svn+ssh transports don't run as any
special user.  It would be simple enough to clear the environment only
when the svn or http transports are used. Or the behaviour could be
configurable.

> I'm a bit surprised that you actually see a passphrase prompt from a hook, as the hook environment redirects stdin, stdout and stderr to the server process. The only prompt you should be able to see is the prompt for starting the ssh process. 

I believe ssh opens the controlling terminal directly. But that's beside
the point.

> (And this ssh isn't called via the function you tried to patch)
>   

The hook script calls ssh. The function I patched calls the hook script.
If you pass the full environment to the hook script then the hook script
passes it to ssh. I have tested this.

> If we would forward the environment hook scripts, the scripts might accidentally use environment variables from the calling process without the user knowing. Which opens a backdoor for all kinds of malware/abusal. And it would also make it very hard to create hook scripts that work identical for all repository users.
>   

What sort of backdoor malware abusal exactly? Whatever this problem is,
it obviously isn't a problem for apache, which has a similar unix user
model and nevertheless passes the full environment down to CGI scripts
that it runs.

Anyway, I'm not really too concerned. It can be configurable or
transport-dependent or an autoconf option, or I can just tell everyone I
know to patch and compile their own subversion client.

-- Tim Starling

RE: Hook scripts start with an empty environment

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: Tim Starling [mailto:tstarling@wikimedia.org]
> Sent: woensdag 24 maart 2010 7:19
> To: dev@subversion.apache.org
> Subject: Hook scripts start with an empty environment
> 
> Hook scripts start with an empty environment instead of inheriting it
> from svnserve or whatever.
> 
> This is inconvenient, not least for the case where you want to commit
> something to an svn+ssh server via a local pushmi mirror on file:/// and
> your SSH_AUTH_SOCK is lost so you have to type your passphrase all the
> time.
> 
> There's no comment in the code explaining why the environment has to be
> empty, so I assume it was just done like that on a whim. Trivial patch
> attached.

	Sorry,

This behavior is by design. 

Repository hooks run as the 'repository owner' and clearing the environment is part of the security around that feature.
http://svnbook.red-bean.com/en/1.5/svn.reposadmin.create.html#svn.reposadmin.create.hooks (or http://tinyurl.com/59yzll )

I'm a bit surprised that you actually see a passphrase prompt from a hook, as the hook environment redirects stdin, stdout and stderr to the server process. The only prompt you should be able to see is the prompt for starting the ssh process. (And this ssh isn't called via the function you tried to patch)

If we would forward the environment hook scripts, the scripts might accidentally use environment variables from the calling process without the user knowing. Which opens a backdoor for all kinds of malware/abusal. And it would also make it very hard to create hook scripts that work identical for all repository users.

	Bert

Re: Hook scripts start with an empty environment

[Alan Barrett <ap...@cequrux.com>]

On Wed, 24 Mar 2010, Tim Starling wrote:
> Hook scripts start with an empty environment instead of inheriting it
> from svnserve or whatever.
> 
> This is inconvenient, not least for the case where you want to commit
> something to an svn+ssh server via a local pushmi mirror on file:/// and
> your SSH_AUTH_SOCK is lost so you have to type your passphrase all the time.

As a workaround for this particular issue, you could do the following
without modifying svn at all:

    * Run svn through a wrapper script that saves the value of
      SSH_AUTH_SOCK to a file whose name is derived from the pid
      (e.g. echo $SSH_AUTH_SOCK >/tmp/ssh-auth-sock.$$).

    * In the hook script, walk up the chain of parent processes (e.g.
      by parsing the output from "ps") until it finds the pid of the
      wrapper script; then read the file that the wrapper script saved.

On the other hand, I think it would be reasonable to have a configurable
way of allowing specified environment variables to be passed to hook
scripts.

--apb (Alan Barrett)