Re: [Monotone-devel] popen replacement

[Nathaniel Smith]

On Sun, Feb 18, 2007 at 08:44:04PM +1100, William Uther wrote:
>   A while ago I came across the fact that popen was disabled in the  
> Lua hooks in monotone for security reasons.  Here is a patch that  
> replaces it with a security conscious version (like spawn() replaces  
> execute()).  The 'security consciousness' is simply accepting the  
> command as an array of arguments rather than a single string to be  
> parsed by the shell.

Sounds security conscious to me.

>   Returning a FILE* from C functions in Lua is tricky (there is a  
> Lua FAQ on just this).  This wasn't working for a while, then it was,  
> and I don't understand what changed.  If someone wants to look at  
> that, I wouldn't mind.  It would be good if people could test it on a  
> bunch of different systems too...
> 
>   I've only implemented this on Unix.  I have no windows knowledge  
> or ability to test.
> 
>   I'm assuming that attaching a patch to an email to the list is the  
> correct way to submit this.  Let me know if there is something else I  
> should do.

Nope, that works.

The code makes me a little uneasy -- lots of pointer arithmetic/malloc
usage.  Not clear that's fixable, though.

The formatting could be better -- we tend not to skimp on the space
bar too much :-).  "(char**)malloc((n+1)*sizeof(char*))" is not
particularly easy to read, for instance.  (Also, old-style casts?)

I can think of two cleaner solutions:
  -- we already have netxx_pipe.cc to provide a cross-platform way to
     spawn and talk to a child process.  Trying to wire up netxx to
     lua filehandles directly sounds totally un-fun, but in fact I
     don't think anyone wants to implement chat(1) in lua -- for your
     use case, at least, one could just have a lua function where you
     hand it the stdin you made up as a string, and it hands you the
     stdout you want as a string.  (In fact, the testsuite has code to
     do something very like this, and cross-platform, doesn't it?)
  -- simply implement OS X keychain integration directly :-).  It
     looks like this would be _really_ easy -- you need one command to
     store your passphrase in the keychain, which is just implemented
     as
       #include <Security/Security.h>
       utf8 service("monotone");
       SecKeychainAddGenericPassword(NULL,
                                     service().size(), service().c_str(),
                                     keyid().size(), keyid().c_str(),
                                     pass().size(), pass().c_str(),
                                     NULL);
     and when we need a passphrase, we just do
       u32 passlen;
       char * passdata;
       SecKeychainFindGenericPassword(NULL,
                                      service().size(), service().c_str(),
                                      keyid().size(), keyid().c_str(),
                                      &passlen, &passdata,
                                      NULL);
       utf8 passphrase(std::string(passdata, passlen));
       SecKeychainItemFreeContent(NULL, passdata);

     These APIs are all available in straight C, and you really only
     need those function calls:
       
http://developer.apple.com/documentation/Security/Conceptual/keychainServConcepts/index.html
     Just need some autoconf to check if the keychain api is
     available.

     (It'd be even nicer if we could use the OS X keychain's signing
     support, but it's all X.509-alicious, so likely not trivial to
     subvert to our needs.)

OTOH, this is a small patch, so maybe it doesn't matter that much if
there are other cleaner ways to do things, we could get to them later.
Does anyone else have an opinion?

> BTW, I'm using this on MacOS X to store my password securely in the  
> system keychain.  In particular, I have this in my monotonerc:
> 
> function get_passphrase(keypair_id)
>       procfin, procfout, pid = spawn_pipe("getPassword", "monotoneKey")
>       procfin:close()
>       pass, errstr = procfout:read()
>       procfout:close()
>       if (pid ~= -1) then ret, pid = wait(pid) end
>       return pass
> end
> 
> Where getPassword is the following shell script:
> 
> /usr/bin/security find-generic-password -ga $1 2>&1 > /dev/null | /sw/ 
> bin/sed -r -e 's/password: \"(.*)\"/\1/'
> 
> You set the password by launching "Keychain Access" from the  
> Utilities folder and click on the little + at the bottom of the  
> window.  The Account Name is "monotoneKey", and the password is the  
> password. :)

Good timing, we've just been debating whether get_passphrase is
actually useful for anything once you have the possibility to store
unencrypted keys :-).  (Since the major current use case for
get_passphrase is server keys.)

-- Nathaniel

-- 
The best book on programming is still Strunk and White.