diff mbox series

[09/12] PCI/CMA: Validate Subject Alternative Name in certificates

Message ID bc1efd945f5d76587787f8351199e1ea45eaf2ef.1695921657.git.lukas@wunner.de (mailing list archive)
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series PCI device authentication | expand

Commit Message

Lukas Wunner Sept. 28, 2023, 5:32 p.m. UTC
PCIe r6.1 sec 6.31.3 stipulates requirements for X.509 Leaf Certificates
presented by devices, in particular the presence of a Subject Alternative
Name extension with a name that encodes the Vendor ID, Device ID, Device
Serial Number, etc.

This prevents a mismatch between the device identity in Config Space and
the certificate.  A device cannot misappropriate a certificate from a
different device without also spoofing Config Space.  As a corollary,
it cannot dupe an arbitrary driver into binding to it.  (Only those
which bind to the device identity in the Subject Alternative Name work.)

Parse the Subject Alternative Name using a small ASN.1 module and
validate its contents.  The theory of operation is explained in a code
comment at the top of the newly added cma-x509.c.

This functionality is introduced in a separate commit on top of basic
CMA-SPDM support to split the code into digestible, reviewable chunks.

The CMA OID added here is taken from the official OID Repository
(it's not documented in the PCIe Base Spec):
https://oid-rep.orange-labs.fr/get/2.23.147

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/Makefile         |   4 +-
 drivers/pci/cma-x509.c       | 119 +++++++++++++++++++++++++++++++++++
 drivers/pci/cma.asn1         |  36 +++++++++++
 drivers/pci/cma.c            |   3 +-
 drivers/pci/pci.h            |   2 +
 include/linux/oid_registry.h |   3 +
 include/linux/spdm.h         |   6 +-
 lib/spdm_requester.c         |  14 ++++-
 8 files changed, 183 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pci/cma-x509.c
 create mode 100644 drivers/pci/cma.asn1

Comments

Jonathan Cameron Oct. 3, 2023, 3:04 p.m. UTC | #1
On Thu, 28 Sep 2023 19:32:39 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> PCIe r6.1 sec 6.31.3 stipulates requirements for X.509 Leaf Certificates
> presented by devices, in particular the presence of a Subject Alternative
> Name extension with a name that encodes the Vendor ID, Device ID, Device
> Serial Number, etc.

Lets you do any of
* What you have here
* Reference Integrity Manifest, e.g. see Trusted Computing Group
* A pointer to a location where such a Reference Integrity Manifest can be
  obtained.

So this text feels a little strong though I'm fine with only support the
Subject Alternative Name bit for now. Whoever has one of the other options
can add that support :)

> 
> This prevents a mismatch between the device identity in Config Space and
> the certificate.  A device cannot misappropriate a certificate from a
> different device without also spoofing Config Space.  As a corollary,
> it cannot dupe an arbitrary driver into binding to it.  (Only those
> which bind to the device identity in the Subject Alternative Name work.)
> 
> Parse the Subject Alternative Name using a small ASN.1 module and
> validate its contents.  The theory of operation is explained in a code
> comment at the top of the newly added cma-x509.c.
> 
> This functionality is introduced in a separate commit on top of basic
> CMA-SPDM support to split the code into digestible, reviewable chunks.
> 
> The CMA OID added here is taken from the official OID Repository
> (it's not documented in the PCIe Base Spec):
> https://oid-rep.orange-labs.fr/get/2.23.147
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

I haven't looked asn.1 recently enough to have any confidence on
a review of that bit...
So, for everything except the asn.1
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Lukas Wunner Oct. 5, 2023, 2:04 p.m. UTC | #2
On Tue, Oct 03, 2023 at 04:04:55PM +0100, Jonathan Cameron wrote:
> On Thu, 28 Sep 2023 19:32:39 +0200 Lukas Wunner <lukas@wunner.de> wrote:
> > PCIe r6.1 sec 6.31.3 stipulates requirements for X.509 Leaf Certificates
> > presented by devices, in particular the presence of a Subject Alternative
> > Name extension with a name that encodes the Vendor ID, Device ID, Device
> > Serial Number, etc.
> 
> Lets you do any of
> * What you have here
> * Reference Integrity Manifest, e.g. see Trusted Computing Group
> * A pointer to a location where such a Reference Integrity Manifest can be
>   obtained.
> 
> So this text feels a little strong though I'm fine with only support the
> Subject Alternative Name bit for now. Whoever has one of the other options
> can add that support :)

I intend to amend the commit message as follows.  If anyone believes
this is inaccurate, please let me know:

    Side note:  Instead of a Subject Alternative Name, Leaf Certificates may
    include "a Reference Integrity Manifest, e.g., see Trusted Computing
    Group" or "a pointer to a location where such a Reference Integrity
    Manifest can be obtained" (PCIe r6.1 sec 6.31.3).

    A Reference Integrity Manifest contains "golden" measurements which can
    be compared to actual measurements retrieved from a device.  It serves a
    different purpose than the Subject Alternative Name, hence it is unclear
    why the spec says only either of them is necessary.  It is also unclear
    how a Reference Integrity Manifest shall be encoded into a certificate.

    Ignore the Reference Integrity Manifest requirement until this confusion
    is resolved by a spec update.


> I haven't looked asn.1 recently enough to have any confidence on
> a review of that bit...
> So, for everything except the asn.1
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

In case it raises the confidence in that portion of the patch,
I have tested it successfully not just with certificates containing
a single CMA otherName, but also:

- a single otherName with a different OID
- multiple otherNames with a mix of CMA and other OIDs
- multiple otherNames plus additional unrelated dNSNames
- no Subject Alternative Name

Getting the IMPLICIT annotations right was a bit nontrivial.
It turned out that the existing crypto/asymmetric_keys/x509_akid.asn1
got that wrong as well, so I fixed it up as a byproduct of this series:

https://git.kernel.org/herbert/cryptodev-2.6/c/a1e452026e6d

The debug experience made me appreciate the kernel's ASN.1 compiler
and parser though:  Their code is surprisingly small, the generated
output of the compiler is quite readable and the split architecture
with a compiler+parser feels much safer than what openssl does.

Thanks,

Lukas
Bjorn Helgaas Oct. 5, 2023, 8:09 p.m. UTC | #3
On Thu, Oct 05, 2023 at 04:04:47PM +0200, Lukas Wunner wrote:
> On Tue, Oct 03, 2023 at 04:04:55PM +0100, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2023 19:32:39 +0200 Lukas Wunner <lukas@wunner.de> wrote:
> > > PCIe r6.1 sec 6.31.3 stipulates requirements for X.509 Leaf Certificates

The PCIe spec does not contain "X.509", so I assume this is sort of a
transitive requirement from SPDM.

> > > presented by devices, in particular the presence of a Subject Alternative
> > > Name extension with a name that encodes the Vendor ID, Device ID, Device
> > > Serial Number, etc.
> > 
> > Lets you do any of
> > * What you have here
> > * Reference Integrity Manifest, e.g. see Trusted Computing Group
> > * A pointer to a location where such a Reference Integrity Manifest can be
> >   obtained.
> > 
> > So this text feels a little strong though I'm fine with only support the
> > Subject Alternative Name bit for now. Whoever has one of the other options
> > can add that support :)
> 
> I intend to amend the commit message as follows.  If anyone believes
> this is inaccurate, please let me know:
> 
>     Side note:  Instead of a Subject Alternative Name, Leaf Certificates may
>     include "a Reference Integrity Manifest, e.g., see Trusted Computing
>     Group" or "a pointer to a location where such a Reference Integrity
>     Manifest can be obtained" (PCIe r6.1 sec 6.31.3).
> 
>     A Reference Integrity Manifest contains "golden" measurements which can
>     be compared to actual measurements retrieved from a device.  It serves a
>     different purpose than the Subject Alternative Name, hence it is unclear
>     why the spec says only either of them is necessary.  It is also unclear
>     how a Reference Integrity Manifest shall be encoded into a certificate.
> 
>     Ignore the Reference Integrity Manifest requirement until this confusion
>     is resolved by a spec update.

