crypto: rsa - fix buffer overread when stripping leading zeroes

PATCH In review Eric Biggers 4 participants 6 messages Nov 27, 2017 – Nov 29, 2017

Subsystems (4)

Stable branch Crypto API Keys/keyrings Asymmetric keys - rsa

lore

Share & tools

lore

Eric Biggers

author

2017-11-27 07:16 UTC #0 lore raw

PATCH 1/1 crypto: rsa - fix buffer overread when stripping leading zeroes

From: Eric Biggers <[email protected]>

In rsa_get_n(), if the buffer contained all 0's and "FIPS mode" is enabled, we would read one byte past the end of the buffer while scanning the leading zeroes. Fix it by checking 'n_sz' before '!*ptr'.

This bug was reachable by adding a specially crafted key of type "asymmetric" (requires CONFIG_RSA and CONFIG_X509_CERTIFICATE_PARSER).

KASAN report:

    BUG: KASAN: slab-out-of-bounds in rsa_get_n+0x19e/0x1d0 crypto/rsa_helper.c:33
    Read of size 1 at addr ffff88003501a708 by task keyctl/196

    CPU: 1 PID: 196 Comm: keyctl Not tainted 4.14.0-09238-g1d3b78bbc6e9 #26
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
    Call Trace:
     rsa_get_n+0x19e/0x1d0 crypto/rsa_helper.c:33
     asn1_ber_decoder+0x82a/0x1fd0 lib/asn1_decoder.c:328
     rsa_set_pub_key+0xd3/0x320 crypto/rsa.c:278
     crypto_akcipher_set_pub_key ./include/crypto/akcipher.h:364 [inline]
     pkcs1pad_set_pub_key+0xae/0x200 crypto/rsa-pkcs1pad.c:117
     crypto_akcipher_set_pub_key ./include/crypto/akcipher.h:364 [inline]
     public_key_verify_signature+0x270/0x9d0 crypto/asymmetric_keys/public_key.c:106
     x509_check_for_self_signed+0x2ea/0x480 crypto/asymmetric_keys/x509_public_key.c:141
     x509_cert_parse+0x46a/0x620 crypto/asymmetric_keys/x509_cert_parser.c:129
     x509_key_preparse+0x61/0x750 crypto/asymmetric_keys/x509_public_key.c:174
     asymmetric_key_preparse+0xa4/0x150 crypto/asymmetric_keys/asymmetric_type.c:388
     key_create_or_update+0x4d4/0x10a0 security/keys/key.c:850
     SYSC_add_key security/keys/keyctl.c:122 [inline]
     SyS_add_key+0xe8/0x290 security/keys/keyctl.c:62
     entry_SYSCALL_64_fastpath+0x1f/0x96

    Allocated by task 196:
     __do_kmalloc mm/slab.c:3711 [inline]
     __kmalloc_track_caller+0x118/0x2e0 mm/slab.c:3726
     kmemdup+0x17/0x40 mm/util.c:118
     kmemdup ./include/linux/string.h:414 [inline]
     x509_cert_parse+0x2cb/0x620 crypto/asymmetric_keys/x509_cert_parser.c:106
     x509_key_preparse+0x61/0x750 crypto/asymmetric_keys/x509_public_key.c:174
     asymmetric_key_preparse+0xa4/0x150 crypto/asymmetric_keys/asymmetric_type.c:388
     key_create_or_update+0x4d4/0x10a0 security/keys/key.c:850
     SYSC_add_key security/keys/keyctl.c:122 [inline]
     SyS_add_key+0xe8/0x290 security/keys/keyctl.c:62
     entry_SYSCALL_64_fastpath+0x1f/0x96

Fixes 5a7de97309f5 ("crypto: rsa - return raw integers for the ASN.1 parser")

Cc <[email protected]> # v4.8+

Cc Tudor Ambarus <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/rsa_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

crypto/rsa_helper.c +1 -1

