[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r25095 - in trunk/subversion: include libsvn_repos tests/cmdline

From: Malcolm Rowe <malcolm-svn-dev_at_farside.org.uk>
Date: 2007-05-22 09:46:33 CEST

[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

This is an archived mail posted to the Subversion Dev mailing list.