sha2 error cleanup looks broken

[avsm]

There are a number of cases in sha2.c where the sizeof a pointer is zeroed instead of the size of the underlying buffer. See all the cases of MEMSET_BZERO

e.g.

@@ -1111,7 +1111,7 @@ char *SHA256_End(SHA_CTX* context, char buffer[]) {
                }
                *buffer = (char)0;
        } else {
-               MEMSET_BZERO(context, sizeof(context));
+               MEMSET_BZERO(context, sizeof(SHA_CTX));
        }
        MEMSET_BZERO(digest, SHA256_DIGEST_LENGTH);
        return buffer;

[pqwy]

I'm trying to wrap my head around this... looks like such a rookie mistake.

I took this implementation without reading, because it seems to be often used in free software projects and just assumed that 10 years was enough for people to feast their eyes on the code.

Do you know of a better SHA1+2 public domain source?

[avsm]

It does look pretty bad; what is the source? Should bring this up upstream.

I notice there's a reference C implementation in RFC6234...

[hannesm]

the sha2 implementation originates from https://web.archive.org/web/20070205153303/http://www.adg.us/computers/sha.html ... we were searching hard for a public domain one (thus not the OpenSSL one)

[pqwy]

Yeah, the idea was to use public domain, old and known C kernels for block ciphers and hashes. This implementation was what a zillion open source projects included for their SHA2 needs (google it).

The logic being that if it's used all over and has been around for a while, it's also probably well-audited.

I'm still undecided. This happens when erasing the hash state, something I'm in general not sure about, and a pointer is certainly smaller than any of the components of the union. I just can't accept such widely used code has had such an error for so long...

[avsm]

This is a very common failure mode; I patched the OpenBSD gcc to find bugs of this nature a decade ago and it found hundreds. http://undeadly.org/cgi?action=article&sid=20030627104847

Don't worry about age when it comes to code correctness -- the bazaar can empty out remarkably quickly when it comes to reading other people's code :-)