[Bug] in SHA1_Transform - const uint8_t buffer's data is modified

[rhoneyager]

Hi,

When calculating SHA-1 hashes of a block of data, SHA1_Update is called with a pointer to const uint8_t data.

void SHA1_Update(SHA1_CTX* context, const uint8_t* data, const size_t len)

Unfortunately, the constness of data is casted away, and the contents of data are modified during the SHA1_Transform function.

An example, using sha1.h, and also sha1.cpp's digest_to_hex:

#include <array>
#include <string>
#include <iostream>
#include "sha1.h"
void digest_to_hex(const uint8_t digest[SHA1_DIGEST_SIZE], char *output);

std::string getHash(const std::string &buffer) {
	using namespace std;
	SHA1_CTX context;
	array<uint8_t, 20> digest;
	array<char, 80> outputHash;
	SHA1_Init(&context);
	SHA1_Update(&context, reinterpret_cast<const uint8_t*>(buffer.data()), buffer.size());
	SHA1_Final(&context, digest.data());
	digest_to_hex(digest.data(), outputHash.data());
	string sHash;
	sHash += outputHash.data();
	return sHash;
}
int main(void) {
	using namespace std;
	string test_buffer(1000, 'x');
	auto hash1 = getHash(test_buffer);
	auto hash2 = getHash(test_buffer);
	auto hash3 = getHash(test_buffer);
	cout << hash1 << endl << hash2 << endl << hash3 <<endl;
}

hash1!=hash2!=hash3, and test_buffer is changed every time its SHA-1 hash is calculated.

This is fixed in other repositories, based off of the same original code (ex.: http://download.redis.io/redis-stable/src/sha1.c).

[benjamg]

An alternative fix for this would be to replace the line sha1.cpp#L198

memcpy(&context->buffer[0], data + i, 64);
SHA1_Transform(context->state, context->buffer);

Which I assume was the intened use of the buffer as it was done for the transform prior to the loop.

[sebres]

An alternative fix for this would be to replace the line sha1.cpp#L198

No, then it would not fix basic issue within SHA1_Transform, it would fix SHA1_Update only.
Either it must be fixed like I provided in #60, or header of SHA1_Transform must be changed (const gets removed).

[benjamg]

An alternative fix for this would be to replace the line sha1.cpp#L198

No, then it would not fix basic issue within SHA1_Transform, it would fix SHA1_Update only.
Either it must be fixed like I provided in #60, or header of SHA1_Transform must be changed (const gets removed).

That is the only place it's called with inital data instead of a copy, but copying in the transform works fine as well, although I'd recommend removing the context buffer then, as well as the lines where we copy into it.

[sebres]

That is the only place it's called with inital data instead of a copy

1. unless it is used later somewhere else;
2. no matter, the handling in the function does not match its declaration;

although I'd recommend removing the context buffer then

What do you talking about? SHA1_CTX::buffer is needed to follow SHA calculation (basically on every update for every chunk, because SHA-calculation is independent of data chunk size):
https://github.com/aappleby/smhasher/blob/a46b1fb03ccab42ecbc049620f0d16d56a437414/src/sha1.cpp#L193-#L194