diff mbox series

[RESEND#15] keys: Handle missing Authority Key Identifier X509 extension

Message ID 20210104164048.1378237-1-andrew.zaborowski@intel.com (mailing list archive)
State New
Headers show
Series [RESEND#15] keys: Handle missing Authority Key Identifier X509 extension | expand

Commit Message

Andrew Zaborowski Jan. 4, 2021, 4:40 p.m. UTC 
In a self-signed certificate the subject and issuer are the same and so
the Authority Key Identifier X.509 v3 extension is explicitly made
optional in RFC5280 section 4.2.1.1.
crypto/asymmetric_keys/x509_cert_parser.c can't handle this and makes
(at least) the restrict.c functions refuse to work with certificates
that don't include the AKID.  Fix this by filling in the missing
cert->sig->auth_ids with the certificate's own IDs after parsing and
determinig the certificate is self-signed.

The asymmetric_key_generate_id return value is not checked because it's
already succeeded once at this point.

There are root X.509 v3 certificates in use where this is the case,
mostly oldish ones.

Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Jarkko Sakkinen Jan. 10, 2021, 4:45 a.m. UTC | #1 
On Mon, Jan 04, 2021 at 05:40:47PM +0100, Andrew Zaborowski wrote:
> In a self-signed certificate the subject and issuer are the same and so
> the Authority Key Identifier X.509 v3 extension is explicitly made
> optional in RFC5280 section 4.2.1.1.
> crypto/asymmetric_keys/x509_cert_parser.c can't handle this and makes
> (at least) the restrict.c functions refuse to work with certificates
> that don't include the AKID.  Fix this by filling in the missing
> cert->sig->auth_ids with the certificate's own IDs after parsing and
> determinig the certificate is self-signed.
> 
> The asymmetric_key_generate_id return value is not checked because it's
> already succeeded once at this point.
> 
> There are root X.509 v3 certificates in use where this is the case,
> mostly oldish ones.
> 
> Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

I'll take active role with these.

I don't think that adding field data that does not exist is a great
idea.

I think that a more sane way to fix this issue, would be to add field
'self_signed' to 'struct public_key_signature', and remove the
equivalent field from 'struct x509_certificate'.

Minor updates are required to:

- x509_check_for_self_signed()
- pkcs7_verify_sig_chain()

Then you can use then new field to refine the check in
restrict_link_by_signature().

/Jarkko
Andrew Zaborowski Jan. 11, 2021, 2:27 p.m. UTC | #2 
Hi,

On Sun, 10 Jan 2021 at 05:45, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> On Mon, Jan 04, 2021 at 05:40:47PM +0100, Andrew Zaborowski wrote:
> > In a self-signed certificate the subject and issuer are the same and so
> > the Authority Key Identifier X.509 v3 extension is explicitly made
> > optional in RFC5280 section 4.2.1.1.
> > crypto/asymmetric_keys/x509_cert_parser.c can't handle this and makes
> > (at least) the restrict.c functions refuse to work with certificates
> > that don't include the AKID.  Fix this by filling in the missing
> > cert->sig->auth_ids with the certificate's own IDs after parsing and
> > determinig the certificate is self-signed.
> >
> > The asymmetric_key_generate_id return value is not checked because it's
> > already succeeded once at this point.
> >
> > There are root X.509 v3 certificates in use where this is the case,
> > mostly oldish ones.
> >
> > Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> I'll take active role with these.

Great!

>
> I don't think that adding field data that does not exist is a great
> idea.
>
> I think that a more sane way to fix this issue, would be to add field
> 'self_signed' to 'struct public_key_signature', and remove the
> equivalent field from 'struct x509_certificate'.

Ok, that's also an option.  I went for directly adding the
cert->sig->auth_ids because the name ".auth_ids" only implies that it
contains the signing authority's key IDs, which is something that we
have and does exist in the input file.  The name doesn't imply that it
should specifically be in the AKID extension.  Similarly
x509_key_preparse even generates a key description even though a
description is not part of the x509 format.

>
> Minor updates are required to:
>
> - x509_check_for_self_signed()
> - pkcs7_verify_sig_chain()
>
> Then you can use then new field to refine the check in
> restrict_link_by_signature().

Ok.

Best regards
Jarkko Sakkinen Jan. 12, 2021, 1:40 a.m. UTC | #3 
On Mon, Jan 11, 2021 at 03:27:57PM +0100, Andrew Zaborowski wrote:
> Hi,
> 
> On Sun, 10 Jan 2021 at 05:45, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > On Mon, Jan 04, 2021 at 05:40:47PM +0100, Andrew Zaborowski wrote:
> > > In a self-signed certificate the subject and issuer are the same and so
> > > the Authority Key Identifier X.509 v3 extension is explicitly made
> > > optional in RFC5280 section 4.2.1.1.
> > > crypto/asymmetric_keys/x509_cert_parser.c can't handle this and makes
> > > (at least) the restrict.c functions refuse to work with certificates
> > > that don't include the AKID.  Fix this by filling in the missing
> > > cert->sig->auth_ids with the certificate's own IDs after parsing and
> > > determinig the certificate is self-signed.
> > >
> > > The asymmetric_key_generate_id return value is not checked because it's
> > > already succeeded once at this point.
> > >
> > > There are root X.509 v3 certificates in use where this is the case,
> > > mostly oldish ones.
> > >
> > > Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
> > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > I'll take active role with these.
> 
> Great!
> 
> >
> > I don't think that adding field data that does not exist is a great
> > idea.
> >
> > I think that a more sane way to fix this issue, would be to add field
> > 'self_signed' to 'struct public_key_signature', and remove the
> > equivalent field from 'struct x509_certificate'.
> 
> Ok, that's also an option.  I went for directly adding the
> cert->sig->auth_ids because the name ".auth_ids" only implies that it
> contains the signing authority's key IDs, which is something that we
> have and does exist in the input file.  The name doesn't imply that it
> should specifically be in the AKID extension.  Similarly
> x509_key_preparse even generates a key description even though a
> description is not part of the x509 format.
> 
> >
> > Minor updates are required to:
> >
> > - x509_check_for_self_signed()
> > - pkcs7_verify_sig_chain()
> >
> > Then you can use then new field to refine the check in
> > restrict_link_by_signature().
> 
> Ok.

I mean in common sense: kernel stores the information needed.
It just needs a bit of relocation.

/Jarkko
Andrew Zaborowski Jan. 13, 2021, 2:31 p.m. UTC | #4 
On Tue, 12 Jan 2021 at 02:41, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> On Mon, Jan 11, 2021 at 03:27:57PM +0100, Andrew Zaborowski wrote:
> > On Sun, 10 Jan 2021 at 05:45, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > I think that a more sane way to fix this issue, would be to add field
> > > 'self_signed' to 'struct public_key_signature', and remove the
> > > equivalent field from 'struct x509_certificate'.
> >
> > Ok, that's also an option.  I went for directly adding the
> > cert->sig->auth_ids because the name ".auth_ids" only implies that it
> > contains the signing authority's key IDs, which is something that we
> > have and does exist in the input file.  The name doesn't imply that it
> > should specifically be in the AKID extension.  Similarly
> > x509_key_preparse even generates a key description even though a
> > description is not part of the x509 format.
> >
> > >
> > > Minor updates are required to:
> > >
> > > - x509_check_for_self_signed()
> > > - pkcs7_verify_sig_chain()
> > >
> > > Then you can use then new field to refine the check in
> > > restrict_link_by_signature().

I decided to try supporting key lookups without the AKID extension
first, as I proposed in
https://marc.info/?l=linux-keyrings&m=158621913606374&w=2 , because
this would make the new sig->self_signed field obsolete.  It's a just
slightly bigger change but fixes verification for a bigger set of
certificates that are in use.

If there are arguments against it then I'm going to send a patch to
add a sig->self_signed as you suggested.

Many thanks
Jarkko Sakkinen Jan. 14, 2021, 3:19 a.m. UTC | #5 
On Wed, Jan 13, 2021 at 03:31:04PM +0100, Andrew Zaborowski wrote:
> On Tue, 12 Jan 2021 at 02:41, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > On Mon, Jan 11, 2021 at 03:27:57PM +0100, Andrew Zaborowski wrote:
> > > On Sun, 10 Jan 2021 at 05:45, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > I think that a more sane way to fix this issue, would be to add field
> > > > 'self_signed' to 'struct public_key_signature', and remove the
> > > > equivalent field from 'struct x509_certificate'.
> > >
> > > Ok, that's also an option.  I went for directly adding the
> > > cert->sig->auth_ids because the name ".auth_ids" only implies that it
> > > contains the signing authority's key IDs, which is something that we
> > > have and does exist in the input file.  The name doesn't imply that it
> > > should specifically be in the AKID extension.  Similarly
> > > x509_key_preparse even generates a key description even though a
> > > description is not part of the x509 format.
> > >
> > > >
> > > > Minor updates are required to:
> > > >
> > > > - x509_check_for_self_signed()
> > > > - pkcs7_verify_sig_chain()
> > > >
> > > > Then you can use then new field to refine the check in
> > > > restrict_link_by_signature().
> 
> I decided to try supporting key lookups without the AKID extension
> first, as I proposed in
> https://marc.info/?l=linux-keyrings&m=158621913606374&w=2 , because
> this would make the new sig->self_signed field obsolete.  It's a just
> slightly bigger change but fixes verification for a bigger set of
> certificates that are in use.
> 
> If there are arguments against it then I'm going to send a patch to
> add a sig->self_signed as you suggested.
> 
> Many thanks

OK, I'll check that one then. Thank you.

/Jarkko
diff mbox series

Patch

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 26ec20ef489..a5a2f93e242 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -136,6 +136,25 @@  struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
 	if (ret < 0)
 		goto error_decode;
 
+	if (cert->self_signed) {
+		if (!cert->sig->auth_ids[0]) {
+			/* Duplicate cert->id */
+			kid = asymmetric_key_generate_id(cert->raw_serial,
+							 cert->raw_serial_size,
+							 cert->raw_issuer,
+							 cert->raw_issuer_size);
+			cert->sig->auth_ids[0] = kid;
+		}
+
+		if (!cert->sig->auth_ids[1] && cert->skid) {
+			/* Duplicate cert->skid */
+			kid = asymmetric_key_generate_id(cert->raw_skid,
+							 cert->raw_skid_size,
+							 "", 0);
+			cert->sig->auth_ids[1] = kid;
+		}
+	}
+
 	kfree(ctx);
 	return cert;