[Something that wasn't at all clear from this log message is that this patch
actually fixes CVE-2007-2448. Might have been clearer with two separate
patches :-)
Actually, since I'm having a little trouble working out exactly what
the security problem is here, could you add a description to
www/security/ ? We don't necessarily need to publish it far and wide if
we don't think it's a significant problem, but I think we should at
least have a description we can point people to.]
On Mon, May 21, 2007 at 05:44:48PM -0700, cmpilato@tigris.org wrote:
> --- trunk/subversion/include/svn_repos.h (original)
> +++ trunk/subversion/include/svn_repos.h Mon May 21 17:44:48 2007
> @@ -74,6 +74,19 @@
> apr_pool_t *pool);
>
>
> +/** An enum defining levels of revision access.
> + *
> + * @since New in 1.5.
> + */
> +typedef enum
> +{
> + svn_repos_rev_readable = 1,
Any particular reason that needs to be =1?
> + svn_repos_rev_partially_readable,
> + svn_repos_rev_unreadable
Might be useful to have some documentation about what these value
indicate, somewhere.
> +/**
> + * Set @a access to the access level granted for @a revision in @a
> + * repos, as determined by consulting the @a authz_read_func callback
> + * function and its associated @a authz_read_baton.
It looks like (from usage) authz_read_func can be NULL. We should
mention that in the doc comments.
Perhaps this is a good place to explain what readable and
partially_readable mean?
> + *
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +svn_repos_check_revision_access(svn_repos_revision_access_level_t *access,
> Modified: trunk/subversion/libsvn_repos/fs-wrap.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_repos/fs-wrap.c?pathrev=25095&r1=25094&r2=25095
> ==============================================================================
> --- trunk/subversion/libsvn_repos/fs-wrap.c (original)
> +++ trunk/subversion/libsvn_repos/fs-wrap.c Mon May 21 17:44:48 2007
> @@ -299,13 +225,14 @@
> apr_pool_t *pool)
> {
> svn_string_t *old_value;
> - int readability = rev_readable;
> + svn_repos_revision_access_level_t readability = svn_repos_rev_readable;
There's no need to initialise this any more - we're unconditionally
setting it via the svn_repos_check_revision_access() function now.
(I was originally going to ask if there was some aspect of that function
that I hadn't understood, but I see you just converted one initialiser
to another).
(Also occurs twice more in this file).
> char action;
>
> - if (authz_read_func)
> - SVN_ERR(get_readability(&readability, repos->fs, rev,
> - authz_read_func, authz_read_baton, pool));
> - if (readability == rev_readable)
> + SVN_ERR(svn_repos_check_revision_access(&readability, repos, rev,
> + authz_read_func, authz_read_baton,
> + pool));
> +
> + if (readability == svn_repos_rev_readable)
> ==============================================================================
> --- trunk/subversion/libsvn_repos/log.c (original)
> +++ trunk/subversion/libsvn_repos/log.c Mon May 21 17:44:48 2007
> @@ -32,6 +32,115 @@
>
>
>
> +svn_error_t *
> +svn_repos_check_revision_access(svn_repos_revision_access_level_t *access,
> + svn_repos_t *repos,
> + svn_revnum_t revision,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + apr_pool_t *pool)
> +{
> + svn_fs_t *fs = svn_repos_fs(repos);
> + svn_fs_root_t *rev_root;
> + apr_hash_t *changes;
> + apr_hash_index_t *hi;
> + svn_boolean_t found_readable = FALSE;
> + svn_boolean_t found_unreadable = FALSE;
> + apr_pool_t *subpool;
> +
> + /* By default, we'll grant full read access to REVISION. */
> + *access = svn_repos_rev_readable;
> +
> + /* No auth-checking function? We're done. */
> + if (! authz_read_func)
> + return SVN_NO_ERROR;
> +
> + /* Fetch the changes associated with REVISION. */
> + SVN_ERR(svn_fs_revision_root(&rev_root, fs, revision, pool));
> + SVN_ERR(svn_fs_paths_changed(&changes, rev_root, pool));
> +
> + /* No changed paths? We're done. */
> + if (apr_hash_count(changes) == 0)
> + return SVN_NO_ERROR;
> +
> + /* Otherwise, we have to check the readability of each changed
> + path, or at least enough to answer the question asked. */
> + subpool = svn_pool_create(pool);
> + for (hi = apr_hash_first(pool, changes); hi; hi = apr_hash_next(hi))
No need to create a new iterator in its own pool, surely? You can pass
NULL and use the hash's inbuilt iterator.
Regards,
Malcolm
- application/pgp-signature attachment: stored
Received on Tue May 22 09:46:49 2007