Skip to content

hash: openssl: check return values of SHA1_* functions - #4437

Merged
ethomson merged 2 commits into
libgit2:masterfrom
pks-t:pks/openssl-hash-errors
Jan 3, 2018
Merged

hash: openssl: check return values of SHA1_* functions#4437
ethomson merged 2 commits into
libgit2:masterfrom
pks-t:pks/openssl-hash-errors

Conversation

@pks-t

@pks-t pks-t commented Dec 8, 2017

Copy link
Copy Markdown
Member

The OpenSSL functions SHA1_Init, SHA1_Update and SHA1_Final all
return 1 for success and 0 otherwise, but we never check their return
values. Do so.

@ethomson

Copy link
Copy Markdown
Member

Agreed. We should set an error message though, otherwise it will likely be sent empty down to the caller, which would be unhelpful.

The OpenSSL functions `SHA1_Init`, `SHA1_Update` and `SHA1_Final` all
return 1 for success and 0 otherwise, but we never check their return
values. Do so.
@pks-t
pks-t force-pushed the pks/openssl-hash-errors branch from ffcbdbe to 75e1737 Compare January 3, 2018 12:49
The function `ERR_error_string` can be invoked without providing a
buffer, in which case OpenSSL will simply return a string printed into a
static buffer. Obviously and as documented in ERR_error_string(3), this
is not thread-safe at all. As libgit2 is a library, though, it is easily
possible that other threads may be using OpenSSL at the same time, which
might lead to clobbered error strings.

Fix the issue by instead using a stack-allocated buffer. According to
the documentation, the caller has to provide a buffer of at least 256
bytes of size. While we do so, make sure that the buffer will never get
overflown by switching to `ERR_error_string_n` to specify the buffer's
size.
@pks-t

pks-t commented Jan 3, 2018

Copy link
Copy Markdown
Member Author

Thanks @ethomson. I am now also setting an error message, even though I'm not using 'ERR_error_message' as I have no idea whether that is being set for SHA1 errors. While digging into its documentation, I also realized we are using 'ERR_error_message' in a thread-unsafe way, as we have it print into a shared static buffer. I fixed that issue as well.

@ethomson
ethomson merged commit a223bae into libgit2:master Jan 3, 2018
@pks-t pks-t added the backport label Jan 11, 2018
@pks-t pks-t mentioned this pull request Jan 12, 2018
@pks-t
pks-t deleted the pks/openssl-hash-errors branch July 11, 2019 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants