Simplify RSA and EC public key formats #13
No reviewers
Labels
No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-ARMmbed/mbed-crypto!13
Loading…
Reference in a new issue
No description provided.
Delete branch "pubkey-format"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Out of scope of this PR:
Force push to update commit descriptions.
Rebased to add format specification for DH and DSA keys, and remove the request for crypto know-howerage. No implementation updates required for those, as Mbed Crypto doesn't have a PSA-exposed implementation of DH or DSA yet.
Rebase to fix memory leak fix (remember to free the imported "their_key") in ECDH.
Rebased and force-pushed an old version of this patchset so @gilles-peskine-arm can post his comments on the patchset he reviewed. Will push back the new version after no more active reviewers.
crypto_sizes.hneeds some updates: implementation and documentation of some of thePSA_KEY_EXPORT_xxxmacros.@ -448,8 +467,32 @@ psa_status_t psa_export_key(psa_key_handle_t handle,* The output of this function can be passed to psa_import_key() toRemove this paragraph. SubjectPublicKeyInfo is no longer used. Use similar wording as in
psa_export_keyto introduce the list and the list elements.@ -453,0 +477,4 @@* For standard key types, the output format is as follows:* - For RSA public keys (#PSA_KEY_TYPE_RSA_PUBLIC_KEY), the DER encoding of* the representation defined by RFC 3279 §2.3.1 as `RSAPublicKey`.* ```@ -459,21 +502,6 @@ psa_status_t psa_export_key(psa_key_handle_t handle,* algorithm OBJECT IDENTIFIER,* parameters ANY DEFINED BY algorithm OPTIONAL }@ -2171,1 +2139,3 @@* #PSA_ALG_IS_KEY_AGREEMENT(\p alg) is true).* \param[in,out] generator The generator object to set up. It must have been* initialized as per the documentation for* #psa_crypto_generator_t and not yet in use.More generally, the peer key must be in the same format that
psa_import_keyaccepts for the public key type corresponding to the type ofprivate_key. That is, this function performs the equivalent ofpsa_import_key(internal_public_key_handle, PSA_KEY_TYPE_PUBLIC_KEY_OF_KEYPAIR(private_key_type), peer_key, peer_key_length)whereprivate_key_typeis the type ofprivate_key. For EC keys, this means thatpeer_keyis interpreted as a point on the curve that the private key is on.Now that the pk logic is only used for RSA keys, please move it to
psa_import_rsa_key.@ -638,59 +701,29 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot,status = psa_import_ec_private_key( PSA_KEY_TYPE_GET_CURVE( slot->type ),This is confusing. The code is only used for public keys (which is fortunate because it only works for public keys) but this is not immediately apparent when reading the code. Please move the two ECC blocks together, and change the condition on the public-key block to
PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY.@ -693,3 +726,3 @@return( PSA_SUCCESS );return( status );}Good fix! It happened to be ok with our implementation because
mbedtls_ecp_keypair_initsets everything to 0, but it was a bug withMBEDTLS_ECP_ALTimplementations that don't set everything to 0.Should be under
#if defined(MBEDTLS_RSA_C)#if defined(MBEDTLS_ECP_C)Should be under
#if defined(MBEDTLS_RSA_C)@ -973,1 +1022,4 @@ret = pk_write_pubkey_simple( &pk, data, data_size );}else{The two branches of this condition call identical functions. We may eventually have different code paths for RSA and ECC to decrease our reliance on pk, but it's too early for that. Just call
mbedtls_pk_write_pubkeyhere.Instead of having each case end with
if(status != PSA_SUCCESS) return status, I think this function would be clearer if it just didstatus = call_the_right_function_for_this_block()and thenreturn( status )at the end.???
Rebased to update back to where we were before and additionally improve the Doxygen comments.
@ -2171,1 +2139,3 @@* #PSA_ALG_IS_KEY_AGREEMENT(\p alg) is true).* \param[in,out] generator The generator object to set up. It must have been* initialized as per the documentation for* #psa_crypto_generator_t and not yet in use.Thanks for the explanation.
Do you suggest a change to the documentation here?
@ -638,59 +701,29 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot,status = psa_import_ec_private_key( PSA_KEY_TYPE_GET_CURVE( slot->type ),Yeah, that's an obvious change to make now that we don't have a shared block for RSA and ECC public keys anymore.
Agreed, but out of scope for now.
This is something I noticed when reading the ASN.1 and reading SEC 1 (http://www.secg.org/sec1-v2.pdf). We previously were using
BIT_STRING(0x03) instead ofOCTET_STRING(0x04) per the standard.This might be worth fling an inter-op bug about unless we know some reason using
BIT_STRINGis correct. It's not an issue for PSA anymore, since we won't use an ASN.1-formatted key, but is still a potential issue for Mbed TLS.@ -973,1 +1022,4 @@ret = pk_write_pubkey_simple( &pk, data, data_size );}else{I was thinking to possibly do that in this PR, but since we can agree it's out of scope, I'll merge the implementations.
+ my comments from the previous review
Missing a
handle_tparameterconstNot relevant for this function (you won't get a handle in the first place)
INVALID_ARGUMENTSNot relevant for this function (you won't get a handle in the first place)
INVALID_ARGUMENTif the key type has no domain parametersNOT_SUPPORTEDif the implementation doesn't have code to format domain parametersMissing handle parameter
@ -453,0 +484,4 @@* ```* - For elliptic curve public keys (key types for which* #PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY is true), the format is the uncompressed* representation defined by SEC1 §2.3.3 as the content of an ECPoint:“a” → “the”, and specify the length of
x. There are two sensible choices: determined by the key size, or without leading zeros. DER packaging would strip leading zeros, but this leaks a fraction of a bit of information about the private key through the private key size, so I think it's better to avoid this. A fixed size is also simpler for applications. So the size ofxis the private key size in bytes. (DSA private key sizes are always a multiple of 32 bits, so there are no concerns about non-byte-aligned sizes.)“a” → “the”, and specify the length of
x. There are two sensible choices: determined by the key size, or without leading zeros. DER packaging would strip leading zeros, but this leaks a fraction of a bit of information about the private key through the private key size, so I think it's better to avoid this. A fixed size is also simpler for applications. So the size ofxis the byte size of the subgroup primeq.Like for the private key, specify the length of the string and “a” → “the”. For DSA the public key should have the same length as the base prime
p.Like for the private key, specify the length of the string and “a” → “the”. For DH, like for DSA, the public key should have the same length as the base prime
p.“
y(g^x mod p)” looks likey(g^x mod p)ory × (g^x mod p). Writey = g^x mod por “y(which isg^x mod p)” or just leave out theg^x mod pbit. Be consistent with DSA: it's the same math.What optional structure?
@ -2171,1 +2139,3 @@* #PSA_ALG_IS_KEY_AGREEMENT(\p alg) is true).* \param[in,out] generator The generator object to set up. It must have been* initialized as per the documentation for* #psa_crypto_generator_t and not yet in use.Yes: what I wrote above. With or without the specific case of EC keys.
What should be returned if the handle is invalid and the library hasn't been initialized yet? Both errors, right? :p
When would we return
INVALID_ARGUMENT? We'd expect to be able to call this function before we know the key type as specified during import or generate, right? So, it wouldn't be when the key type is known to not need or support key domain parameters, as we don't know the key type yet.How do we know the key type?
The one specifying the key domain parameters and whose documentation is referenced a bit later in the entry. Do you have a suggestion to make it more clear or less confusing?
Oh, I see what you mean. I'm quite confident that there is no interoperability bug since Mbed TLS is able to interact with the other TLS implementations out there. The bug is partly on me for using the name
ECPointfor something that is not what the applicable specifications callECPoint, and I would say partly on SEC1 for having confusing language.RFC 5280 defines SubjectPublicKeyInfo as containing a subjectPublicKey which is always a BIT STRING. The content of this BIT STRING depends on the key type. RFC 3279 defines the content of a subjectPublicKey for some common key types. For an RSA or DSA or DH key, the content of the BIT STRING is an ASN.1 SEQUENCE. For an EC key, SEC 1 §C.3 says (the same language appears in RFC 3279 §2.3.5):
What you do here is to take the content of the ECPoint and stuff it into a BIT STRING. You don't stuff the OCTET STRING (
04+ length + content) into the BIT STRING. By coincidence, the content of the BIT STRING happens to start with04in all the data we manipulate because that's the prefix for an uncompressed point representation (SEC1 §2.3.3 action 3.3). You can argue that SEC1 is ambiguous, and I'd agree with that, but everybody including the de facto standard openssl stuffs an EC point representation into a BIT STRING and not an ECPoint into a BIT STRING.I filed a spec bug to go through all uses of “ECPoint” and fix them.
Unless otherwise specified, if multiple error codes apply, the implementation picks whichever it likes.
Rebased to address code review feedback.
That's a problem: this function needs to know how to interpret the domain parameters. It will probably want to convert the string to an internal representation. The type needs to be known at this point. The domain parameters are a sort of subtype, you can't have domain parameters (as in the mathematical object and not just some string representation with an unspecified encoding) without having the key type.
It needs to have been specified one way or the other. See the internal spec discussion at https://github.com/ARMmbed/psa-crypto/issues/96
Rebased to remove XXX observation for ECPoint representation.
It's not a matter of clarity, it's a matter of completeness. You never specify what the structure is. This should probably not be a structure since
psa_set_key_domain_parametersdoesn't take a structure as input: it should be a buffer.It's an issue with clarity, because I didn't mean structure in the C struct case. I meant in the abstract sense, so I can clarify by saying "buffer" instead of "structure".
Handle parameter documentation is missing. Similarly for
psa_set_key_domain_parameters.Rebased to remove DH and DSA work, which has been raised in PR #14 instead.
@ -902,0 +937,4 @@unsigned char *buf, size_t size ){int ret;unsigned char *c;With this change, I think most of the
if( status != PSA_SUCCESS ) return( status );code could be removed and thereturn( PSA_SUCCESS )at the end changed toreturn( status )The only one that needs to remain is the one in the raw byte section.
Added a commit to refactor import, as suggested by both @gilles-peskine-arm and @dgreen-arm, making the code a bit easier to read.
Darryl suggested the same change, so I went ahead and did it.
Added commit for documentation change suggestion from @gilles-peskine-arm
Rebased to fix https://github.com/ARMmbed/mbed-crypto/pull/13#discussion_r247152162 in the EC format simplifying commit
crypto_sizes.h.@ -448,8 +467,32 @@ psa_status_t psa_export_key(psa_key_handle_t handle,* The output of this function can be passed to psa_import_key() toPreexisting: that's not a DER encoding and it's the content of an ECPoint, not an ECPoint.
Fixing all the misuses of
ECPointis out of scope of this PR, but please fix it in the places that you're modifying anyway.@ -2170,2 +2139,2 @@* (\c PSA_ALG_XXX value such that* #PSA_ALG_IS_KEY_AGREEMENT(\p alg) is true).* \param[in,out] generator The generator object to set up. It must have been* initialized as per the documentation formissing formatting codes (
`…`or equivalent).@ -512,50 +512,112 @@ static psa_status_t psa_check_rsa_key_byte_aligned(return( status );“any type-specific checks” is an obsolete comment that must date back from when this code also handled non-RSA. The comment should now read something like “Check that it's an RSA key”, or maybe the flow is easy enough to follow without a comment.
Since
rsahas been lifted outside theelse, might as well also liftbitsand not have anelseblock here.(“The point belongs to the group” is a mathematically correct way to phrase it but not what crypto people use in this context.)
@ -4075,4 +4067,3 @@size_t *shared_secret_length ){mbedtls_pk_context pk;mbedtls_ecp_keypair *their_key = NULL;Missing
mbedtls_to_psa_error.Why wasn't this caught by a test? I think because if we reach this point, we know that
our_keyis a valid key pair on a Montgomery curve and so the only possible failures areALLOC_FAILEDandHW_ACCEL_FAILED.Rebased to address code review feedback
Rebased to add a few more formatting markers to
psa_key_agreement()Doxygen.@ -448,8 +467,32 @@ psa_status_t psa_export_key(psa_key_handle_t handle,* The output of this function can be passed to psa_import_key() toDouble 'the format is the'
Rebased to fix the "the format is the the format is" problem.
Ok for
crypto.h,psa_crypto.cand the tests. Missing: updating the relevant macros and documentation incrypto_sizes.h.@ -453,0 +491,4 @@* - `x_P` as a `ceiling(m/8)`-byte string, big-endian;* - `y_P` as a `ceiling(m/8)`-byte string, big-endian.** For other public key types, the format is the DER representation defined byWe don't specify what implementations should/can do about the
ycoordinate in the case ofPSA_ECC_CURVE_CURVE25519andPSA_ECC_CURVE_CURVE448. Do we want to? I would assume that implementations simply ignore it, but are they allowed to omit it? Do we want to prescribe what is allowed if there is a parsing error ony?@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted by"private key is on." -> "private key is associated with."
(The private key is an integer it is not a point it is not on any curve.)
@ -453,0 +491,4 @@* - `x_P` as a `ceiling(m/8)`-byte string, big-endian;* - `y_P` as a `ceiling(m/8)`-byte string, big-endian.** For other public key types, the format is the DER representation defined byWhat would we do with
yif we had it? If we don't imagine we or any other implementations would have a use for it, we shouldn't require it.@ -453,0 +491,4 @@* - `x_P` as a `ceiling(m/8)`-byte string, big-endian;* - `y_P` as a `ceiling(m/8)`-byte string, big-endian.** For other public key types, the format is the DER representation defined bySome implementations might want to require it to be a specific value for robustness.
But I just realised that Montgomery curves are out of scope for 1.0:
https://github.com/ARMmbed/psa-crypto/issues/101
So we could perhaps make it explicit that Montgomery and Edwards curves are not supported yet.
@gilles-peskine-arm what would be the best way to resolve the ambiguity here?
@ -453,0 +491,4 @@* - `x_P` as a `ceiling(m/8)`-byte string, big-endian;* - `y_P` as a `ceiling(m/8)`-byte string, big-endian.** For other public key types, the format is the DER representation defined byIt's in scope for 1.0, but not for 1.0beta1. I think the 1.0 specification should define the format for Montgomery curves (and the reference implementation should support ECDH) and for Edwards curves (even if the reference implementation doesn't support them yet).
@ -453,0 +491,4 @@* - `x_P` as a `ceiling(m/8)`-byte string, big-endian;* - `y_P` as a `ceiling(m/8)`-byte string, big-endian.** For other public key types, the format is the DER representation defined byIn this case I suggest we define the format according to RFC8032 and RFC7748. (That is
xcoordinate in little endian byte order in the case of Montgomery andycoordinate in little endian byte order, with the most significant bit of the last byte set to the least significant bit ofx.)@ -448,8 +467,32 @@ psa_status_t psa_export_key(psa_key_handle_t handle,* The output of this function can be passed to psa_import_key() toMinor: The reading flow is slightly interrupted here because it's not clear that you'll go on explaining the format. I think using a colon
:instead of a full stop.after... of an ECPointwould help.@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byMinor: Use
\p.@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byMinor: Use
\p private_key.@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byMinor: Use
\p peer_key.(Very) minor: "algorithm to compute" sounds wrong, but that might just be me as a non-native. What about "The key agreement algorithm to use."?
@ -545,0 +572,4 @@#if defined(MBEDTLS_ECP_C)/* Import a public key given as the uncompressed representation defined by SEC1* 2.3.3 as the content of an ECPoint. */Do you also want to apply a lower bound on the number of bits in the RSA modulus?
@ -549,0 +588,4 @@return( PSA_ERROR_INSUFFICIENT_MEMORY );mbedtls_ecp_keypair_init( ecp );/* Load the group. */Note: Strictly speaking, this violates the PK API because it is assumed that apart from the RSA context within the PK context, there is no other structure worth freeing.
That formulation is from me. It's copied from
psa_key_derivationand will move topsa_key_derivation_setupin https://github.com/ARMmbed/mbedtls-psa/pull/244 . I'm not fond of “use” because it is very generic. “Algorithm to perform”? What do you think @Patater ?@ -545,0 +572,4 @@#if defined(MBEDTLS_ECP_C)/* Import a public key given as the uncompressed representation defined by SEC1* 2.3.3 as the content of an ECPoint. */At the moment we're applying an upper bound in PSA to limit the amount of memory and CPU time that an operation might use. A lower bound would come from a security policy, and we don't have a minimum-key-size security policy. I'd prefer to do nothing in the PSA code for now and only follow Mbed TLS, but this isn't workable here since Mbed TLS allows unrealistiaclly large RSA key sizes.
@ -549,0 +588,4 @@return( PSA_ERROR_INSUFFICIENT_MEMORY );mbedtls_ecp_keypair_init( ecp );/* Load the group. */mbedtls_pk_contextis defined inpk.h, notpk_internal.h, and its fields are documented. So this assumption is validate by the documentation ofpk.h, even if we might wish that the type was more opaque.mbedtls_ecp_point_read_binary()supports the 1-Byte representation{ 0x00 }of the point of infinity, but as far as I see it's not documented above. If we don't want to accept the point at infinity here, we should rule it out by e.g. checkingdata_length.@ -573,6 +635,7 @@ static psa_status_t psa_import_ec_private_key( psa_ecc_curve_t curve,ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) );if( ecp == NULL )return( PSA_ERROR_INSUFFICIENT_MEMORY );mbedtls_ecp_keypair_init( ecp );Oh ok, this would fail for the point at infinity.
Yeah, algorithm to compute sounds like you are computing an algorithm. That's second order. Can't have computers programming themselves...
"Algorithm to follow" sounds OK to me.
Won't this later get rejected during
mbedtls_ecp_check_pubkey?@ -573,6 +635,7 @@ static psa_status_t psa_import_ec_private_key( psa_ecc_curve_t curve,ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) );if( ecp == NULL )return( PSA_ERROR_INSUFFICIENT_MEMORY );mbedtls_ecp_keypair_init( ecp );Yes, OK.
@ -545,0 +572,4 @@#if defined(MBEDTLS_ECP_C)/* Import a public key given as the uncompressed representation defined by SEC1* 2.3.3 as the content of an ECPoint. */No change planned here for this PR
@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byFixed
@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byFixed
@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byFixed
@ -448,8 +467,32 @@ psa_status_t psa_export_key(psa_key_handle_t handle,* The output of this function can be passed to psa_import_key() toAdded a colon
@ -2156,19 +2136,28 @@ psa_status_t psa_key_derivation(psa_crypto_generator_t *generator,* The resulting generator always has the maximum capacity permitted byFixed
@ -453,0 +491,4 @@* - `x_P` as a `ceiling(m/8)`-byte string, big-endian;* - `y_P` as a `ceiling(m/8)`-byte string, big-endian.** For other public key types, the format is the DER representation defined byNot changing format in this PR. Agreeing on a format is tracked in ARMmbed/psa-crypto#101
Rebased to update macros and documentation in
crypto_sizes.h. Also addressed review comments from @hanno-armRebased to remove stale reference to ECPoint in
crypto_sizes.h.