Forbid zero-length keys #257
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clarify how key creation functions use attributes. Explain the meaning of attribute values, espcially what 0 means in each field where it has a special meaning. Explain what an algorithm usage policy can be (an algorithm, a wildcard with ANY_HASH, or 0).
Keys of size 0 generally don't make sense: a key is supposed to be secret. There is one edge case which is "raw data" keys, which are useful to store non-key objects in the same storage location as keys. However those are also problematic because they involve a zero-length buffer. Manipulating zero-length buffers in C requires special cases with functions like malloc() and memcpy(). Additionally, 0 as a key size already has a meaning "unspecified", which does not always overlap seamlessly with the meaning "0". Therefore, forbid keys of size 0. No implementation may accept them.
If there isn't already a test with a raw data key of the now-minimal length (1 byte), change the test case to a 1-byte key.
Check that zero-length keys cannot be imported, generated or derived.
Implement the prohibition on keys of size 0.
Add tests for derivation. Test both 7 bits and 9 bits, in case the implementation truncated the bit size down and 7 was rejected as 0 rather than because it isn't a multiple of 8. There is no corresponding test for import because import determines the key size from the key data, which is always a whole number of bytes.
Zero-length keys are rejected at creation time, so we don't need any special handling internally. When exporting a key, we do need to take care of the case where the output buffer is empty, but this is easy: an empty output buffer is never valid.
gilles-peskine-arm
added
bug
In psa_asymmetric_sign, immediately reject an empty signature buffer. This can never be right. Add test cases (one RSA and one ECDSA). Change the SE HAL mock tests not to use an empty signature buffer.
Patater
suggested changes
Sep 13, 2019
|
Is there any code from #48 that we'd like to remove now that we disallow zero-length keys? |
|
Regarding #48, I didn't look at it specifically, but “Remove special handling for zero-length keys” effectively reverts it as well as removing similar checks in other functions. |
The signature must have exactly the same length as the key, it can't be longer. Fix ARMmbed#258 If the signature doesn't have the correct size, that's an invalid signature, not a problem with an output buffer size. Fix the error code. Add test cases.
gilles-peskine-arm
force-pushed
the
psa-remove_zero_length_keys
branch
from
33c0c31 to
89cc74f
Compare
|
I'm happy with the updates to the documentation.. |
|
When this is merged, should we also close #50 now that it's a feature and no longer a bug? |
Yes. I see you've address this in your PR description. Steps ahead of me. :) |
This was referenced Sep 17, 2019
k-stachowiak
approved these changes
Sep 19, 2019
gilles-peskine-arm
removed
the
needs: review
This was referenced Sep 23, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Keys of size 0 generally don't make sense: a key is supposed to be secret. There is one edge case which is "raw data" keys, which are useful to store non-key objects in the same storage location as keys. However those are also problematic because they involve a zero-length buffer. Manipulating zero-length buffers in C requires special cases with functions like malloc() and memcpy(). Additionally, 0 as a key size already has a meaning "unspecified", which does not always overlap seamlessly with the meaning "0".
Therefore, forbid keys of size 0. No PSA implementation may accept them. Reject them in Mbed Crypto.
callocon different libraries when trying to allocate 0 bytes #137, but only for key objects and inpsa_asymmetric_sign, not for buffers containing other things.