> -----Original Message-----
> From: Evgeny Kotkov [mailto:evgeny.kot...@visualsvn.com]
> Sent: maandag 9 februari 2015 15:06
> To: Subversion Development
> Subject: Re: svn commit: r1483570 -
> /subversion/trunk/subversion/libsvn_repos/log.c
>
> Stefan Fuhrmann <stef...@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.
>
> I don't know what are the performance implications of using strncmp() in favor
> of strcmp(), 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.
+1
Bert
