[botan-devel] Bug in SHA-160 with resize to zero not allocated empty buffer

Dennis Weldy dennis at correlatedmagnetics.com
Mon Jun 23 22:20:17 EDT 2014


Since deallocation of non-null 0-length pointer should be safe (presuming alloc->deallocate follows free’s semantics, likely the check against n could be removed.

Dennis

From: botan-devel [mailto:botan-devel-bounces at randombit.net] On Behalf Of William K. Foster
Sent: Monday, June 23, 2014 8:18 PM
To: Botan development list
Subject: Re: [botan-devel] Bug in SHA-160 with resize to zero not allocated empty buffer

I have found that my proposed fix leads to a memory leak due to:

      void deallocate(T* p, size_t n)
         { if(alloc && p && n) alloc->deallocate(p, sizeof(T)*n); }

skipping the deallocate of zero length.

This makes me think that the original solution even if fixed here too would lead to performance degradation.

What would be the proper solution to the original SEGV I faced with SHA-160?  Other hashes work fine, it seems only SHA-160 gets SEGVs due to null pointer dereference.



On Wed, Jun 11, 2014 at 12:32 PM, William K. Foster <wkf at alum.mit.edu<mailto:wkf at alum.mit.edu>> wrote:
No comments on this proposed bug fix?

On Tue, Jun 3, 2014 at 3:08 PM, William K. Foster <wkf at alum.mit.edu<mailto:wkf at alum.mit.edu>> wrote:
Hello,

I have come across a SEGV due to following a null pointer in the SHA-160 implementation for Botan v1.10.8.  The failure is seen in SHA_160::compress_n() being called when W has a null buf and the null pointer is dereferenced.  The SHA_160 is constructed by SHA_160_SSE2() in file src/hash/sha1_sse2/sha1_sse2.h line 22 where the SHA_160(0) is used, overriding the size to be zero. This becomes an issue during construction where Botan::MemoryRegion<unsigned int>::resize() in file src/alloc/secmem.h line 213 the code is reached with a size of n being zero but allocated is also zero, and thus the conditional takes the path of keeping the existing allocation for buf (a null pointer) for the requested zero length of memory which is then later dereferenced.

I have worked around the bug by changing the condition on line 213 to instead be:

   if(n <= allocated && (n || buf))

So that if n is zero and buf is null, the else condition is taken.

Perhaps there is a more elegant solution, but at the least the code I've changed here is unsafe in that it allows buf to be null on a resize request to zero length that appears to violate assumptions elsewhere that the buf is non null.

Please suggest a more permanent fix or let me know if you feel this patch is all that is needed.

Thanks.

-William


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.randombit.net/pipermail/botan-devel/attachments/20140624/26e5180d/attachment-0001.html>


More information about the botan-devel mailing list