Thanks for this; I was about to comment the same.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index e0705b82690b..a18812b8832b 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -34,7 +34,9 @@  obj-$(CONFIG_VGA_ARB)		+= vgaarb.o
 obj-$(CONFIG_PCI_DOE)		+= doe.o
 obj-$(CONFIG_PCI_DYNAMIC_OF_NODES) += of_property.o
 
-obj-$(CONFIG_PCI_CMA)		+= cma.o
+obj-$(CONFIG_PCI_CMA)		+= cma.o cma-x509.o cma.asn1.o
+$(obj)/cma-x509.o:		$(obj)/cma.asn1.h
+$(obj)/cma.asn1.o:		$(obj)/cma.asn1.c $(obj)/cma.asn1.h
 
 # Endpoint library must be initialized before its users
 obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
diff --git a/drivers/pci/cma-x509.c b/drivers/pci/cma-x509.c
new file mode 100644
index 000000000000..614590303b38
--- /dev/null
+++ b/drivers/pci/cma-x509.c
@@ -0,0 +1,119 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Component Measurement and Authentication (CMA-SPDM, PCIe r6.1 sec 6.31)
+ *
+ * The spdm_requester.c library calls pci_cma_validate() to check requirements
+ * for X.509 Leaf Certificates per PCIe r6.1 sec 6.31.3.
+ *
+ * It parses the Subject Alternative Name using the ASN.1 module cma.asn1,
+ * which calls pci_cma_note_oid() and pci_cma_note_san() to compare an
+ * OtherName against the expected name.
+ *
+ * The expected name is constructed beforehand by pci_cma_construct_san().
+ *
+ * Copyright (C) 2023 Intel Corporation
+ */
+
+#define dev_fmt(fmt) "CMA: " fmt
+
+#include <keys/x509-parser.h>
+#include <linux/asn1_decoder.h>
+#include <linux/oid_registry.h>
+#include <linux/pci.h>
+
+#include "cma.asn1.h"
+#include "pci.h"
+
+#define CMA_NAME_MAX sizeof("Vendor=1234:Device=1234:CC=123456:"	  \
+			    "REV=12:SSVID=1234:SSID=1234:1234567890123456")
+
+struct pci_cma_x509_context {
+	struct pci_dev *pdev;
+	enum OID last_oid;
+	char expected_name[CMA_NAME_MAX];
+	unsigned int expected_len;
+	unsigned int found:1;
+};
+
+int pci_cma_note_oid(void *context, size_t hdrlen, unsigned char tag,
+		     const void *value, size_t vlen)
+{
+	struct pci_cma_x509_context *ctx = context;
+
+	ctx->last_oid = look_up_OID(value, vlen);
+
+	return 0;
+}
+
+int pci_cma_note_san(void *context, size_t hdrlen, unsigned char tag,
+		     const void *value, size_t vlen)
+{
+	struct pci_cma_x509_context *ctx = context;
+
+	/* These aren't the drOIDs we're looking for. */
+	if (ctx->last_oid != OID_CMA)
+		return 0;
+
+	if (tag != ASN1_UTF8STR ||
+	    vlen != ctx->expected_len ||
+	    memcmp(value, ctx->expected_name, vlen) != 0) {
+		pci_err(ctx->pdev, "Invalid X.509 Subject Alternative Name\n");
+		return -EINVAL;
+	}
+
+	ctx->found = true;
+
+	return 0;
+}
+
+static unsigned int pci_cma_construct_san(struct pci_dev *pdev, char *name)
+{
+	unsigned int len;
+	u64 serial;
+
+	len = snprintf(name, CMA_NAME_MAX,
+		       "Vendor=%04hx:Device=%04hx:CC=%06x:REV=%02hhx",
+		       pdev->vendor, pdev->device, pdev->class, pdev->revision);
+
+	if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL)
+		len += snprintf(name + len, CMA_NAME_MAX - len,
+				":SSVID=%04hx:SSID=%04hx",
+				pdev->subsystem_vendor, pdev->subsystem_device);
+
+	serial = pci_get_dsn(pdev);
+	if (serial)
+		len += snprintf(name + len, CMA_NAME_MAX - len,
+				":%016llx", serial);
+
+	return len;
+}
+
+int pci_cma_validate(struct device *dev, struct x509_certificate *leaf_cert)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct pci_cma_x509_context ctx;
+	int ret;
+
+	if (!leaf_cert->raw_san) {
+		pci_err(pdev, "Missing X.509 Subject Alternative Name\n");
+		return -EINVAL;
+	}
+
+	ctx.pdev = pdev;
+	ctx.found = false;
+	ctx.expected_len = pci_cma_construct_san(pdev, ctx.expected_name);
+
+	ret = asn1_ber_decoder(&cma_decoder, &ctx, leaf_cert->raw_san,
+			       leaf_cert->raw_san_size);
+	if (ret == -EBADMSG || ret == -EMSGSIZE)
+		pci_err(pdev, "Malformed X.509 Subject Alternative Name\n");
+	if (ret < 0)
+		return ret;
+
+	if (!ctx.found) {
+		pci_err(pdev, "Missing X.509 OtherName with CMA OID\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/pci/cma.asn1 b/drivers/pci/cma.asn1
new file mode 100644
index 000000000000..10f90e107009
--- /dev/null
+++ b/drivers/pci/cma.asn1
@@ -0,0 +1,36 @@ 
+-- Component Measurement and Authentication (CMA-SPDM, PCIe r6.1 sec 6.31.3)
+-- X.509 Subject Alternative Name (RFC 5280 sec 4.2.1.6)
+--
+-- https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.6
+--
+-- The ASN.1 module in RFC 5280 appendix A.1 uses EXPLICIT TAGS whereas the one
+-- in appendix A.2 uses IMPLICIT TAGS.  The kernel's simplified asn1_compiler.c
+-- always uses EXPLICIT TAGS, hence this ASN.1 module differs from RFC 5280 in
+-- that it adds IMPLICIT to definitions from appendix A.2 (such as OtherName)
+-- and omits EXPLICIT in those definitions.
+
+SubjectAltName ::= GeneralNames
+
+GeneralNames ::= SEQUENCE OF GeneralName
+
+GeneralName ::= CHOICE {
+	otherName			[0] IMPLICIT OtherName,
+	rfc822Name			[1] IMPLICIT IA5String,
+	dNSName				[2] IMPLICIT IA5String,
+	x400Address			[3] ANY,
+	directoryName			[4] ANY,
+	ediPartyName			[5] IMPLICIT EDIPartyName,
+	uniformResourceIdentifier	[6] IMPLICIT IA5String,
+	iPAddress			[7] IMPLICIT OCTET STRING,
+	registeredID			[8] IMPLICIT OBJECT IDENTIFIER
+	}
+
+OtherName ::= SEQUENCE {
+	type-id			OBJECT IDENTIFIER ({ pci_cma_note_oid }),
+	value			[0] ANY ({ pci_cma_note_san })
+	}
+
+EDIPartyName ::= SEQUENCE {
+	nameAssigner		[0] ANY OPTIONAL,
+	partyName		[1] ANY
+	}
diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
index 06e5846325e3..012190c54ab6 100644
--- a/drivers/pci/cma.c
+++ b/drivers/pci/cma.c
@@ -64,7 +64,8 @@  void pci_cma_init(struct pci_dev *pdev)
 		return;
 
 	pdev->spdm_state = spdm_create(&pdev->dev, pci_doe_transport, doe,
-				       PCI_DOE_MAX_PAYLOAD, pci_cma_keyring);
+				       PCI_DOE_MAX_PAYLOAD, pci_cma_keyring,
+				       pci_cma_validate);
 	if (!pdev->spdm_state) {
 		return;
 	}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index bd80a0369c9c..6c4755a2c91c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -325,6 +325,8 @@  static inline void pci_doe_disconnected(struct pci_dev *pdev) { }
 #ifdef CONFIG_PCI_CMA
 void pci_cma_init(struct pci_dev *pdev);
 void pci_cma_destroy(struct pci_dev *pdev);
+struct x509_certificate;
+int pci_cma_validate(struct device *dev, struct x509_certificate *leaf_cert);
 #else
 static inline void pci_cma_init(struct pci_dev *pdev) { }
 static inline void pci_cma_destroy(struct pci_dev *pdev) { }
diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h
index f86a08ba0207..cafec7111473 100644
--- a/include/linux/oid_registry.h
+++ b/include/linux/oid_registry.h
@@ -141,6 +141,9 @@  enum OID {
 	OID_TPMImportableKey,		/* 2.23.133.10.1.4 */
 	OID_TPMSealedData,		/* 2.23.133.10.1.5 */
 
+	/* PCI */
+	OID_CMA,			/* 2.23.147 */
+
 	OID__NR
 };
 
diff --git a/include/linux/spdm.h b/include/linux/spdm.h
index e824063793a7..69a83bc2eb41 100644
--- a/include/linux/spdm.h
+++ b/include/linux/spdm.h
@@ -17,14 +17,18 @@ 
 struct key;
 struct device;
 struct spdm_state;
+struct x509_certificate;
 
 typedef int (spdm_transport)(void *priv, struct device *dev,
 			     const void *request, size_t request_sz,
 			     void *response, size_t response_sz);
 
+typedef int (spdm_validate)(struct device *dev,
+			    struct x509_certificate *leaf_cert);
+
 struct spdm_state *spdm_create(struct device *dev, spdm_transport *transport,
 			       void *transport_priv, u32 transport_sz,
-			       struct key *keyring);
+			       struct key *keyring, spdm_validate *validate);
 
 int spdm_authenticate(struct spdm_state *spdm_state);
 