index 0b66dc824606..cad395d70d78 100644
@@ -30,7 +30,7 @@ int rsa_get_n(void *context, size_t hdrlen, unsigned char tag,
 		return -EINVAL;
 
 	if (fips_enabled) {
-		while (!*ptr && n_sz) {
+		while (n_sz && !*ptr) {
 			ptr++;
 			n_sz--;
 		}

Signature · 1 line

2.15.0

James Morris

Reviewed-by on v1 1/1

2017-11-27 08:22 UTC #1 lore raw

Quoted 13 lines · Eric Biggers

Fixes 5a7de97309f5 ("crypto: rsa - return raw integers for the ASN.1 parser")
Cc <[email protected]> # v4.8+
Cc Tudor Ambarus <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
---
 crypto/rsa_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
crypto/rsa_helper.c +0 -0
index 0b66dc824606..cad395d70d78 100644

crypto/rsa_helper.c int rsa_get_n(void *context, size_t hdrlen, unsigned char tag, · line 36

Quoted 8 lines

crypto/rsa_helper.c +1 -1

@@ -30,7 +30,7 @@ int rsa_get_n(void *context, size_t hdrlen, unsigned char tag,
 		return -EINVAL;
 
 	if (fips_enabled) {
-		while (!*ptr && n_sz) {
+		while (n_sz && !*ptr) {
 			ptr++;
 			n_sz--;
 		}

Reviewed-by James Morris <[email protected]>

Signature · 2 lines

James Morris
<[email protected]>

David Howells

maintainer

2017-11-28 10:54 UTC #2 lore raw

In reply to Eric Biggers

Hi Herbert,

Are you going to take this?

David

Herbert Xu

maintainer

2017-11-28 23:26 UTC #3 lore raw

Quoted 3 lines · David Howells

Hi Herbert,
Are you going to take this?

Yes it's in my queue.

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Howells

maintainer Reviewed-by on v1 1/1

2017-11-28 10:55 UTC #4 lore raw

In reply to Eric Biggers

Quoted 3 lines · Eric Biggers

In rsa_get_n(), if the buffer contained all 0's and "FIPS mode" is enabled, we would read one byte past the end of the buffer while scanning the leading zeroes. Fix it by checking 'n_sz' before '!*ptr'.

Reviewed-by David Howells <[email protected]>

Herbert Xu

maintainer

2017-11-29 05:22 UTC #5 lore raw

In reply to Eric Biggers

Quoted 46 lines · Eric Biggers

From: Eric Biggers <[email protected]>

This bug was reachable by adding a specially crafted key of type "asymmetric" (requires CONFIG_RSA and CONFIG_X509_CERTIFICATE_PARSER).

KASAN report:

   BUG: KASAN: slab-out-of-bounds in rsa_get_n+0x19e/0x1d0 crypto/rsa_helper.c:33
   Read of size 1 at addr ffff88003501a708 by task keyctl/196

   CPU: 1 PID: 196 Comm: keyctl Not tainted 4.14.0-09238-g1d3b78bbc6e9 #26
   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
   Call Trace:
    rsa_get_n+0x19e/0x1d0 crypto/rsa_helper.c:33
    asn1_ber_decoder+0x82a/0x1fd0 lib/asn1_decoder.c:328
    rsa_set_pub_key+0xd3/0x320 crypto/rsa.c:278
    crypto_akcipher_set_pub_key ./include/crypto/akcipher.h:364 [inline]
    pkcs1pad_set_pub_key+0xae/0x200 crypto/rsa-pkcs1pad.c:117
    crypto_akcipher_set_pub_key ./include/crypto/akcipher.h:364 [inline]
    public_key_verify_signature+0x270/0x9d0 crypto/asymmetric_keys/public_key.c:106
    x509_check_for_self_signed+0x2ea/0x480 crypto/asymmetric_keys/x509_public_key.c:141
    x509_cert_parse+0x46a/0x620 crypto/asymmetric_keys/x509_cert_parser.c:129
    x509_key_preparse+0x61/0x750 crypto/asymmetric_keys/x509_public_key.c:174
    asymmetric_key_preparse+0xa4/0x150 crypto/asymmetric_keys/asymmetric_type.c:388
    key_create_or_update+0x4d4/0x10a0 security/keys/key.c:850
    SYSC_add_key security/keys/keyctl.c:122 [inline]
    SyS_add_key+0xe8/0x290 security/keys/keyctl.c:62
    entry_SYSCALL_64_fastpath+0x1f/0x96

   Allocated by task 196:
    __do_kmalloc mm/slab.c:3711 [inline]
    __kmalloc_track_caller+0x118/0x2e0 mm/slab.c:3726
    kmemdup+0x17/0x40 mm/util.c:118
    kmemdup ./include/linux/string.h:414 [inline]
    x509_cert_parse+0x2cb/0x620 crypto/asymmetric_keys/x509_cert_parser.c:106
    x509_key_preparse+0x61/0x750 crypto/asymmetric_keys/x509_public_key.c:174
    asymmetric_key_preparse+0xa4/0x150 crypto/asymmetric_keys/asymmetric_type.c:388
    key_create_or_update+0x4d4/0x10a0 security/keys/key.c:850
    SYSC_add_key security/keys/keyctl.c:122 [inline]
    SyS_add_key+0xe8/0x290 security/keys/keyctl.c:62
    entry_SYSCALL_64_fastpath+0x1f/0x96

Quoted 4 lines

Fixes 5a7de97309f5 ("crypto: rsa - return raw integers for the ASN.1 parser")
Cc <[email protected]> # v4.8+
Cc Tudor Ambarus <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt