Hi James,
Here's a collection of fixes for Linux keyrings, mostly thanks to Eric Biggers, if you could pass them along to Linus. They include:
(1) Fix some PKCS#7 verification issues.
(2) Fix handling of unsupported crypto in X.509.
(3) Fix too-large allocation in big_key.
The patches can be found here also:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/tag/?h...
And also on the keys-fixes branch.
David --- David Howells (1): KEYS: Use individual pages in big_key for crypto buffers
Eric Biggers (5): PKCS#7: fix certificate chain verification PKCS#7: fix certificate blacklisting PKCS#7: fix direct verification of SignerInfo signature X.509: fix BUG_ON() when hash algorithm is unsupported X.509: fix NULL dereference when restricting key with unsupported_sig
crypto/asymmetric_keys/pkcs7_trust.c | 1 crypto/asymmetric_keys/pkcs7_verify.c | 12 ++-- crypto/asymmetric_keys/public_key.c | 4 + crypto/asymmetric_keys/restrict.c | 21 ++++-- security/keys/big_key.c | 110 ++++++++++++++++++++++++++------- 5 files changed, 111 insertions(+), 37 deletions(-)
From: Eric Biggers ebiggers@google.com
When pkcs7_verify_sig_chain() is building the certificate chain for a SignerInfo using the certificates in the PKCS#7 message, it is passing the wrong arguments to public_key_verify_signature(). Consequently, when the next certificate is supposed to be used to verify the previous certificate, the next certificate is actually used to verify itself.
An attacker can use this bug to create a bogus certificate chain that has no cryptographic relationship between the beginning and end.
Fortunately I couldn't quite find a way to use this to bypass the overall signature verification, though it comes very close. Here's the reasoning: due to the bug, every certificate in the chain beyond the first actually has to be self-signed (where "self-signed" here refers to the actual key and signature; an attacker might still manipulate the certificate fields such that the self_signed flag doesn't actually get set, and thus the chain doesn't end immediately). But to pass trust validation (pkcs7_validate_trust()), either the SignerInfo or one of the certificates has to actually be signed by a trusted key. Since only self-signed certificates can be added to the chain, the only way for an attacker to introduce a trusted signature is to include a self-signed trusted certificate.
But, when pkcs7_validate_trust_one() reaches that certificate, instead of trying to verify the signature on that certificate, it will actually look up the corresponding trusted key, which will succeed, and then try to verify the *previous* certificate, which will fail. Thus, disaster is narrowly averted (as far as I could tell).
Fixes: 6c2dc5ae4ab7 ("X.509: Extract signature digest and make self-signed cert checks earlier") Cc: stable@vger.kernel.org # v4.7+ Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com ---
crypto/asymmetric_keys/pkcs7_verify.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c index 39e6de0c2761..2f6a768b91d7 100644 --- a/crypto/asymmetric_keys/pkcs7_verify.c +++ b/crypto/asymmetric_keys/pkcs7_verify.c @@ -270,7 +270,7 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7, sinfo->index); return 0; } - ret = public_key_verify_signature(p->pub, p->sig); + ret = public_key_verify_signature(p->pub, x509->sig); if (ret < 0) return ret; x509->signer = p;
From: Eric Biggers ebiggers@google.com
If there is a blacklisted certificate in a SignerInfo's certificate chain, then pkcs7_verify_sig_chain() sets sinfo->blacklisted and returns 0. But, pkcs7_verify() fails to handle this case appropriately, as it actually continues on to the line 'actual_ret = 0;', indicating that the SignerInfo has passed verification. Consequently, PKCS#7 signature verification ignores the certificate blacklist.
Fix this by not considering blacklisted SignerInfos to have passed verification.
Also fix the function comment with regards to when 0 is returned.
Fixes: 03bb79315ddc ("PKCS#7: Handle blacklisted certificates") Cc: stable@vger.kernel.org # v4.12+ Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com ---
crypto/asymmetric_keys/pkcs7_verify.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c index 2f6a768b91d7..97c77f66b20d 100644 --- a/crypto/asymmetric_keys/pkcs7_verify.c +++ b/crypto/asymmetric_keys/pkcs7_verify.c @@ -366,8 +366,7 @@ static int pkcs7_verify_one(struct pkcs7_message *pkcs7, * * (*) -EBADMSG if some part of the message was invalid, or: * - * (*) 0 if no signature chains were found to be blacklisted or to contain - * unsupported crypto, or: + * (*) 0 if a signature chain passed verification, or: * * (*) -EKEYREJECTED if a blacklisted key was encountered, or: * @@ -423,8 +422,11 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) { ret = pkcs7_verify_one(pkcs7, sinfo); - if (sinfo->blacklisted && actual_ret == -ENOPKG) - actual_ret = -EKEYREJECTED; + if (sinfo->blacklisted) { + if (actual_ret == -ENOPKG) + actual_ret = -EKEYREJECTED; + continue; + } if (ret < 0) { if (ret == -ENOPKG) { sinfo->unsupported_crypto = true;
From: Eric Biggers ebiggers@google.com
If none of the certificates in a SignerInfo's certificate chain match a trusted key, nor is the last certificate signed by a trusted key, then pkcs7_validate_trust_one() tries to check whether the SignerInfo's signature was made directly by a trusted key. But, it actually fails to set the 'sig' variable correctly, so it actually verifies the last signature seen. That will only be the SignerInfo's signature if the certificate chain is empty; otherwise it will actually be the last certificate's signature.
This is not by itself a security problem, since verifying any of the certificates in the chain should be sufficient to verify the SignerInfo. Still, it's not working as intended so it should be fixed.
Fix it by setting 'sig' correctly for the direct verification case.
Fixes: 757932e6da6d ("PKCS#7: Handle PKCS#7 messages that contain no X.509 certs") Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com ---
crypto/asymmetric_keys/pkcs7_trust.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c index 1f4e25f10049..598906b1e28d 100644 --- a/crypto/asymmetric_keys/pkcs7_trust.c +++ b/crypto/asymmetric_keys/pkcs7_trust.c @@ -106,6 +106,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7, pr_devel("sinfo %u: Direct signer is key %x\n", sinfo->index, key_serial(key)); x509 = NULL; + sig = sinfo->sig; goto matched; } if (PTR_ERR(key) != -ENOKEY)
From: Eric Biggers ebiggers@google.com
The X.509 parser mishandles the case where the certificate's signature's hash algorithm is not available in the crypto API. In this case, x509_get_sig_params() doesn't allocate the cert->sig->digest buffer; this part seems to be intentional. However, public_key_verify_signature() is still called via x509_check_for_self_signed(), which triggers the 'BUG_ON(!sig->digest)'.
Fix this by making public_key_verify_signature() return -ENOPKG if the hash buffer has not been allocated.
Reproducer when all the CONFIG_CRYPTO_SHA512* options are disabled:
openssl req -new -sha512 -x509 -batch -nodes -outform der \ | keyctl padd asymmetric desc @s
Fixes: 6c2dc5ae4ab7 ("X.509: Extract signature digest and make self-signed cert checks earlier") Reported-by: Paolo Valente paolo.valente@linaro.org Cc: Paolo Valente paolo.valente@linaro.org Cc: stable@vger.kernel.org # v4.7+ Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com ---
crypto/asymmetric_keys/public_key.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index de996586762a..e929fe1e4106 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -79,9 +79,11 @@ int public_key_verify_signature(const struct public_key *pkey,
BUG_ON(!pkey); BUG_ON(!sig); - BUG_ON(!sig->digest); BUG_ON(!sig->s);
+ if (!sig->digest) + return -ENOPKG; + alg_name = sig->pkey_algo; if (strcmp(sig->pkey_algo, "rsa") == 0) { /* The data wangled by the RSA algorithm is typically padded
From: Eric Biggers ebiggers@google.com
The asymmetric key type allows an X.509 certificate to be added even if its signature's hash algorithm is not available in the crypto API. In that case 'payload.data[asym_auth]' will be NULL. But the key restriction code failed to check for this case before trying to use the signature, resulting in a NULL pointer dereference in key_or_keyring_common() or in restrict_link_by_signature().
Fix this by returning -ENOPKG when the signature is unsupported.
Reproducer when all the CONFIG_CRYPTO_SHA512* options are disabled and keyctl has support for the 'restrict_keyring' command:
keyctl new_session keyctl restrict_keyring @s asymmetric builtin_trusted openssl req -new -sha512 -x509 -batch -nodes -outform der \ | keyctl padd asymmetric desc @s
Fixes: a511e1af8b12 ("KEYS: Move the point of trust determination to __key_link()") Cc: stable@vger.kernel.org # v4.7+ Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com ---
crypto/asymmetric_keys/restrict.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c index 86fb68508952..7c93c7728454 100644 --- a/crypto/asymmetric_keys/restrict.c +++ b/crypto/asymmetric_keys/restrict.c @@ -67,8 +67,9 @@ __setup("ca_keys=", ca_keys_setup); * * Returns 0 if the new certificate was accepted, -ENOKEY if we couldn't find a * matching parent certificate in the trusted list, -EKEYREJECTED if the - * signature check fails or the key is blacklisted and some other error if - * there is a matching certificate but the signature check cannot be performed. + * signature check fails or the key is blacklisted, -ENOPKG if the signature + * uses unsupported crypto, or some other error if there is a matching + * certificate but the signature check cannot be performed. */ int restrict_link_by_signature(struct key *dest_keyring, const struct key_type *type, @@ -88,6 +89,8 @@ int restrict_link_by_signature(struct key *dest_keyring, return -EOPNOTSUPP;
sig = payload->data[asym_auth]; + if (!sig) + return -ENOPKG; if (!sig->auth_ids[0] && !sig->auth_ids[1]) return -ENOKEY;
@@ -139,6 +142,8 @@ static int key_or_keyring_common(struct key *dest_keyring, return -EOPNOTSUPP;
sig = payload->data[asym_auth]; + if (!sig) + return -ENOPKG; if (!sig->auth_ids[0] && !sig->auth_ids[1]) return -ENOKEY;
@@ -222,9 +227,9 @@ static int key_or_keyring_common(struct key *dest_keyring, * * Returns 0 if the new certificate was accepted, -ENOKEY if we * couldn't find a matching parent certificate in the trusted list, - * -EKEYREJECTED if the signature check fails, and some other error if - * there is a matching certificate but the signature check cannot be - * performed. + * -EKEYREJECTED if the signature check fails, -ENOPKG if the signature uses + * unsupported crypto, or some other error if there is a matching certificate + * but the signature check cannot be performed. */ int restrict_link_by_key_or_keyring(struct key *dest_keyring, const struct key_type *type, @@ -249,9 +254,9 @@ int restrict_link_by_key_or_keyring(struct key *dest_keyring, * * Returns 0 if the new certificate was accepted, -ENOKEY if we * couldn't find a matching parent certificate in the trusted list, - * -EKEYREJECTED if the signature check fails, and some other error if - * there is a matching certificate but the signature check cannot be - * performed. + * -EKEYREJECTED if the signature check fails, -ENOPKG if the signature uses + * unsupported crypto, or some other error if there is a matching certificate + * but the signature check cannot be performed. */ int restrict_link_by_key_or_keyring_chain(struct key *dest_keyring, const struct key_type *type,
kmalloc() can't always allocate large enough buffers for big_key to use for crypto (1MB + some metadata) so we cannot use that to allocate the buffer. Further, vmalloc'd pages can't be passed to sg_init_one() and the aead crypto accessors cannot be called progressively and must be passed all the data in one go (which means we can't pass the data in one block at a time).
Fix this by allocating the buffer pages individually and passing them through a multientry scatterlist to the crypto layer. This has the bonus advantage that we don't have to allocate a contiguous series of pages.
We then vmap() the page list and pass that through to the VFS read/write routines.
This can trigger a warning:
WARNING: CPU: 0 PID: 60912 at mm/page_alloc.c:3883 __alloc_pages_nodemask+0xb7c/0x15f8 ([<00000000002acbb6>] __alloc_pages_nodemask+0x1ee/0x15f8) [<00000000002dd356>] kmalloc_order+0x46/0x90 [<00000000002dd3e0>] kmalloc_order_trace+0x40/0x1f8 [<0000000000326a10>] __kmalloc+0x430/0x4c0 [<00000000004343e4>] big_key_preparse+0x7c/0x210 [<000000000042c040>] key_create_or_update+0x128/0x420 [<000000000042e52c>] SyS_add_key+0x124/0x220 [<00000000007bba2c>] system_call+0xc4/0x2b0
from the keyctl/padd/useradd test of the keyutils testsuite on s390x.
Note that it might be better to shovel data through in page-sized lumps instead as there's no particular need to use a monolithic buffer unless the kernel itself wants to access the data.
Fixes: 13100a72f40f ("Security: Keys: Big keys stored encrypted") Reported-by: Paul Bunyan pbunyan@redhat.com Signed-off-by: David Howells dhowells@redhat.com cc: Kirill Marinushkin k.marinushkin@gmail.com ---
security/keys/big_key.c | 110 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 87 insertions(+), 23 deletions(-)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c index 929e14978c42..4e99e8ba8612 100644 --- a/security/keys/big_key.c +++ b/security/keys/big_key.c @@ -22,6 +22,13 @@ #include <keys/big_key-type.h> #include <crypto/aead.h>
+struct big_key_buf { + unsigned int nr_pages; + void *virt; + struct scatterlist *sg; + struct page *pages[]; +}; + /* * Layout of key payload words. */ @@ -91,10 +98,9 @@ static DEFINE_MUTEX(big_key_aead_lock); /* * Encrypt/decrypt big_key data */ -static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key) +static int big_key_crypt(enum big_key_op op, struct big_key_buf *buf, size_t datalen, u8 *key) { int ret; - struct scatterlist sgio; struct aead_request *aead_req; /* We always use a zero nonce. The reason we can get away with this is * because we're using a different randomly generated key for every @@ -109,8 +115,7 @@ static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key) return -ENOMEM;
memset(zero_nonce, 0, sizeof(zero_nonce)); - sg_init_one(&sgio, data, datalen + (op == BIG_KEY_ENC ? ENC_AUTHTAG_SIZE : 0)); - aead_request_set_crypt(aead_req, &sgio, &sgio, datalen, zero_nonce); + aead_request_set_crypt(aead_req, buf->sg, buf->sg, datalen, zero_nonce); aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL); aead_request_set_ad(aead_req, 0);
@@ -130,21 +135,81 @@ static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key) }
/* + * Free up the buffer. + */ +static void big_key_free_buffer(struct big_key_buf *buf) +{ + unsigned int i; + + if (buf->virt) { + memset(buf->virt, 0, buf->nr_pages * PAGE_SIZE); + vunmap(buf->virt); + } + + for (i = 0; i < buf->nr_pages; i++) + if (buf->pages[i]) + __free_page(buf->pages[i]); + + kfree(buf); +} + +/* + * Allocate a buffer consisting of a set of pages with a virtual mapping + * applied over them. + */ +static void *big_key_alloc_buffer(size_t len) +{ + struct big_key_buf *buf; + unsigned int npg = (len + PAGE_SIZE - 1) >> PAGE_SHIFT; + unsigned int i, l; + + buf = kzalloc(sizeof(struct big_key_buf) + + sizeof(struct page) * npg + + sizeof(struct scatterlist) * npg, + GFP_KERNEL); + if (!buf) + return NULL; + + buf->nr_pages = npg; + buf->sg = (void *)(buf->pages + npg); + sg_init_table(buf->sg, npg); + + for (i = 0; i < buf->nr_pages; i++) { + buf->pages[i] = alloc_page(GFP_KERNEL); + if (!buf->pages[i]) + goto nomem; + + l = min(len, PAGE_SIZE); + sg_set_page(&buf->sg[i], buf->pages[i], l, 0); + len -= l; + } + + buf->virt = vmap(buf->pages, buf->nr_pages, VM_MAP, PAGE_KERNEL); + if (!buf->virt) + goto nomem; + + return buf; + +nomem: + big_key_free_buffer(buf); + return NULL; +} + +/* * Preparse a big key */ int big_key_preparse(struct key_preparsed_payload *prep) { + struct big_key_buf *buf; struct path *path = (struct path *)&prep->payload.data[big_key_path]; struct file *file; u8 *enckey; - u8 *data = NULL; ssize_t written; - size_t datalen = prep->datalen; + size_t datalen = prep->datalen, enclen = datalen + ENC_AUTHTAG_SIZE; int ret;
- ret = -EINVAL; if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data) - goto error; + return -EINVAL;
/* Set an arbitrary quota */ prep->quotalen = 16; @@ -157,13 +222,12 @@ int big_key_preparse(struct key_preparsed_payload *prep) * * File content is stored encrypted with randomly generated key. */ - size_t enclen = datalen + ENC_AUTHTAG_SIZE; loff_t pos = 0;
- data = kmalloc(enclen, GFP_KERNEL); - if (!data) + buf = big_key_alloc_buffer(enclen); + if (!buf) return -ENOMEM; - memcpy(data, prep->data, datalen); + memcpy(buf->virt, prep->data, datalen);
/* generate random key */ enckey = kmalloc(ENC_KEY_SIZE, GFP_KERNEL); @@ -176,7 +240,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) goto err_enckey;
/* encrypt aligned data */ - ret = big_key_crypt(BIG_KEY_ENC, data, datalen, enckey); + ret = big_key_crypt(BIG_KEY_ENC, buf, datalen, enckey); if (ret) goto err_enckey;
@@ -187,7 +251,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) goto err_enckey; }
- written = kernel_write(file, data, enclen, &pos); + written = kernel_write(file, buf->virt, enclen, &pos); if (written != enclen) { ret = written; if (written >= 0) @@ -202,7 +266,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) *path = file->f_path; path_get(path); fput(file); - kzfree(data); + big_key_free_buffer(buf); } else { /* Just store the data in a buffer */ void *data = kmalloc(datalen, GFP_KERNEL); @@ -220,7 +284,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) err_enckey: kzfree(enckey); error: - kzfree(data); + big_key_free_buffer(buf); return ret; }
@@ -298,15 +362,15 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen) return datalen;
if (datalen > BIG_KEY_FILE_THRESHOLD) { + struct big_key_buf *buf; struct path *path = (struct path *)&key->payload.data[big_key_path]; struct file *file; - u8 *data; u8 *enckey = (u8 *)key->payload.data[big_key_data]; size_t enclen = datalen + ENC_AUTHTAG_SIZE; loff_t pos = 0;
- data = kmalloc(enclen, GFP_KERNEL); - if (!data) + buf = big_key_alloc_buffer(enclen); + if (!buf) return -ENOMEM;
file = dentry_open(path, O_RDONLY, current_cred()); @@ -316,26 +380,26 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen) }
/* read file to kernel and decrypt */ - ret = kernel_read(file, data, enclen, &pos); + ret = kernel_read(file, buf->virt, enclen, &pos); if (ret >= 0 && ret != enclen) { ret = -EIO; goto err_fput; }
- ret = big_key_crypt(BIG_KEY_DEC, data, enclen, enckey); + ret = big_key_crypt(BIG_KEY_DEC, buf, enclen, enckey); if (ret) goto err_fput;
ret = datalen;
/* copy decrypted data to user */ - if (copy_to_user(buffer, data, datalen) != 0) + if (copy_to_user(buffer, buf->virt, datalen) != 0) ret = -EFAULT;
err_fput: fput(file); error: - kzfree(data); + big_key_free_buffer(buf); } else { ret = datalen; if (copy_to_user(buffer, key->payload.data[big_key_data],
David Howells dhowells@redhat.com wrote:
l = min(len, PAGE_SIZE);
Sigh. This needs to be a min_t().
David
I've posted an update that converts the min() to a min_t() and tagged it:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/tag/?h...
Do you want me to repost the patches?
David
On Thu, 22 Feb 2018, David Howells wrote:
I've posted an update that converts the min() to a min_t() and tagged it:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/tag/?h...
Do you want me to repost the patches?
Nope, I'll grab them from there.
linux-stable-mirror@lists.linaro.org
-
David Howells -
James Morris