On 10/18/06, Malcolm Rowe <malcolm-svn-dev@farside.org.uk> wrote:
> Background for people not at the summit: We discussed the
> regularly-reported problem today about the Subversion client storing
> plaintext auth info on Unix, and the response that elicits from users.
>
> We all agreed that scrambling the password wouldn't add any security at
> all, but we also had a vague consensus that as long as we made sure that
> users didn't think the passwords _were_ encrypted, obfuscating them in
> some way might not be objectionable, since not doing so aggravates some
> people.
+1, see Karl's explanation on this. He explains very plainly why this
is a win in just about any situation. I'd just like to note that we
are not treating this as an excuse to not investigat password manager
for !(win32 || OSX). But until that investigation takes place, this is
a silencer for the recurring complaints the users@ list has had for
years.
> K 7
> comment
> V 252
> WARNING! This file contains a version of your password that has been
> slightly scrambled to avoid accidental disclosure.
>
> The scrambled password is NOT ENCRYPTED in any way, and so you should
> take the same care of it as you would your regular password.
<pedantic>
The warning could maybe do with a little extra formatting. Something
along the lines of:
******************** WARNING ********************
This file contains passwords. They have been slightly scrambled so
that people reading over your shoulder cannot read them.
However, they are NOT ENCRYPTED in any way. Anyone who gets hold of
this file and is serious about it can recover your password in
seconds. Take the necessary care to protect this file from being
stolen.
Just a little more verbose and flashy, to get to the joe end user more.
> Index: subversion/include/svn_auth.h
> ===================================================================
> --- subversion/include/svn_auth.h (revision 21975)
> +++ subversion/include/svn_auth.h (working copy)
> @@ -701,6 +701,26 @@ svn_auth_get_keychain_simple_provider(sv
>
> #endif /* DARWIN || DOXYGEN */
>
> +/**
> + * Create and return @a *provider, an authentication provider of type @c
> + * svn_auth_cred_simple_t that gets/sets information from the user's
> + * ~/.subversion configuration directory. Allocate @a *provider in
> + * @a pool.
> + *
> + * This is like svn_client_get_simple_provider(), except that the
> + * provider will lightly scramble the password before storing it to
> + * disk so that accidental disclosure is harder.
Accidental disclosure becoming "harder" is not the right term imo. "to
avoid accidental disclosure" maybe?
> + *
> + * This provider does @em not encrypt the data in any meaningful way.
> + * To ensure that users are aware of this, it will also add a warning to
> + * the auth file.
> + *
> + * @since New in 1.5.
> + */
> +void
> +svn_auth_get_obfuscated_simple_provider(svn_auth_provider_object_t **provider,
> + apr_pool_t *pool);
> +
> /** Create and return @a *provider, an authentication provider of type @c
> * svn_auth_cred_username_t that gets/sets information from a user's
> * ~/.subversion configuration directory. Allocate @a *provider in
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (revision 21975)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -23,6 +23,7 @@
> /*** Includes. ***/
>
> #include <apr_pools.h>
> +#include <apr_base64.h>
> #include "svn_auth.h"
> #include "svn_error.h"
> #include "svn_utf.h"
> @@ -39,10 +40,12 @@
> #define SVN_AUTH__AUTHFILE_USERNAME_KEY "username"
> #define SVN_AUTH__AUTHFILE_PASSWORD_KEY "password"
> #define SVN_AUTH__AUTHFILE_PASSTYPE_KEY "passtype"
> +#define SVN_AUTH__AUTHFILE_COMMENT_KEY "comment"
>
> #define SVN_AUTH__SIMPLE_PASSWORD_TYPE "simple"
> #define SVN_AUTH__WINCRYPT_PASSWORD_TYPE "wincrypt"
> #define SVN_AUTH__KEYCHAIN_PASSWORD_TYPE "keychain"
> +#define SVN_AUTH__OBFUSCATED_PASSWORD_TYPE "obfuscated"
>
>
> /* A function that stores PASSWORD (or some encrypted version thereof)
> @@ -537,17 +540,12 @@ svn_auth_get_simple_prompt_provider
>
> #ifdef WIN32
> #include <wincrypt.h>
> -#include <apr_base64.h>
>
> /* The description string that's combined with unencrypted data by the
> Windows CryptoAPI. Used during decryption to verify that the
> encrypted data were valid. */
> static const WCHAR description[] = L"auth_svn.simple.wincrypt";
>
> -/* The password type used by the windows simple auth provider, passed
> - into simple_first_creds_helper and simple_save_creds_helper. */
> -static const char windows_crypto_type[] = "wincrypt";
> -
Why is this removed? Are we replacing "wincrypt" with this 'universal'
obfuscator?
> /* Dynamically load the address of function NAME in PDLL into
> PFN. Return TRUE if the function name was found, otherwise
> FALSE. Equivalent to dlsym(). */
> @@ -877,3 +875,117 @@ svn_auth_get_keychain_simple_provider(sv
> }
>
> #endif /* SVN_HAVE_KEYCHAIN_SERVICES */
> +
> +/*-----------------------------------------------------------------------*/
> +/* Obfuscating simple provider, stores obfuscated passwords in the creds */
> +/* file. This does not add any security at all, but does mitigate */
> +/* against accidental disclosure. */
> +/*-----------------------------------------------------------------------*/
"but does help to avoid accidental disclosure."
> +
> +/* Store an obfuscated (base-64) version of PASSWORD in CREDS, together with
> + a note to warn users that the password is not encrypted.
> +
> + This function implements password_set_t. */
> +static svn_boolean_t
> +obfuscated_password_set(apr_hash_t *creds,
> + const char *realmstring,
> + const char *username,
> + const char *password,
> + svn_boolean_t non_interactive,
> + apr_pool_t *pool)
> +{
> + size_t len = strlen(password);
> + char *obfuscated = apr_palloc(pool, apr_base64_encode_len(len));
> + apr_base64_encode(obfuscated, password, len);
> + if (!simple_password_set(creds, realmstring, username, obfuscated,
> + non_interactive, pool))
> + return FALSE;
> +
> + const char * warning =
> + _("WARNING! This file contains a version of your password that has been\n"
> + "slightly scrambled to avoid accidental disclosure.\n\n"
> + "The scrambled password is NOT ENCRYPTED in any way, and so you should\n"
> + "take the same care of it as you would your regular password.");
> + apr_hash_set(creds, SVN_AUTH__AUTHFILE_COMMENT_KEY, APR_HASH_KEY_STRING,
> + svn_string_create(warning, pool));
> +
> + return TRUE;
> +}
> +
> +/* Retrieve an obfuscated password from CREDS.
> + This function implements password_get_t. */
> +static svn_boolean_t
> +obfuscated_password_get(const char **password,
> + apr_hash_t *creds,
> + const char *realmstring,
> + const char *username,
> + svn_boolean_t non_interactive,
> + apr_pool_t *pool)
> +{
> + const char *obfuscated;
> + char *plaintext;
> + size_t len;
> +
> + if (!simple_password_get(&obfuscated, creds, realmstring, username,
> + non_interactive, pool))
> + return FALSE;
> +
> + len = apr_base64_decode_len(obfuscated);
> + plaintext = apr_palloc(pool, len);
> + apr_base64_decode(plaintext, obfuscated);
> + *password = plaintext;
> +
> + return TRUE;
> +}
> +
> +/* Get cached obfuscated credentials from the simple provider's cache. */
Should this read "from the obfuscated provider's cache" ?
> +static svn_error_t *
> +obfuscated_simple_first_creds(void **credentials,
> + void **iter_baton,
> + void *provider_baton,
> + apr_hash_t *parameters,
> + const char *realmstring,
> + apr_pool_t *pool)
> +{
> + return simple_first_creds_helper(credentials,
> + iter_baton, provider_baton,
> + parameters, realmstring,
> + obfuscated_password_get,
> + SVN_AUTH__OBFUSCATED_PASSWORD_TYPE,
> + pool);
> +}
> +
> +/* Save obfuscated credentials to the simple provider's cache. */
> +static svn_error_t *
> +obfuscated_simple_save_creds(svn_boolean_t *saved,
> + void *credentials,
> + void *provider_baton,
> + apr_hash_t *parameters,
> + const char *realmstring,
> + apr_pool_t *pool)
> +{
> + return simple_save_creds_helper(saved, credentials, provider_baton,
> + parameters, realmstring,
> + obfuscated_password_set,
> + SVN_AUTH__OBFUSCATED_PASSWORD_TYPE,
> + pool);
> +}
> +
> +static const svn_auth_provider_t obfuscated_simple_provider = {
> + SVN_AUTH_CRED_SIMPLE,
> + obfuscated_simple_first_creds,
> + NULL,
> + obfuscated_simple_save_creds
> +};
> +
> +
> +/* Public API */
> +void
> +svn_auth_get_obfuscated_simple_provider(svn_auth_provider_object_t **provider,
> + apr_pool_t *pool)
> +{
> + svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
> +
> + po->vtable = &obfuscated_simple_provider;
> + *provider = po;
> +}
> Index: subversion/libsvn_subr/cmdline.c
> ===================================================================
> --- subversion/libsvn_subr/cmdline.c (revision 21975)
> +++ subversion/libsvn_subr/cmdline.c (working copy)
> @@ -362,6 +362,8 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
> svn_auth_get_keychain_simple_provider(&provider, pool);
> APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> #endif
> + svn_auth_get_obfuscated_simple_provider(&provider, pool);
> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> svn_auth_get_simple_provider(&provider, pool);
> APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> svn_auth_get_username_provider(&provider, pool);
Good job. As an aside, I motion that the simple auth provider be
altered to use the obfuscated set_password. That way, the cache is
progressively updated to obfuscated storage when touched. The change
should be a one-or-so-liner.
- Dave
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 19 10:44:27 2006