diff --git a/lib/spdm_requester.c b/lib/spdm_requester.c
index 407041036599..b2af2074ba6f 100644
--- a/lib/spdm_requester.c
+++ b/lib/spdm_requester.c
@@ -489,6 +489,7 @@  static int spdm_err(struct device *dev, struct spdm_error_rsp *rsp)
  *	responder's signatures.
  * @root_keyring: Keyring against which to check the first certificate in
  *	responder's certificate chain.
+ * @validate: Function to validate additional leaf certificate requirements.
  */
 struct spdm_state {
 	struct mutex lock;
@@ -520,6 +521,7 @@  struct spdm_state {
 	/* Certificates */
 	struct public_key *leaf_key;
 	struct key *root_keyring;
+	spdm_validate *validate;
 };
 
 static int __spdm_exchange(struct spdm_state *spdm_state,
@@ -1003,6 +1005,13 @@  static int spdm_validate_cert_chain(struct spdm_state *spdm_state, u8 slot,
 	}
 
 	prev = NULL;
+
+	if (spdm_state->validate) {
+		rc = spdm_state->validate(spdm_state->dev, cert);
+		if (rc)
+			goto err_free_cert;
+	}
+
 	spdm_state->leaf_key = cert->pub;
 	cert->pub = NULL;
 
@@ -1447,12 +1456,14 @@  EXPORT_SYMBOL_GPL(spdm_authenticated);
  * @transport_priv: Transport private data
  * @transport_sz: Maximum message size the transport is capable of (in bytes)
  * @keyring: Trusted root certificates
+ * @validate: Function to validate additional leaf certificate requirements
+ *	(optional, may be %NULL)
  *
  * Returns a pointer to the allocated SPDM session state or NULL on error.
  */
 struct spdm_state *spdm_create(struct device *dev, spdm_transport *transport,
 			       void *transport_priv, u32 transport_sz,
-			       struct key *keyring)
+			       struct key *keyring, spdm_validate *validate)
 {
 	struct spdm_state *spdm_state = kzalloc(sizeof(*spdm_state), GFP_KERNEL);
 
@@ -1464,6 +1475,7 @@  struct spdm_state *spdm_create(struct device *dev, spdm_transport *transport,
 	spdm_state->transport_priv = transport_priv;
 	spdm_state->transport_sz = transport_sz;
 	spdm_state->root_keyring = keyring;
+	spdm_state->validate = validate;
 
 	mutex_init(&spdm_state->lock);