On Mon, Feb 9, 2015 at 3:05 PM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
wrote:
> Stefan Fuhrmann <stefan2_at_apache.org> writes:
>
> > Within libsvn_repos get_log functionality, pass the list of wanted
> revprops
> > around as an array of svn_string_t* instead of const char*. The added
> length
> > info allows for more effient functions to be used. Do that.
>
> [...]
>
> > - char *name = APR_ARRAY_IDX(revprops, i, char *);
> > - svn_string_t *value = svn_hash_gets(r_props, name);
> > - if (censor_revprops
> > - && !(strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0
> > - || strcmp(name, SVN_PROP_REVISION_DATE) == 0))
> > - /* ... but we can only return author/date. */
> > - continue;
>
> [...]
>
> > + const svn_string_t *name
> > + = APR_ARRAY_IDX(revprops, i, const svn_string_t *);
> > + svn_string_t *value
> > + = apr_hash_get(r_props, name->data, name->len);
> > + if (censor_revprops
> > + && !(strncmp(name->data, SVN_PROP_REVISION_AUTHOR,
> > + name->len) == 0
> > + || strncmp(name->data, SVN_PROP_REVISION_DATE,
> > + name->len) == 0))
> > + /* ... but we can only return author/date. */
> > + continue;
>
> As it turns out, this particular micro-optimization makes a data leak
> possible.
> This is not a real security issue, as the change happened on trunk and
> didn't
> become part of any released version. Still, I think that we should fix
> this
> prior to making 1.9 public.
>
Good catch, Evgeny! Fixed in r1658439.
> I don't know what are the performance implications of using strncmp() in
> favor
> of strcmp(),
No that is a simple thinko. The actual optimization
happened in the block before the fallback code block
that introduced the error. With the fix, we now use
proper and fast string comparison even in that code
block.
> but the new check will not censor properties like 's', 'sv', ...
> 'svn:a', 'svn:d' and others. This means that we might incorrectly leak
> these
> revision properties for partially visible revisions. Subversion 1.8.x only
> outputs svn:date / svn:author when perfoming log requests for partially
> visible
> revisions, and *all* other revision properties are censored out, but with
> this
> changeset this is no longer true.
>
> I committed a failing test in r1658406. As for fixing this issue, I think
> that
> we should entirely revert this changeset.
>
Authz performance has always been a sore spot.
I think making hurt less is worth some code.
-- Stefan^2.
Received on 2015-02-09 16:36:02 CET