Re: [PATCH] trusted-keys: skcipher bug info
On Tue, 2016-09-20 at 20:35 +0800, Herbert Xu wrote: > On Tue, Sep 20, 2016 at 08:11:51AM -0400, Mimi Zohar wrote: > > Hi Herbert, > > > > The initial random iv value, initialized in encrypted_init(), should > > not be modified. Commit c3917fd "KEYS: Use skcipher", which replaced > > the blkcipher with skcipher, modifies the iv in > > crypto_skcipher_encrypt()/decrypt(). > > > > The following example creates an encrypted key, writes the key to a > > file, and then loads the key from the file. To illustrate the problem, > > this patch provides crypto_skcipher_encrypt()/decrypt() with a copy of > > the iv. With this change, the resulting test-key and test-key1 keys > > are the same. > > Sorry, I missed the subtlety. This patch should fix the problem. Thanks! Mimi > > ---8<--- > Subject: KEYS: Fix skcipher IV clobbering > > The IV must not be modified by the skcipher operation so we need > to duplicate it. > > Fixes: c3917fd9dfbc ("KEYS: Use skcipher") > Cc: sta...@vger.kernel.org > Reported-by: Mimi Zohar> Signed-off-by: Herbert Xu > > diff --git a/security/keys/encrypted-keys/encrypted.c > b/security/keys/encrypted-keys/encrypted.c > index 5adbfc3..17a0610 100644 > --- a/security/keys/encrypted-keys/encrypted.c > +++ b/security/keys/encrypted-keys/encrypted.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -478,6 +479,7 @@ static int derived_key_encrypt(struct > encrypted_key_payload *epayload, > struct crypto_skcipher *tfm; > struct skcipher_request *req; > unsigned int encrypted_datalen; > + u8 iv[AES_BLOCK_SIZE]; > unsigned int padlen; > char pad[16]; > int ret; > @@ -500,8 +502,8 @@ static int derived_key_encrypt(struct > encrypted_key_payload *epayload, > sg_init_table(sg_out, 1); > sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen); > > - skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, > -epayload->iv); > + memcpy(iv, epayload->iv, sizeof(iv)); > + skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv); > ret = crypto_skcipher_encrypt(req); > tfm = crypto_skcipher_reqtfm(req); > skcipher_request_free(req); > @@ -581,6 +583,7 @@ static int derived_key_decrypt(struct > encrypted_key_payload *epayload, > struct crypto_skcipher *tfm; > struct skcipher_request *req; > unsigned int encrypted_datalen; > + u8 iv[AES_BLOCK_SIZE]; > char pad[16]; > int ret; > > @@ -599,8 +602,8 @@ static int derived_key_decrypt(struct > encrypted_key_payload *epayload, > epayload->decrypted_datalen); > sg_set_buf(_out[1], pad, sizeof pad); > > - skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, > -epayload->iv); > + memcpy(iv, epayload->iv, sizeof(iv)); > + skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv); > ret = crypto_skcipher_decrypt(req); > tfm = crypto_skcipher_reqtfm(req); > skcipher_request_free(req); > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] trusted-keys: skcipher bug info
On Tue, Sep 20, 2016 at 08:11:51AM -0400, Mimi Zohar wrote: > Hi Herbert, > > The initial random iv value, initialized in encrypted_init(), should > not be modified. Commit c3917fd "KEYS: Use skcipher", which replaced > the blkcipher with skcipher, modifies the iv in > crypto_skcipher_encrypt()/decrypt(). > > The following example creates an encrypted key, writes the key to a > file, and then loads the key from the file. To illustrate the problem, > this patch provides crypto_skcipher_encrypt()/decrypt() with a copy of > the iv. With this change, the resulting test-key and test-key1 keys > are the same. Sorry, I missed the subtlety. This patch should fix the problem. ---8<--- Subject: KEYS: Fix skcipher IV clobbering The IV must not be modified by the skcipher operation so we need to duplicate it. Fixes: c3917fd9dfbc ("KEYS: Use skcipher") Cc: sta...@vger.kernel.org Reported-by: Mimi ZoharSigned-off-by: Herbert Xu diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 5adbfc3..17a0610 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -478,6 +479,7 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, struct crypto_skcipher *tfm; struct skcipher_request *req; unsigned int encrypted_datalen; + u8 iv[AES_BLOCK_SIZE]; unsigned int padlen; char pad[16]; int ret; @@ -500,8 +502,8 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, sg_init_table(sg_out, 1); sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen); - skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, - epayload->iv); + memcpy(iv, epayload->iv, sizeof(iv)); + skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv); ret = crypto_skcipher_encrypt(req); tfm = crypto_skcipher_reqtfm(req); skcipher_request_free(req); @@ -581,6 +583,7 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, struct crypto_skcipher *tfm; struct skcipher_request *req; unsigned int encrypted_datalen; + u8 iv[AES_BLOCK_SIZE]; char pad[16]; int ret; @@ -599,8 +602,8 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, epayload->decrypted_datalen); sg_set_buf(_out[1], pad, sizeof pad); - skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, - epayload->iv); + memcpy(iv, epayload->iv, sizeof(iv)); + skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv); ret = crypto_skcipher_decrypt(req); tfm = crypto_skcipher_reqtfm(req); skcipher_request_free(req); -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] trusted-keys: skcipher bug info
Hi Herbert, The initial random iv value, initialized in encrypted_init(), should not be modified. Commit c3917fd "KEYS: Use skcipher", which replaced the blkcipher with skcipher, modifies the iv in crypto_skcipher_encrypt()/decrypt(). The following example creates an encrypted key, writes the key to a file, and then loads the key from the file. To illustrate the problem, this patch provides crypto_skcipher_encrypt()/decrypt() with a copy of the iv. With this change, the resulting test-key and test-key1 keys are the same. keyctl add user kmk "`dd if=/dev/urandom bs=1 count=32 2>/dev/null`" @u keyctl pipe `keyctl search @u user kmk` > ~/tmp/kmk keyctl add encrypted test-key "new user:kmk 64" @u keyctl pipe `keyctl search @u encrypted test-key` > /tmp/test-key1 keyctl add encrypted test-key1 "load `cat /tmp/test-key1`" @u keyctl print `keyctl search @u encrypted test-key` keyctl print `keyctl search @u encrypted test-key1` Either the underlying problem should be fixed or commit c3917fd "KEYS: Use skcipher" should be reverted. thanks, Mimi --- security/keys/encrypted-keys/encrypted.c | 22 +++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 5adbfc3..e94f586 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -480,6 +480,7 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, unsigned int encrypted_datalen; unsigned int padlen; char pad[16]; + u8 iv[16]; int ret; encrypted_datalen = roundup(epayload->decrypted_datalen, blksize); @@ -500,9 +501,19 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, sg_init_table(sg_out, 1); sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen); + memcpy(iv, epayload->iv, 16); /* iv is modified */ skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, - epayload->iv); + iv); +print_hex_dump(KERN_INFO, "original iv: ", DUMP_PREFIX_NONE, 32, 1, + epayload->iv, ivsize, 0); +print_hex_dump(KERN_INFO, "copied iv: ", DUMP_PREFIX_NONE, 32, 1, + iv, ivsize, 0); ret = crypto_skcipher_encrypt(req); +print_hex_dump(KERN_INFO, "original iv: ", DUMP_PREFIX_NONE, 32, 1, + epayload->iv, ivsize, 0); +print_hex_dump(KERN_INFO, "modified iv: ", DUMP_PREFIX_NONE, 32, 1, + iv, ivsize, 0); + tfm = crypto_skcipher_reqtfm(req); skcipher_request_free(req); crypto_free_skcipher(tfm); @@ -582,6 +593,7 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, struct skcipher_request *req; unsigned int encrypted_datalen; char pad[16]; + u8 iv[16]; int ret; encrypted_datalen = roundup(epayload->decrypted_datalen, blksize); @@ -599,8 +611,9 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, epayload->decrypted_datalen); sg_set_buf(_out[1], pad, sizeof pad); + memcpy(iv, epayload->iv, 16); /* iv is modified */ skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, - epayload->iv); + iv); ret = crypto_skcipher_decrypt(req); tfm = crypto_skcipher_reqtfm(req); skcipher_request_free(req); @@ -778,8 +791,11 @@ static int encrypted_init(struct encrypted_key_payload *epayload, get_random_bytes(epayload->decrypted_data, epayload->decrypted_datalen); - } else + } else { ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv); +print_hex_dump(KERN_INFO, "init iv: ", DUMP_PREFIX_NONE, 32, 1, + epayload->iv, ivsize, 0); + } return ret; } -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html