Hi Hyrum, On Wed, Oct 27, 2010 at 11:34 PM, Hyrum K. Wright <[email protected]> wrote: > On Wed, Oct 27, 2010 at 3:40 PM, <[email protected]> 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.
