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.
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.
Thoughts?
Regards,
Evgeny Kotkov
Received on 2015-02-09 15:07:24 CET