[PATCH] don't store plain-text passwords by default (was: Re: subversion reveals passwords)

On Sun, Apr 06, 2008 at 11:32:03PM +0200, Stefan Sperling wrote:
> On Sun, Apr 06, 2008 at 04:55:36PM -0400, Karl Fogel wrote:
> > "Erik Huelsmann" <ehuels_at_gmail.com> writes:
> > > Well, there's a big chance of me being perceivede as rude after my
> > > next statement, but this has been discussed *many* times before.
> > >
> > > The choice to store passwords in plain text has been a very conscious
> > > decision; it has also been replaced by more appropriate storage
> > > mechanisms on platforms which support that (Keychain on OSX,
> > > Crypto-API on Windows). Unfortunately, Linux doesn't feature a
> > > *standardized* crypto-agent. We don't need people lecturing us what's
> > > secure and what's not: we need people implementing secure storage
> > > mechanisms or patches to Subversion to support these mechanisms.
> >
> > Basically agree with your sentiment, but:
> >
> > We could switch to a default of not storing plaintext passwords, if we
> >
> > 1) Had a run-time option ('--store-password' or something) that
> > causes it to be stored permanently for that working copy, and
> >
> > 2) Had a config option to turn on password-storing as a default
>
> +1
>
> I'd rather post a patch than simply agree though.

Here is the patch I promised.

As I already said, I think that storing plain text passwords
wasn't a good idea in the first place. As everyone knows it
generates quite a lot of complaints on our lists for one thing
(I doubt the people who lose a bit of convenience will be as vocal).
And especially it's a real threat in single-sign-on environments,
which are mostly used at large sites (companies, universities).
Many people in these environments do not like or cannot tolerate
the current behaviour, for example, see:
http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=131022
(This does not only affect Collab.net's customers, of course.
Anyone trying to introduce Subversion into such an environment will
lose an argument about the "clear-text-password-caching" point.)

I recommend to read that whole thread if you plan to comment
here further. The thread starts at:
http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=131005
There are many good points in there. One of them is that an effort
should be made to come up with a way of encrypting passwords on *nix.
Note that Lieven has started working on GNOME Keyring integration,
so this is also being addressed, and does not conflict with this
patch in any way.

I would like to commit this patch, but first I'd like to get:

1) Feedback from people using WinCrypt (i.e. running svn on Windows 2k
   and upwards) to make sure password storing still works by default
   on that platform.

2) Feedback from people using Keychain (i.e. running svn on Mac OS X)
   to make sure password storing still works by default on that platform.

3) Feedback from anyone who wants the current behaviour to be
   retained at all costs. :)

I've tested the patch with svnserve locally, on FreeBSD.
It works as I expected it to work. The password isn't stored by default.
It is however stored when I pass --store-plaintext-pw (this option only
needs to be used once per server/realm), or when I set the
store-plaintext-passwords option in ~/.subversion/config (which only
needs to be done once per user account).

If anyone objects with anything this patch is doing please let me know.

I've put a few people in Cc: who have recently (up to half a year ago)
complained about this issue on our lists. Please provide feedback if
you can. If you can, please test the patch with current trunk and
see if it works as you expected. That would be really great. Thanks.

Log message (patch is attached):

[[[

Default to not storing plain text passwords on disk.

Users now have to explicitely tell Subversion to save plain text
passwords, either via a configuration file setting, or by passing
a new command line option.

* subversion/include/svn_config.h
  (SVN_CONFIG_OPTION_STORE_PLAINTEXT_PASSWORDS): New constant.

* subversion/include/svn_cmdline.h
  (svn_cmdline_setup_auth_baton2): New revision of
   svn_cmdline_setup_auth_baton.

* subversion/include/svn_auth.h
  (SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSWORDS): New constant.

* subversion/libsvn_subr/config_file.c
  (svn_config_ensure): Document the new store-plaintext-passwords
   in the example configuration file.

* subversion/libsvn_subr/cmdline.c
  (svn_cmdline_setup_auth_baton2): New revision of
   svn_cmdline_setup_auth_baton. Takes a new parameter and reads a
   new configuration file option, both indicating whether storing
   plain-text passwords is allowed. This information is fed into
   the returned auth baton.
  (svn_cmdline_setup_auth_baton): Now calls svn_cmdline_setup_auth_baton2
   with store_plaintext_passwords set to TRUE. This means the old
   behaviour of storing plain-text passwords by default is retained
   for clients using the old API.

* subversion/libsvn_subr/simple_providers.c
  (simple_save_creds_helper): Only store passwords by default for
   providers encrypting passwords. Currently this means that only
   the wincrypt and keychain providers store passwords by default.
   I opted for listing the providers allowed to store passwords by
   default expliclitely, rather than simply disallowing password storing
   for the "simple" provider. This approach is safer, because if new
   providers are added in the future that store passwords in plain-text,
   we don't want them to slip through the cracks accidentally.

* subversion/svn/cl.h
  (svn_cl__opt_state_t): Add new member store_plaintext_passwords.

* subversion/svn/main.c
  (svn_cl__longopt_t): Add opt_store_plaintext_passwords.
  (svn_cl__options): Add "--store-plaintext-pw" option. I wanted to
   call this --store-plaintext-passwords but this was too wide for
   the help output :(
  (svn_cl__global_options): Add opt_store_plaintext_passwords.
  (main): Handle new command line option, and pass it on to
   svn_cmdline_setup_auth_baton2.

]]]

-- 
Stefan Sperling <stsp_at_elego.de>                 Software Developer
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                 Geschaeftsfuehrer: Olaf Wagner