New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update hkdf.c to avoid potentially vulnerable code pattern #20990
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, trivial.
You need to have the CLA: trivial line in each commit. It's only in the second, not the first.
I'd suggest deleting the second commit entirely and amending the first to include the CLA: trivial line.
The expression "if (a+b>c) a=c-b" is incorrect if "a+b" overflows. It should be replaced by "if (a>c-b) a=c-b", which avoids the potential overflow and is much easier to understand. This pattern is the root cause of CVE-2022-37454, a buffer overflow vulnerability in the "official" SHA-3 implementation. It has been confirmed that the addition in https://github.com/openssl/openssl/blob/master/providers/implementations/kdfs/hkdf.c#L534 cannot overflow. So this is only a minor change proposal to avoid a potentially vulnerable code pattern and to improve readability. More information: github/codeql#12036 (comment) CLA: trivial
Took the liberty of squashing everything down neatly. This should fix the CLA tag. |
Hi @paulidale, Thanks for looking into this and for fixing the CLA tag! Sorry for messing up the pull request... |
No problem, we've all done similar and will again. |
This pull request is ready to merge |
Merged to master, 3.1, and 3.0 branches. Thank you for your contribution. |
The expression "if (a+b>c) a=c-b" is incorrect if "a+b" overflows. It should be replaced by "if (a>c-b) a=c-b", which avoids the potential overflow and is much easier to understand. This pattern is the root cause of CVE-2022-37454, a buffer overflow vulnerability in the "official" SHA-3 implementation. It has been confirmed that the addition in https://github.com/openssl/openssl/blob/master/providers/implementations/kdfs/hkdf.c#L534 cannot overflow. So this is only a minor change proposal to avoid a potentially vulnerable code pattern and to improve readability. More information: github/codeql#12036 (comment) CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20990) (cherry picked from commit 56a51b5)
The expression "if (a+b>c) a=c-b" is incorrect if "a+b" overflows. It should be replaced by "if (a>c-b) a=c-b", which avoids the potential overflow and is much easier to understand. This pattern is the root cause of CVE-2022-37454, a buffer overflow vulnerability in the "official" SHA-3 implementation. It has been confirmed that the addition in https://github.com/openssl/openssl/blob/master/providers/implementations/kdfs/hkdf.c#L534 cannot overflow. So this is only a minor change proposal to avoid a potentially vulnerable code pattern and to improve readability. More information: github/codeql#12036 (comment) CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20990) (cherry picked from commit 56a51b5)
The expression "if (a+b>c) a=c-b" is incorrect if "a+b" overflows. It should be replaced by "if (a>c-b) a=c-b", which avoids the potential overflow and is much easier to understand. This pattern is the root cause of CVE-2022-37454, a buffer overflow vulnerability in the "official" SHA-3 implementation. It has been confirmed that the addition in https://github.com/openssl/openssl/blob/master/providers/implementations/kdfs/hkdf.c#L534 cannot overflow. So this is only a minor change proposal to avoid a potentially vulnerable code pattern and to improve readability. More information: github/codeql#12036 (comment) CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20990)
The expression
if (a+b>c) a=c-b
is incorrect ifa+b
overflows. It should be replaced byif (a>c-b) a=c-b
, which avoids the potential overflow and is much easier to understand. This pattern is the root cause of CVE-2022-37454, a buffer overflow vulnerability in the "official" SHA-3 implementation.It has been confirmed that the addition in hkdf.c:534 cannot overflow. So this is only a minor proposed change to avoid potentially vulnerable code patterns and to improve readability. More information: github/codeql#12036 (comment)