Karl Fogel wrote on Wed, 2 Jul 2008 at 17:09 -0400:
> Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> > I don't think realm can be NULL because of the way it is used in
> > svn_auth__simple_first_creds_helper(): it passes it to
> > svn_config_read_auth_data() which eventually passes it to strlen().
> >
> > I managed to get password_get() called (by svn_auth__simple_first_creds_helper)
> > with username=NULL by manually removing the "username" key from the dumped
> > hash file in ~/.subversion/auth/ -- that way, cached creds for the realm
> > existed (so creds_hash was non-NULL), but
> >
> > str = apr_hash_get(creds_hash,
> > SVN_AUTH__AUTHFILE_USERNAME_KEY,
> > APR_HASH_KEY_STRING);
> >
> > returned NULL. I don't know if it's possible to have username=NULL
> > without manually editing the auth files.
>
> Well, we certainly shouldn't assume valid input from disk files, in any
> case. How does the following patch look to you? (I'd much rather
> define for certain whether realmstring and username can be NULL or not,
> and modify every authn function accordingly, but that's a much bigger
> change. I didn't want the perfect to be the enemy of the good.)
>
keychain_password_get currently assumes that both REALMSTRING and
USERNAME are non-NULL (it passes both of them to strlen).
> [[[
> Follow up to r31884 with a null check.
>
> * subversion/libsvn_subr/simple_providers.c
> (simple_password_get): If username is NULL -- which can happen if
> someone manually edits the authn cache -- then return FALSE.
>
> * subversion/include/private/svn_auth_private.h
> (svn_auth__password_get_t): Document that realmstring and username
> really ought not be NULL.
> ]]]
>
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (revision 31978)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -67,7 +67,7 @@
> svn_string_t *str;
> str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> APR_HASH_KEY_STRING);
> - if (str && strcmp(str->data, username) == 0)
> + if (str && username && strcmp(str->data, username) == 0)
> {
> str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
> APR_HASH_KEY_STRING);
> Index: subversion/include/private/svn_auth_private.h
> ===================================================================
> --- subversion/include/private/svn_auth_private.h (revision 31978)
> +++ subversion/include/private/svn_auth_private.h (working copy)
> @@ -39,6 +39,7 @@
> /* A function that stores in *PASSWORD (potentially after decrypting it)
> the user's password. It might be obtained directly from CREDS, or
> from an external store, using REALMSTRING and USERNAME as keys.
> + (The behavior is undefined if REALMSTRING or USERNAME are NULL.)
> If NON_INTERACTIVE is set, the user must not be involved in the
> retrieval process. POOL is used for any necessary allocation. */
> typedef svn_boolean_t (*svn_auth__password_get_t)
>
+1.
This change should cause us not to read passwords that don't have an
associated username. So, how about this (in addition to -- not instead
of -- your patch)? ---
[[[
* subversion/libsvn_subr/simple_providers.c
(svn_auth__simple_first_creds_helper):
Don't even try to get a password when we don't have a username.
]]]
Index: subversion/libsvn_subr/simple_providers.c
===================================================================
--- subversion/libsvn_subr/simple_providers.c (revision 31912)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -152,7 +152,7 @@ svn_auth__simple_first_creds_helper(void **credent
username = str->data;
}
- if (! password)
+ if (username && ! password)
{
svn_boolean_t have_passtype;
/* The password type in the auth data must match the
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-07-03 06:14:29 CEST