Hi Mat, On 06/10/2016 12:09 PM, Mat Martineau wrote: > There are some significant differences in the current iteration of > kernel AF_ALG akcipher support: > * Kernel handles padding > * Kernel accepts DER-encoded certs as-is (no need to extract values) > * Must explicitly set private or public key > --- > ell/cipher-private.h | 3 - > ell/cipher.c | 311 ++++++++++++++------------------------------------- > ell/tls.c | 43 +++---- > 3 files changed, 97 insertions(+), 260 deletions(-) > > LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv, > @@ -275,6 +293,7 @@ LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv, > struct l_asymmetric_cipher { > struct l_cipher cipher; > int key_size; > + bool public_key; Do we really need this variable? It seems that we only use it inside l_asymmetric_cipher_new. In which case we can have a local variable to that function and have parse_rsa_key have an out parameter for this. > }; > > static inline int parse_asn1_definite_length(const uint8_t **buf, > @@ -334,20 +353,31 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key, > * and cache the size of the modulus n for later use. > * (RFC3279) > */ > + size_t seq_length; > size_t n_length; > + uint8_t *seq; > uint8_t *der; > uint8_t tag; > + bool pubkey = true; > > if (key_length < 8) > return false; > > /* Unpack the outer SEQUENCE */ > - der = der_find_elem((uint8_t *) key, key_length, 0, &tag, &n_length); > - if (!der || tag != ASN1_ID_SEQUENCE) > + seq = der_find_elem((uint8_t *) key, key_length, 0, &tag, &seq_length); > + if (!seq || tag != ASN1_ID_SEQUENCE) > return false; > > - /* Take first INTEGER as the modulus */ > - der = der_find_elem(der, n_length, 0, &tag, &n_length); > + /* First INTEGER may be a 1-byte version (for private key) or > + * the modulus (public key) > + */ > + der = der_find_elem(seq, seq_length, 0, &tag, &n_length); > + if (der && tag == ASN1_ID_INTEGER && n_length == 1) { > + /* Found version number, implies this is a private key. */ > + der = der_find_elem(seq, seq_length, 1, &tag, &n_length); > + pubkey = false; > + } > + > if (!der || tag != ASN1_ID_INTEGER || n_length < 4) > return false; > > @@ -358,95 +388,11 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key, > } > > cipher->key_size = n_length; > + cipher->public_key = pubkey; > I'm okay with this for now, but really I'd rather that we didn't have any ASN.1 parsers inside ell. Should we go in the direction of doing something like: acipher = l_asymmetric_cipher_new(type); l_asymmetric_cipher_set_pubkey(acipher, key, key_size); l_asymmetric_cipher_set_privkey(acipher, key, key_size); l_asymmetric_cipher_get_key_size(acipher); > return true; > } > > LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new( > enum l_asymmetric_cipher_type type, > const void *key, size_t key_length) > @@ -468,19 +414,23 @@ LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new( > if (!parse_rsa_key(cipher, key, key_length)) > goto error_free; > > - alg_name = "rsa"; > + alg_name = "pkcs1pad(rsa)"; > break; > } > > cipher->cipher.encrypt_sk = create_alg("akcipher", alg_name, > - key, key_length); > + key, key_length, > + cipher->public_key); > if (cipher->cipher.encrypt_sk < 0) > goto error_free; > > - cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name, > - key, key_length); > - if (cipher->cipher.decrypt_sk < 0) > - goto error_close; > + if (!cipher->public_key) { > + cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name, > + key, key_length, > + cipher->public_key); > + if (cipher->cipher.decrypt_sk < 0) > + goto error_close; > + } > > return cipher; > > -LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher, > +LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *acipher, > const void *in, void *out, > size_t len_in, size_t len_out) > { > - if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) { > - /* PKCS#1 v1.5 RSA padding according to RFC3447 */ > - uint8_t buf[cipher->key_size]; > - int pos; > - > - if (len_in != (size_t) cipher->key_size) > - return false; > - > - if (!l_cipher_decrypt(&cipher->cipher, in, buf, > - cipher->key_size)) > - return false; > - > - if (buf[0] != 0x00) > - return false; > - if (buf[1] != 0x02) > - return false; > - > - for (pos = 2; pos < cipher->key_size; pos++) > - if (buf[pos] == 0) > - break; > - if (pos < 10 || pos == cipher->key_size) > - return false; > - > - pos++; > - if (len_out != (size_t) cipher->key_size - pos) > - return false; > - > - memcpy(out, buf + pos, cipher->key_size - pos); > - } > + if (unlikely(!acipher)) > + return false; > > - return true; > + if (unlikely(!in) || unlikely(!out)) > + return false; > + This should probably check for validity of decrypt_sk > + return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_DECRYPT, > + in, out, len_in, len_out); > } > > LIB_EXPORT bool l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher, > const void *in, void *out, > size_t len_in, size_t len_out) > { > - if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) { > - /* PKCS#1 v1.5 RSA padding according to RFC3447 */ > - uint8_t buf[cipher->key_size]; > - int ps_len = cipher->key_size - len_in - 3; > - > - if (len_in > (size_t) cipher->key_size - 11) > - return false; > - if (len_out != (size_t) cipher->key_size) > - return false; > - > - buf[0] = 0x00; > - buf[1] = 0x01; > - memset(buf + 2, 0xff, ps_len); > - buf[ps_len + 2] = 0x00; > - memcpy(buf + ps_len + 3, in, len_in); > - > - /* > - * The RSA signing operation uses the same primitive as > - * decryption so just call decrypt for now. > - */ > - if (!l_cipher_decrypt(&cipher->cipher, buf, out, > - cipher->key_size)) > - return false; > - } > + if (unlikely(!acipher)) > + return false; > > - return true; > + if (unlikely(!in) || unlikely(!out)) > + return false; > + > + return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_SIGN, > + in, out, len_in, len_out); Same here > } > Regards, -Denis