Audit findings #3
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
relatko
changed the base branch from
develop
to
merging-with-onflow-latest-development-branch
src/crypto.c
Outdated
| @@ -185,7 +185,7 @@ zxerr_t crypto_sign(const hd_path_t path, | |||
| sizeof(messageDigest), | |||
| &messageDigestSize)); | |||
|
|
|||
| if (messageDigestSize != 32) { | |||
| if (messageDigestSize != CX_SHA256_SIZE) { | |||
There was a problem hiding this comment.
-
I think the audit report pointed to replacing the
32on this line. However, I don't agree with the report suggestion. There should be a new defined constantCX_SHA3_256_SIZE(also equal to 32). SHA256 refers to SHA-2, while here we are working with SHA-3 -
The line modified in this PR should actually remain 32, or should be replaced by another new constant (the elliptic curve order size), otherwise the check doesn't make sense from a cryptographic perspective. In this specific case, all constants happen to be 32 and the code passes, but if we evolve the code to support more curves/hashing, the code would become incorrect (EDIT: after thinking more on this, it can be fine to check against the expected hash output only, if the later signing function checks for curve/hash compatibility)
There was a problem hiding this comment.
I can make a minor suggestion PR in a few hours to clean up the magic numbers from a cryptographic point of a view. It would only improve the code readability, but wouldn't have any functional change on the current code.
relatko
force-pushed
the
audit_findings
branch
2 times, most recently
from
efffc6d
to
4dff0a0
Compare
relatko
merged commit 0c12067
into
merging-with-onflow-latest-development-branch
No description provided.