You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.co.il> on 2008/07/01 06:12:13 UTC
Re: svn commit: r31905 - in branches/1.5.x: . subversion/bindings/swig
subversion/libsvn_subr
[ summarising yesterday's IRC discussion ]
Karl Fogel wrote on Sun, 29 Jun 2008 at 21:18 -0400:
> subversion/libsvn_subr/simple_providers.c:simple_password_get(), which
> is the function affected by r31884, is an implementation of the
> 'svn_auth__password_get_t()' interface:
>
> /* 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.
> 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)
> (const char **password,
> apr_hash_t *creds,
> const char *realmstring,
> const char *username,
> svn_boolean_t non_interactive,
> apr_pool_t *pool);
>
> That doc string is not clear on whether REALMSTRING or USERNAME may be
> null. But r31884 assumes that USERNAME will not be null -- it passes
> the username directly to strcmp().
>
> Is there ever a situation where a null USERNAME and/or REALMSTRING would
> be reasonable? If so, we should fix up the typedef's doc string, and
> then tweak simple_password_get() (and perhaps other implementations of
> that interface) accordingly.
>
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.
Daniel
> Below is r31884, for reference.
>
> -Karl
>
> ------------------------------------------------------------------------
> r31884 | stylesen | 2008-06-26 05:00:37 -0400 (Thu, 26 Jun 2008) | 7 lines
> Changed paths:
> M /trunk/subversion/libsvn_subr/simple_providers.c
>
> Fix issue #2242: auth cache picking up password from wrong username entry.
>
> * subversion/libsvn_subr/simple_providers.c
> (simple_password_get): Validate the username for which we get the password.
>
> Approved by: danielsh
>
> Index: trunk/subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- trunk/subversion/libsvn_subr/simple_providers.c (revision 31883)
> +++ trunk/subversion/libsvn_subr/simple_providers.c (revision 31884)
> @@ -65,12 +65,17 @@
> apr_pool_t *pool)
> {
> svn_string_t *str;
> - str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
> + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> APR_HASH_KEY_STRING);
> - if (str && str->data)
> + if (str && strcmp(str->data, username) == 0)
> {
> - *password = str->data;
> - return TRUE;
> + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
> + APR_HASH_KEY_STRING);
> + if (str && str->data)
> + {
> + *password = str->data;
> + return TRUE;
> + }
> }
> return FALSE;
> }
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31905 - in branches/1.5.x: . subversion/bindings/swig
subversion/libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Committed both patches in r32023. (They pass all tests, except a few
mergeinfo tests that fail on buildbot too.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31905 - in branches/1.5.x: . subversion/bindings/swig
subversion/libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Wed, 2 Jul 2008 at 17:09 -0400:
> Daniel Shahaf <d....@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31905 - in branches/1.5.x: . subversion/bindings/swig subversion/libsvn_subr
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@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.)
[[[
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)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org