On 09.02.2015 15:56, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Evgeny Kotkov [mailto:evgeny.kotkov_at_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 <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.
> +1
+1; I'm tired of micro-optimizations without proper proof of performance
comparisons that also break code.
-- Brane
Received on 2015-02-09 16:08:20 CET