On Tue, Jan 27, 2015 at 5:07 AM, Hyrum K Wright <hy...@hyrumwright.org>
wrote:
> There's a stack overflow bug in subversion/libsubr/checksum.c.
>
Well, a segfault.
>
> The functions svn_checksum__from_digest_fnv1a_32x4() and
> svn_checksum__from_digest_fnv1a_32() both look something like this:
>
> svn_checksum_t *
> svn_checksum__from_digest_fnv1a_32x4(const unsigned char *digest,
> apr_pool_t *result_pool)
> {
> return checksum_create(svn_checksum_fnv1a_32x4, sizeof(digest), digest,
> result_pool);
> }
>
> The problem is that checksum_create() expects the length of the string
> pointed to by digest, but sizeof(digest) returns the number of bytes that a
> variable of type 'const unsigned char *' requires. These methods are
> currently only called with unint32_t cast to an unsigned char, but on
> platforms which have 8-byte pointers, this leads to a buffer overflow in
> checksum_create() when it tries to read more than the provided 4 bytes.
>
It is a remnant of the original implementation that passed in a uint32
and a later change to char*.
> I *think* the correct fix is to add a digest_size argument
> to svn_checksum__from_digest_fnv1a_32x4() and
> svn_checksum__from_digest_fnv1a_32(), but I'm not sure if there's a better
> way which somebody more familiar with this code would know about.
>
The digest size is known just like in the functions for sha1 and md5.
Moreover, it is redundant with the checksum kind already given.
r1654981 fixes the problem.
> By all accounts, this code hasn't been released yet, so it's not a yet
> security issue (hence the post to dev@ and not private@), but I thought
> somebody would like to be aware of it. :)
>
Thanks for the review! Because the current callers would probably
never have triggered the segfault, this bug might have lingered in
there for years ...
- Stefan^2.
