Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> hwright_at_tigris.org wrote on Fri, 27 Jun 2008 at 10:10 -0700:
>> Author: hwright
>> Date: Fri Jun 27 10:10:54 2008
>> New Revision: 31905
>>
>> Log:
>> Merge r31884 from trunk:
>>
>> * r31884
>> Fix issue #2242: auth cache picking up password from wrong username entry.
>> Votes:
>> +1: arfrever, danielsh, stylesen
>
> Technically, we still need a third +1 on this (since stylesen isn't
> a full committer). Any volunteers?
I've added my +1, because I think the code is more correct after r31884
than before, but I still have a concern about this change.
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.
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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-30 03:20:14 CEST