Hi Hyrum,
On Wed, Oct 27, 2010 at 11:34 PM, Hyrum K. Wright
<hyrum_wright_at_mail.utexas.edu> wrote:
> On Wed, Oct 27, 2010 at 3:40 PM, <stefan2_at_apache.org> wrote:
>> Author: stefan2
>> Date: Wed Oct 27 20:40:53 2010
>> New Revision: 1028092
>>
>> URL: http://svn.apache.org/viewvc?rev=1028092&view=rev
>> Log:
>> Incorporate feedback I got on r985606.
>>
>> * subversion/libsvn_ra_svn/marshal.c
>> (SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD): introduce symbolic name
>> for an otherwise arbitrary number
>> (read_long_string): fix docstring
>> (read_string): use symbolic name and explain the rationale behind the special case
>>
>> Modified:
>> subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
>>
>> Modified: subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
>> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c?rev=1028092&r1=1028091&r2=1028092&view=diff
>> ==============================================================================
>> --- subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c (original)
>> +++ subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c Wed Oct 27 20:40:53 2010
>> @@ -44,6 +44,12 @@
>>
>> #define svn_iswhitespace(c) ((c) == ' ' || (c) == '\n')
>>
>> +/* If we receive data that *claims* to be followed by a very long string,
>> + * we should not trust that claim right away. But everything up to 1 MB
>> + * should be too small to be instrumental for a DOS attack. */
>> +
>> +#define SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD (0x100000)
>
> I like the name!
>
>> +
>> /* --- CONNECTION INITIALIZATION --- */
>>
>> svn_ra_svn_conn_t *svn_ra_svn_create_conn2(apr_socket_t *sock,
>> @@ -555,9 +561,8 @@ svn_error_t *svn_ra_svn_write_tuple(svn_
>>
>> /* --- READING DATA ITEMS --- */
>>
>> -/* Read LEN bytes from CONN into already-allocated structure ITEM.
>> - * Afterwards, *ITEM is of type 'SVN_RA_SVN_STRING', and its string
>> - * data is allocated in POOL. */
>> +/* Read LEN bytes from CONN into a supposedly empty STRINGBUF.
>> + * POOL will be used for temporary allocations. */
>> static svn_error_t *
>> read_long_string(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
>> svn_stringbuf_t *stringbuf, apr_uint64_t len)
>> @@ -593,7 +598,14 @@ static svn_error_t *read_string(svn_ra_s
>> svn_ra_svn_item_t *item, apr_uint64_t len)
>> {
>> svn_stringbuf_t *stringbuf;
>> - if (len > 0x100000)
>> +
>> + /* We should not use large strings in our protocol. However, we may
>> + * receive a claim that a very long string is going to follow. In that
>> + * case, we start small and wait for all that data to actually show up.
>> + * This does not fully prevent DOS attacs but makes them harder (you
>> + * have to actually send gigabytes of data).
>
> Wow, I hadn't even considered this. Once we get this on trunk, it
> might make sense to propose a backport, since this has (potential?)
> security implications.
Actually, that was already released as a security vulnerability some
years ago. The comment by Stefan makes it painfully apparent that it
is, but I guess that's a good thing. Notice that he did nothing but
name the constant and add the explanation. See
http://subversion.apache.org/security/CAN-2004-0413-advisory.txt
This is exactly the point I was talking about when I said properties
are length-limited by ra_svn. (In relation to the maximum size of
merge-tracking information.) The actual code is a little bit
different than what I remembered., because it does seem to grow the
buffer once it gets past the first MiB.
Regards,
Erik.
Received on 2010-10-28 09:31:25 CEST