Re: [Monotone-devel] popen from a hook

[Nathaniel Smith]

On Sun, Nov 19, 2006 at 01:53:08PM +1100, William Uther wrote:
> Hi,
>   After a bit of digging I discovered that io.popen has been  
> disabled in mtn's lua because it is considered a security risk (if  
> you pass a file name into it, then it might do weird things in the  
> shell).  I find this ironic, because the workaround for lack of a  
> popen in my use-case is to save a password to a file in cleartext...  
> also a security a risk.
> 
>   So, I've made two patches.  The first does two things:
>    - adds some error messages to make it clear that popen was  
> deliberately disabled.
>    - leaves io.unsafepopen as a replacement for popen.  This makes  
> it clear that it is considered a security risk in some situations,  
> but allows its use where there is no security problem (e.g. if you're  
> not passing versioned filenames on the command line).

This seems reasonable enough.  A better solution, if it's not too
hard, would be to just provide an actually safe version of popen --
one that takes a command line vector, rather than string.

>   The second version leaves 'popen' available unrenamed, but wraps  
> it in a fairly strict check to make sure it isn't a security hole (no  
> characters except [a-zA-Z0-9 ] are allowed in the command).

I don't like this one too much.  Partly because I'm still not sure it
goes far enough -- spaces are possibly the most benign shell
metacharacter, but they are still a shell metacharacter, and allowing
them may be dangerous.  Partly because even if this is safe, it makes
it easy to introduce latent bugs -- J. Random User writes a hook, uses
popen without realizing the danger, it works fine in his tests, but
then in real life his hook starts failing.  Better to fail than to
open a security hole, but still not really ideal.

-- Nathaniel

-- 
The Universe may  /  Be as large as they say
But it wouldn't be missed  /  If it didn't exist.
  -- Piet Hein