* [PATCH 1/5] cert: Add cert.c for certificate types and routines
@ 2018-11-08 12:06 Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code Andrew Zaborowski
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 12:06 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 12776 bytes --]
Create a new file for all the generic X.509 certificate utilities so far
living in tls.c and make the l_cert type a public object. There are
only minor changes from the tls.c code to add accessors, otherwise the
intention is not to make the code perfect and only move the existing
code so that for example pem.c can start returning l_cert objects
directly.
---
Makefile.am | 6 +-
ell/cert.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++++
ell/cert.h | 57 ++++++++++
ell/ell.h | 1 +
ell/ell.sym | 8 ++
5 files changed, 376 insertions(+), 2 deletions(-)
create mode 100644 ell/cert.c
create mode 100644 ell/cert.h
diff --git a/Makefile.am b/Makefile.am
index 0a8d0f6..a67f0a5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -48,7 +48,8 @@ pkginclude_HEADERS = ell/ell.h \
ell/file.h \
ell/dir.h \
ell/net.h \
- ell/dhcp.h
+ ell/dhcp.h \
+ ell/cert.h
lib_LTLIBRARIES = ell/libell.la
@@ -109,7 +110,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
ell/dhcp-private.h \
ell/dhcp.c \
ell/dhcp-transport.c \
- ell/dhcp-lease.c
+ ell/dhcp-lease.c \
+ ell/cert.c
ell_libell_la_LDFLAGS = -no-undefined \
-Wl,--version-script=$(top_srcdir)/ell/ell.sym \
diff --git a/ell/cert.c b/ell/cert.c
new file mode 100644
index 0000000..82ea005
--- /dev/null
+++ b/ell/cert.c
@@ -0,0 +1,306 @@
+/*
+ * Embedded Linux library
+ *
+ * Copyright (C) 2018 Intel Corporation. All rights reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+
+#include "private.h"
+#include "key.h"
+#include "asn1-private.h"
+#include "cert.h"
+
+#define X509_CERTIFICATE_POS 0
+#define X509_TBSCERTIFICATE_POS 0
+#define X509_TBSCERT_VERSION_POS 0
+#define X509_TBSCERT_SERIAL_POS 1
+#define X509_TBSCERT_SIGNATURE_POS 2
+#define X509_ALGORITHM_ID_ALGORITHM_POS 0
+#define X509_ALGORITHM_ID_PARAMS_POS 1
+#define X509_TBSCERT_ISSUER_DN_POS 3
+#define X509_TBSCERT_VALIDITY_POS 4
+#define X509_TBSCERT_SUBJECT_DN_POS 5
+#define X509_TBSCERT_SUBJECT_KEY_POS 6
+#define X509_SUBJECT_KEY_ALGORITHM_POS 0
+#define X509_SUBJECT_KEY_VALUE_POS 1
+#define X509_TBSCERT_ISSUER_UID_POS 7
+#define X509_TBSCERT_SUBJECT_UID_POS 8
+#define X509_TBSCERT_EXTENSIONS_POS 9
+#define X509_SIGNATURE_ALGORITHM_POS 1
+#define X509_SIGNATURE_VALUE_POS 2
+
+struct l_cert {
+ struct l_cert *issuer;
+ struct l_cert *issued;
+ enum l_cert_key_type pubkey_type;
+ size_t asn1_len;
+ uint8_t asn1[0];
+};
+
+static const struct pkcs1_encryption_oid {
+ enum l_cert_key_type key_type;
+ struct asn1_oid oid;
+} pkcs1_encryption_oids[] = {
+ { /* rsaEncryption */
+ L_CERT_KEY_RSA,
+ { 9, { 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01 } },
+ },
+};
+
+static bool cert_set_pubkey_type(struct l_cert *cert)
+{
+ const uint8_t *key_type;
+ size_t key_type_len;
+ int i;
+
+ key_type = asn1_der_find_elem_by_path(cert->asn1, cert->asn1_len,
+ ASN1_ID_OID, &key_type_len,
+ X509_CERTIFICATE_POS,
+ X509_TBSCERTIFICATE_POS,
+ X509_TBSCERT_SUBJECT_KEY_POS,
+ X509_SUBJECT_KEY_ALGORITHM_POS,
+ X509_ALGORITHM_ID_ALGORITHM_POS,
+ -1);
+ if (!key_type)
+ return false;
+
+ for (i = 0; i < (int) L_ARRAY_SIZE(pkcs1_encryption_oids); i++)
+ if (asn1_oid_eq(&pkcs1_encryption_oids[i].oid,
+ key_type_len, key_type))
+ break;
+
+ if (i == L_ARRAY_SIZE(pkcs1_encryption_oids))
+ cert->pubkey_type = L_CERT_KEY_UNKNOWN;
+ else
+ cert->pubkey_type = pkcs1_encryption_oids[i].key_type;
+
+ return true;
+}
+
+LIB_EXPORT struct l_cert *l_cert_new_from_der(const uint8_t *buf,
+ size_t buf_len)
+{
+ const uint8_t *seq = buf;
+ size_t seq_len = buf_len;
+ size_t content_len;
+ struct l_cert *cert;
+
+ /* Sanity check: outer element is a SEQUENCE */
+ if (seq_len-- < 1 || *seq++ != ASN1_ID_SEQUENCE)
+ return NULL;
+
+ /* Sanity check: the SEQUENCE spans the whole buffer */
+ content_len = asn1_parse_definite_length(&seq, &seq_len);
+ if (content_len < 64 || content_len != seq_len)
+ return NULL;
+
+ /*
+ * We could require the signature algorithm and the key algorithm
+ * to be one of our supported types here but instead we only
+ * require that when the user wants to verify this certificate or
+ * get the public key respectively.
+ */
+
+ cert = l_malloc(sizeof(struct l_cert) + buf_len);
+ cert->issuer = NULL;
+ cert->issued = NULL;
+ cert->asn1_len = buf_len;
+ memcpy(cert->asn1, buf, buf_len);
+
+ /* Sanity check: structure is correct up to the Public Key Algorithm */
+ if (!cert_set_pubkey_type(cert)) {
+ l_free(cert);
+ return NULL;
+ }
+
+ return cert;
+}
+
+LIB_EXPORT void l_cert_free(struct l_cert *cert)
+{
+ while (cert) {
+ struct l_cert *next = cert->issuer;
+
+ l_free(cert);
+ cert = next;
+ }
+}
+
+LIB_EXPORT bool l_cert_link_issuer(struct l_cert *cert, struct l_cert *issuer)
+{
+ if (!cert)
+ return false;
+
+ if (issuer && (issuer->issued || cert->issuer))
+ return false;
+
+ if (cert->issuer)
+ cert->issuer->issued = NULL;
+
+ cert->issuer = issuer;
+ return true;
+}
+
+LIB_EXPORT struct l_cert *l_cert_get_issuer(struct l_cert *cert)
+{
+ return cert->issuer;
+}
+
+LIB_EXPORT const uint8_t *l_cert_get_der_data(struct l_cert *cert,
+ size_t *out_len)
+{
+ *out_len = cert->asn1_len;
+ return cert->asn1;
+}
+
+LIB_EXPORT bool l_cert_find_certchain(struct l_cert *cert,
+ struct l_cert *ca_cert)
+{
+ return true;
+}
+
+static void cert_key_cleanup(struct l_key **p)
+{
+ l_key_free_norevoke(*p);
+}
+
+static bool cert_verify_with_keyring(struct l_cert *cert,
+ struct l_keyring *ring,
+ struct l_cert *root,
+ struct l_keyring *trusted)
+{
+ if (!cert)
+ return true;
+
+ if (cert->pubkey_type != L_CERT_KEY_RSA)
+ return false;
+
+ /*
+ * RFC5246 7.4.2:
+ * "Because certificate validation requires that root keys be
+ * distributed independently, the self-signed certificate that
+ * specifies the root certificate authority MAY be omitted from
+ * the chain, under the assumption that the remote end must
+ * already possess it in order to validate it in any case."
+ */
+ if (!cert->issuer && root && cert->asn1_len == root->asn1_len &&
+ !memcmp(cert->asn1, root->asn1, root->asn1_len))
+ return true;
+
+ if (cert_verify_with_keyring(cert->issuer, ring, root, trusted)) {
+ L_AUTO_CLEANUP_VAR(struct l_key *, key, cert_key_cleanup);
+
+ key = l_key_new(L_KEY_RSA, cert->asn1, cert->asn1_len);
+ if (!key)
+ return false;
+
+ if (!l_keyring_link(ring, key))
+ return false;
+
+ if (trusted || cert->issuer)
+ return true;
+
+ /*
+ * If execution reaches this point, it's known that:
+ * * No trusted root key was supplied, so the chain is only
+ * being checked against its own root
+ * * The keyring 'ring' is not restricted yet
+ * * The chain's root cert was just linked in to the
+ * previously empty keyring 'ring'.
+ *
+ * By restricting 'ring' now, the rest of the certs in
+ * the chain will have their signature validated using 'key'
+ * as the root.
+ */
+ return l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN,
+ trusted);
+ }
+
+ return false;
+}
+
+static void cert_keyring_cleanup(struct l_keyring **p)
+{
+ l_keyring_free(*p);
+}
+
+LIB_EXPORT bool l_cert_verify_certchain(struct l_cert *certchain,
+ struct l_cert *ca_cert)
+{
+ L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring,
+ cert_keyring_cleanup) = NULL;
+ L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
+ cert_keyring_cleanup) = NULL;
+
+ if (!certchain || certchain->pubkey_type != L_CERT_KEY_RSA)
+ return false;
+
+ if (ca_cert && ca_cert->pubkey_type != L_CERT_KEY_RSA)
+ return false;
+
+ if (ca_cert) {
+ L_AUTO_CLEANUP_VAR(struct l_key *, ca_key, cert_key_cleanup);
+ ca_key = NULL;
+
+ ca_ring = l_keyring_new();
+ if (!ca_ring)
+ return false;
+
+ ca_key = l_key_new(L_KEY_RSA, ca_cert->asn1, ca_cert->asn1_len);
+ if (!ca_key || !l_keyring_link(ca_ring, ca_key))
+ return false;
+ }
+
+ verify_ring = l_keyring_new();
+ if (!verify_ring)
+ return false;
+
+ /*
+ * If a CA cert was supplied, restrict verify_ring now so
+ * everything else in certchain is validated against the CA.
+ * Otherwise, verify_ring will be restricted after the root of
+ * certchain is added to verify_ring by
+ * cert_verify_with_keyring().
+ */
+ if (ca_ring && !l_keyring_restrict(verify_ring,
+ L_KEYRING_RESTRICT_ASYM_CHAIN,
+ ca_ring)) {
+ return false;
+ }
+
+ return cert_verify_with_keyring(certchain, verify_ring, ca_cert,
+ ca_ring);
+}
+
+LIB_EXPORT enum l_cert_key_type l_cert_get_pubkey_type(struct l_cert *cert)
+{
+ return cert->pubkey_type;
+}
+
+LIB_EXPORT struct l_key *l_cert_get_pubkey(struct l_cert *cert)
+{
+ /* Use kernel's ASN.1 certificate parser to find the key data for us */
+ if (cert->pubkey_type == L_CERT_KEY_RSA)
+ return l_key_new(L_KEY_RSA, cert->asn1, cert->asn1_len);
+
+ return NULL;
+}
diff --git a/ell/cert.h b/ell/cert.h
new file mode 100644
index 0000000..4d809be
--- /dev/null
+++ b/ell/cert.h
@@ -0,0 +1,57 @@
+/*
+ *
+ * Embedded Linux library
+ *
+ * Copyright (C) 2018 Intel Corporation. All rights reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifndef __ELL_CERT_H
+#define __ELL_CERT_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stddef.h>
+#include <stdbool.h>
+
+#include <ell/key.h>
+
+struct l_cert;
+
+enum l_cert_key_type {
+ L_CERT_KEY_RSA,
+ L_CERT_KEY_UNKNOWN,
+};
+
+struct l_cert *l_cert_new_from_der(const uint8_t *buf, size_t buf_len);
+void l_cert_free(struct l_cert *cert);
+
+bool l_cert_link_issuer(struct l_cert *cert, struct l_cert *issuer);
+struct l_cert *l_cert_get_issuer(struct l_cert *cert);
+const uint8_t *l_cert_get_der_data(struct l_cert *cert, size_t *out_len);
+bool l_cert_find_certchain(struct l_cert *cert, struct l_cert *ca_cert);
+bool l_cert_verify_certchain(struct l_cert *certchain, struct l_cert *ca_cert);
+enum l_cert_key_type l_cert_get_pubkey_type(struct l_cert *cert);
+struct l_key *l_cert_get_pubkey(struct l_cert *cert);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ELL_CERT_H */
diff --git a/ell/ell.h b/ell/ell.h
index ba1928d..4b0eac4 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -56,3 +56,4 @@
#include <ell/dbus-service.h>
#include <ell/dbus-client.h>
#include <ell/dhcp.h>
+#include <ell/cert.h>
diff --git a/ell/ell.sym b/ell/ell.sym
index 614bc93..d100005 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -432,6 +432,14 @@ global:
l_uuid_is_valid;
l_uuid_get_version;
l_uuid_to_string;
+ /* cert */
+ l_cert_new_from_der;
+ l_cert_free;
+ l_cert_get_der_data;
+ l_cert_find_certchain;
+ l_cert_verify_certchain;
+ l_cert_get_pubkey_type;
+ l_cert_get_pubkey;
local:
*;
};
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code
2018-11-08 12:06 [PATCH 1/5] cert: Add cert.c for certificate types and routines Andrew Zaborowski
@ 2018-11-08 12:06 ` Andrew Zaborowski
2018-11-08 23:20 ` Mat Martineau
2018-11-08 12:06 ` [PATCH 3/5] pem: Return an l_key from l_pem_load_private_key Andrew Zaborowski
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 12:06 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 24369 bytes --]
Drop the struct tls_cert utilities and use the l_cert type for all the
certificate operations. tls_cert_from_certificate_list is renamed to
tls_parse_certificate_list and moved up in the file. l_tls_set_cacert
return type is changed to bool so that errors in loading CA certificate
are returned immediately instead of failing the handshake later on.
Since this just changes a void to a bool it shouldn't break ell user
builds. l_pem_load_certificate is changed to return the l_cert object
instead of a byte buffer so there's no longer to call the l_cert
constructor separately.
---
ell/pem.c | 8 +-
ell/pem.h | 4 +-
ell/tls-private.h | 38 +---
ell/tls-record.c | 1 +
ell/tls.c | 455 +++++++++++++---------------------------------
ell/tls.h | 2 +-
6 files changed, 138 insertions(+), 370 deletions(-)
diff --git a/ell/pem.c b/ell/pem.c
index e24dd7d..f4aa7e2 100644
--- a/ell/pem.c
+++ b/ell/pem.c
@@ -213,13 +213,13 @@ LIB_EXPORT uint8_t *l_pem_load_file(const char *filename, int index,
return result;
}
-LIB_EXPORT uint8_t *l_pem_load_certificate(const char *filename, size_t *len)
+LIB_EXPORT struct l_cert *l_pem_load_certificate(const char *filename)
{
uint8_t *content;
char *label;
+ size_t len;
- content = l_pem_load_file(filename, 0, &label, len);
-
+ content = l_pem_load_file(filename, 0, &label, &len);
if (!content)
return NULL;
@@ -230,7 +230,7 @@ LIB_EXPORT uint8_t *l_pem_load_certificate(const char *filename, size_t *len)
l_free(label);
- return content;
+ return l_cert_new_from_der(content, len);
}
/**
diff --git a/ell/pem.h b/ell/pem.h
index 93282a3..9629ef2 100644
--- a/ell/pem.h
+++ b/ell/pem.h
@@ -27,12 +27,14 @@
extern "C" {
#endif
+#include <ell/cert.h>
+
uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
int index, char **type_label, size_t *out_len);
uint8_t *l_pem_load_file(const char *filename, int index,
char **type_label, size_t *len);
-uint8_t *l_pem_load_certificate(const char *filename, size_t *len);
+struct l_cert *l_pem_load_certificate(const char *filename);
uint8_t *l_pem_load_private_key(const char *filename, const char *passphrase,
bool *encrypted, size_t *len);
diff --git a/ell/tls-private.h b/ell/tls-private.h
index 4f680a0..2257041 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -28,8 +28,6 @@
#define TLS_VERSION TLS_V12
#define TLS_MIN_VERSION TLS_V10
-struct tls_cert;
-
enum tls_cipher_type {
TLS_CIPHER_STREAM,
TLS_CIPHER_BLOCK,
@@ -60,7 +58,7 @@ struct tls_key_exchange_algorithm {
bool certificate_check;
- bool (*validate_cert_key_type)(struct tls_cert *cert);
+ bool (*validate_cert_key_type)(struct l_cert *cert);
bool (*send_client_key_exchange)(struct l_tls *tls);
void (*handle_client_key_exchange)(struct l_tls *tls,
@@ -142,8 +140,8 @@ struct l_tls {
l_tls_destroy_cb_t debug_destroy;
void *debug_data;
- char *ca_cert_path;
- char *cert_path;
+ struct l_cert *ca_cert;
+ struct l_cert *cert;
struct l_key *priv_key;
size_t priv_key_size;
@@ -169,7 +167,7 @@ struct l_tls {
uint16_t negotiated_version;
bool cert_requested, cert_sent;
bool peer_authenticated;
- struct tls_cert *peer_cert;
+ struct l_cert *peer_cert;
struct l_key *peer_pubkey;
size_t peer_pubkey_size;
enum handshake_hash_type signature_hash;
@@ -226,32 +224,8 @@ void tls_tx_record(struct l_tls *tls, enum tls_content_type type,
bool tls_handle_message(struct l_tls *tls, const uint8_t *message,
int len, enum tls_content_type type, uint16_t version);
-/* X509 Certificates and Certificate Chains */
-
-struct tls_cert {
- size_t size;
- struct tls_cert *issuer;
- uint8_t asn1[0];
-};
-
-enum tls_cert_key_type {
- TLS_CERT_KEY_RSA,
- TLS_CERT_KEY_UNKNOWN,
-};
-
-struct tls_cert *tls_cert_load_file(const char *filename);
-int tls_cert_from_certificate_list(const void *data, size_t len,
- struct tls_cert **out_certchain);
-
-bool tls_cert_find_certchain(struct tls_cert *cert,
- const char *cacert_filename);
-
-bool tls_cert_verify_certchain(struct tls_cert *certchain,
- struct tls_cert *ca_cert);
-
-void tls_cert_free_certchain(struct tls_cert *cert);
-
-enum tls_cert_key_type tls_cert_get_pubkey_type(struct tls_cert *cert);
+int tls_parse_certificate_list(const void *data, size_t len,
+ struct l_cert **out_certchain);
void tls_prf_get_bytes(struct l_tls *tls,
enum l_checksum_type type, size_t hash_len,
diff --git a/ell/tls-record.c b/ell/tls-record.c
index 8038049..410f194 100644
--- a/ell/tls-record.c
+++ b/ell/tls-record.c
@@ -29,6 +29,7 @@
#include "tls.h"
#include "checksum.h"
#include "cipher.h"
+#include "cert.h"
#include "tls-private.h"
#include "random.h"
diff --git a/ell/tls.c b/ell/tls.c
index 4b37785..6bea5dc 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -35,6 +35,7 @@
#include "cipher.h"
#include "random.h"
#include "pem.h"
+#include "cert.h"
#include "tls-private.h"
#include "key.h"
#include "asn1-private.h"
@@ -176,7 +177,7 @@ static void tls_reset_handshake(struct l_tls *tls)
memset(tls->pending.key_block, 0, sizeof(tls->pending.key_block));
- l_free(tls->peer_cert);
+ l_cert_free(tls->peer_cert);
l_key_free(tls->peer_pubkey);
tls->peer_cert = NULL;
@@ -336,9 +337,9 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
tls_get_hash_t get_hash);
-static bool tls_rsa_validate_cert_key(struct tls_cert *cert)
+static bool tls_rsa_validate_cert_key(struct l_cert *cert)
{
- return tls_cert_get_pubkey_type(cert) == TLS_CERT_KEY_RSA;
+ return l_cert_get_pubkey_type(cert) == L_CERT_KEY_RSA;
}
static struct tls_key_exchange_algorithm tls_rsa = {
@@ -611,6 +612,55 @@ static void pkcs1_write_digest_info(enum l_checksum_type type,
*out_len += hash_len;
}
+int tls_parse_certificate_list(const void *data, size_t len,
+ struct l_cert **out_certchain)
+{
+ const uint8_t *buf = data;
+ struct l_cert *certchain = NULL, *tail;
+
+ while (len) {
+ struct l_cert *cert;
+ size_t cert_len;
+
+ if (len < 3)
+ goto decode_error;
+
+ cert_len = *buf++ << 16;
+ cert_len |= *buf++ << 8;
+ cert_len |= *buf++ << 0;
+
+ if (cert_len + 3 > len)
+ goto decode_error;
+
+ cert = l_cert_new_from_der(buf, cert_len);
+ if (!cert)
+ goto decode_error;
+
+ if (!certchain)
+ certchain = cert;
+ else if (!l_cert_link_issuer(tail, cert)) {
+ l_cert_free(cert);
+ goto decode_error;
+ }
+
+ tail = cert;
+
+ buf += cert_len;
+ len -= cert_len + 3;
+ }
+
+ if (out_certchain)
+ *out_certchain = certchain;
+ else
+ l_cert_free(certchain);
+
+ return 0;
+
+decode_error:
+ l_cert_free(certchain);
+ return -EBADMSG;
+}
+
static void tls_send_alert(struct l_tls *tls, bool fatal,
enum l_tls_alert_desc alert_desc)
{
@@ -743,36 +793,25 @@ static void tls_send_server_hello(struct l_tls *tls)
static bool tls_send_certificate(struct l_tls *tls)
{
uint8_t *buf, *ptr;
- struct tls_cert *cert, *i;
+ struct l_cert *i;
size_t total;
- if (tls->cert_path)
- cert = tls_cert_load_file(tls->cert_path);
- else
- cert = NULL;
-
- if (tls->server && !cert) {
+ if (tls->server && !tls->cert) {
TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
- "Loading server certificate %s failed",
- tls->cert_path);
+ "Certificate needed in server mode");
return false;
}
- if (cert && !tls_cert_find_certchain(cert, tls->ca_cert_path)) {
- if (tls->server) {
- TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
- TLS_ALERT_UNKNOWN_CA,
- "Can't find certificate chain local "
- "CA cert %s", tls->ca_cert_path);
-
- return false;
- } else
- cert = NULL;
+ if (tls->cert && !l_cert_find_certchain(tls->cert, tls->ca_cert)) {
+ TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_UNKNOWN_CA,
+ "Can't find certificate chain to local "
+ "CA cert");
+ return false;
}
/* TODO: might want check this earlier and exclude the cipher suite */
- if (cert && !tls->pending.cipher_suite->key_xchg->
- validate_cert_key_type(cert)) {
+ if (tls->cert && !tls->pending.cipher_suite->key_xchg->
+ validate_cert_key_type(tls->cert)) {
TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_CERT_UNKNOWN,
"Local certificate has key type incompatible "
"with pending cipher suite %s",
@@ -799,8 +838,12 @@ static bool tls_send_certificate(struct l_tls *tls)
*/
total = 0;
- for (i = cert; i; i = i->issuer)
- total += 3 + i->size;
+ for (i = tls->cert; i; i = l_cert_get_issuer(i)) {
+ size_t der_len;
+
+ l_cert_get_der_data(i, &der_len);
+ total += 3 + der_len;
+ }
buf = l_malloc(128 + total);
ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
@@ -811,24 +854,26 @@ static bool tls_send_certificate(struct l_tls *tls)
*ptr++ = total >> 8;
*ptr++ = total >> 0;
- for (i = cert; i; i = i->issuer) {
- *ptr++ = i->size >> 16;
- *ptr++ = i->size >> 8;
- *ptr++ = i->size >> 0;
+ for (i = tls->cert; i; i = l_cert_get_issuer(i)) {
+ const uint8_t *der;
+ size_t der_len;
- memcpy(ptr, i->asn1, i->size);
- ptr += i->size;
+ der = l_cert_get_der_data(i, &der_len);
+ *ptr++ = der_len >> 16;
+ *ptr++ = der_len >> 8;
+ *ptr++ = der_len >> 0;
+
+ memcpy(ptr, der, der_len);
+ ptr += der_len;
}
tls_tx_handshake(tls, TLS_CERTIFICATE, buf, ptr - buf);
l_free(buf);
- if (cert)
+ if (tls->cert)
tls->cert_sent = true;
- l_free(cert);
-
return true;
}
@@ -1413,7 +1458,7 @@ static void tls_handle_client_hello(struct l_tls *tls,
* TODO: filter supported cipher suites by the certificate/key
* type that was submitted by tls_set_auth_data() if any.
* Perhaps just call cipher_suite->verify_cert_type() on each
- * cipher suite passing a pre-parsed certificate ASN.1 struct.
+ * cipher suite passing tls->cert.
*/
tls->pending.cipher_suite =
tls_find_cipher_suite(cipher_suites);
@@ -1457,21 +1502,20 @@ static void tls_handle_client_hello(struct l_tls *tls,
tls_send_server_hello(tls);
- if (tls->pending.cipher_suite->key_xchg->certificate_check &&
- tls->cert_path)
+ if (tls->pending.cipher_suite->key_xchg->certificate_check && tls->cert)
if (!tls_send_certificate(tls))
return;
/* TODO: don't bother if configured to not authenticate client */
if (tls->pending.cipher_suite->key_xchg->certificate_check &&
- tls->ca_cert_path)
+ tls->ca_cert)
if (!tls_send_certificate_request(tls))
return;
tls_send_server_hello_done(tls);
if (tls->pending.cipher_suite->key_xchg->certificate_check &&
- tls->ca_cert_path)
+ tls->ca_cert)
TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE);
else
TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
@@ -1570,8 +1614,7 @@ static void tls_handle_certificate(struct l_tls *tls,
const uint8_t *buf, size_t len)
{
size_t total;
- struct tls_cert *certchain = NULL;
- struct tls_cert *ca_cert = NULL;
+ struct l_cert *certchain = NULL;
bool dummy;
if (len < 3)
@@ -1584,7 +1627,7 @@ static void tls_handle_certificate(struct l_tls *tls,
if (total + 3 != len)
goto decode_error;
- if (tls_cert_from_certificate_list(buf, total, &certchain) < 0) {
+ if (tls_parse_certificate_list(buf, total, &certchain) < 0) {
TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
"Error decoding peer certificate chain");
@@ -1616,22 +1659,11 @@ static void tls_handle_certificate(struct l_tls *tls,
* Validate the certificate chain's consistency and validate it
* against our CA if we have any.
*/
-
- if (tls->ca_cert_path) {
- ca_cert = tls_cert_load_file(tls->ca_cert_path);
- if (!ca_cert) {
- TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
- TLS_ALERT_BAD_CERT,
- "Can't load %s", tls->ca_cert_path);
-
- goto done;
- }
- }
-
- if (!tls_cert_verify_certchain(certchain, ca_cert)) {
+ if (!l_cert_verify_certchain(certchain, tls->ca_cert)) {
TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
- "Peer certchain verification failed against "
- "CA cert %s", tls->ca_cert_path);
+ "Peer certchain verification failed "
+ "consistency check%s",
+ tls->ca_cert ? " or against local CA cert" : "");
goto done;
}
@@ -1654,13 +1686,11 @@ static void tls_handle_certificate(struct l_tls *tls,
/* Save the end-entity cert and free the rest of the chain */
tls->peer_cert = certchain;
- tls_cert_free_certchain(certchain->issuer);
- certchain->issuer = NULL;
- certchain = NULL;
-
- tls->peer_pubkey = l_key_new(L_KEY_RSA, tls->peer_cert->asn1,
- tls->peer_cert->size);
+ certchain = l_cert_get_issuer(certchain);
+ if (certchain)
+ l_cert_link_issuer(tls->peer_cert, NULL);
+ tls->peer_pubkey = l_cert_get_pubkey(tls->peer_cert);
if (!tls->peer_pubkey) {
TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
"Error loading peer public key to kernel");
@@ -1691,10 +1721,7 @@ decode_error:
"TLS_CERTIFICATE decode error");
done:
- if (ca_cert)
- l_free(ca_cert);
-
- tls_cert_free_certchain(certchain);
+ l_cert_free(certchain);
}
static void tls_handle_certificate_request(struct l_tls *tls,
@@ -1946,8 +1973,8 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
* - If we received an (expected) Certificate Verify, we must have
* sent a Certificate Request.
* - If we sent a Certificate Request that's because
- * tls->ca_cert_path is non-NULL.
- * - If tls->ca_cert_path is non-NULL then tls_handle_certificate
+ * tls->ca_cert is non-NULL.
+ * - If tls->ca_cert is non-NULL then tls_handle_certificate
* will have checked the whole certificate chain to be valid and
* additionally trusted by our CA if known.
* - Additionally cipher_suite->key_xchg->verify has just confirmed
@@ -2159,7 +2186,7 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
/*
* On the client, the server's certificate is only now
* verified, based on the following logic:
- * - tls->ca_cert_path is non-NULL so tls_handle_certificate
+ * - tls->ca_cert is non-NULL so tls_handle_certificate
* (always called on the client) must have veritifed the
* server's certificate chain to be valid and additionally
* trusted by our CA.
@@ -2172,8 +2199,7 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
* message (either should be enough).
*/
if (!tls->server && tls->cipher_suite[0]->key_xchg->
- certificate_check &&
- tls->ca_cert_path)
+ certificate_check && tls->ca_cert)
tls->peer_authenticated = true;
tls_finished(tls);
@@ -2400,17 +2426,24 @@ LIB_EXPORT void l_tls_close(struct l_tls *tls)
TLS_DISCONNECT(TLS_ALERT_CLOSE_NOTIFY, 0, "Closing session");
}
-LIB_EXPORT void l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path)
+LIB_EXPORT bool l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path)
{
TLS_DEBUG("ca-cert-path=%s", ca_cert_path);
- if (tls->ca_cert_path) {
- l_free(tls->ca_cert_path);
- tls->ca_cert_path = NULL;
+ if (tls->ca_cert) {
+ l_cert_free(tls->ca_cert);
+ tls->ca_cert = NULL;
}
- if (ca_cert_path)
- tls->ca_cert_path = l_strdup(ca_cert_path);
+ if (ca_cert_path) {
+ tls->ca_cert = l_pem_load_certificate(ca_cert_path);
+ if (!tls->ca_cert) {
+ TLS_DEBUG("Error loading %s", ca_cert_path);
+ return false;
+ }
+ }
+
+ return true;
}
LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
@@ -2420,9 +2453,9 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
TLS_DEBUG("cert-path=%s priv-key-path=%s priv-key-passphrase=%p",
cert_path, priv_key_path, priv_key_passphrase);
- if (tls->cert_path) {
- l_free(tls->cert_path);
- tls->cert_path = NULL;
+ if (tls->cert) {
+ l_cert_free(tls->cert);
+ tls->cert = NULL;
}
if (tls->priv_key) {
@@ -2431,6 +2464,14 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
tls->priv_key_size = 0;
}
+ if (cert_path) {
+ tls->cert = l_pem_load_certificate(cert_path);
+ if (!tls->cert) {
+ TLS_DEBUG("Error loading %s", cert_path);
+ return false;
+ }
+ }
+
if (priv_key_path) {
uint8_t *priv_key;
bool is_public = true;
@@ -2463,9 +2504,6 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
tls->priv_key_size /= 8;
}
- if (cert_path)
- tls->cert_path = l_strdup(cert_path);
-
return true;
}
@@ -2546,253 +2584,6 @@ const char *tls_handshake_state_to_str(enum tls_handshake_state state)
return buf;
}
-/* X509 Certificates and Certificate Chains */
-
-#define X509_CERTIFICATE_POS 0
-#define X509_TBSCERTIFICATE_POS 0
-#define X509_TBSCERT_VERSION_POS 0
-#define X509_TBSCERT_SERIAL_POS 1
-#define X509_TBSCERT_SIGNATURE_POS 2
-#define X509_ALGORITHM_ID_ALGORITHM_POS 0
-#define X509_ALGORITHM_ID_PARAMS_POS 1
-#define X509_TBSCERT_ISSUER_DN_POS 3
-#define X509_TBSCERT_VALIDITY_POS 4
-#define X509_TBSCERT_SUBJECT_DN_POS 5
-#define X509_TBSCERT_SUBJECT_KEY_POS 6
-#define X509_SUBJECT_KEY_ALGORITHM_POS 0
-#define X509_SUBJECT_KEY_VALUE_POS 1
-#define X509_TBSCERT_ISSUER_UID_POS 7
-#define X509_TBSCERT_SUBJECT_UID_POS 8
-#define X509_TBSCERT_EXTENSIONS_POS 9
-#define X509_SIGNATURE_ALGORITHM_POS 1
-#define X509_SIGNATURE_VALUE_POS 2
-
-struct tls_cert *tls_cert_load_file(const char *filename)
-{
- uint8_t *der;
- size_t len;
- struct tls_cert *cert;
-
- der = l_pem_load_certificate(filename, &len);
- if (!der)
- return NULL;
-
- if (!len || der[0] != ASN1_ID_SEQUENCE) {
- l_free(der);
- return NULL;
- }
-
- cert = l_malloc(sizeof(struct tls_cert) + len);
- cert->size = len;
- cert->issuer = NULL;
- memcpy(cert->asn1, der, len);
-
- l_free(der);
-
- return cert;
-}
-
-int tls_cert_from_certificate_list(const void *data, size_t len,
- struct tls_cert **out_certchain)
-{
- const uint8_t *buf = data;
- struct tls_cert *certchain = NULL, **tail = &certchain;
-
- while (len) {
- size_t cert_len;
-
- if (len < 3)
- goto decode_error;
-
- cert_len = *buf++ << 16;
- cert_len |= *buf++ << 8;
- cert_len |= *buf++ << 0;
-
- if (cert_len + 3 > len)
- goto decode_error;
-
- *tail = l_malloc(sizeof(struct tls_cert) + cert_len);
- (*tail)->size = cert_len;
- (*tail)->issuer = NULL;
- memcpy((*tail)->asn1, buf, cert_len);
-
- tail = &(*tail)->issuer;
-
- buf += cert_len;
- len -= cert_len + 3;
- }
-
- if (out_certchain)
- *out_certchain = certchain;
-
- return 0;
-
-decode_error:
- tls_cert_free_certchain(certchain);
- return -EBADMSG;
-}
-
-bool tls_cert_find_certchain(struct tls_cert *cert,
- const char *cacert_filename)
-{
- return true;
-}
-
-static const struct pkcs1_encryption_oid {
- enum tls_cert_key_type key_type;
- struct asn1_oid oid;
-} pkcs1_encryption_oids[] = {
- { /* rsaEncryption */
- TLS_CERT_KEY_RSA,
- { 9, { 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01 } },
- },
-};
-
-static void tls_key_cleanup(struct l_key **p)
-{
- l_key_free_norevoke(*p);
-}
-
-static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
- struct l_keyring *ring,
- struct tls_cert *root,
- struct l_keyring *trusted)
-{
- if (!cert)
- return true;
-
- /*
- * RFC5246 7.4.2:
- * "Because certificate validation requires that root keys be
- * distributed independently, the self-signed certificate that
- * specifies the root certificate authority MAY be omitted from
- * the chain, under the assumption that the remote end must
- * already possess it in order to validate it in any case."
- */
- if (!cert->issuer && root && cert->size == root->size &&
- !memcmp(cert->asn1, root->asn1, root->size))
- return true;
-
- if (tls_cert_verify_with_keyring(cert->issuer, ring, root, trusted)) {
- L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup);
-
- key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
- if (!key)
- return false;
-
- if (!l_keyring_link(ring, key))
- return false;
-
- if (trusted || cert->issuer)
- return true;
-
- /*
- * If execution reaches this point, it's known that:
- * * No trusted root key was supplied, so the chain is only
- * being checked against its own root
- * * The keyring 'ring' is not restricted yet
- * * The chain's root cert was just linked in to the
- * previously empty keyring 'ring'.
- *
- * By restricting 'ring' now, the rest of the certs in
- * the chain will have their signature validated using 'key'
- * as the root.
- */
- return l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN,
- trusted);
- }
-
- return false;
-}
-
-static void tls_keyring_cleanup(struct l_keyring **p)
-{
- l_keyring_free(*p);
-}
-
-bool tls_cert_verify_certchain(struct tls_cert *certchain,
- struct tls_cert *ca_cert)
-{
- L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
- L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
- tls_keyring_cleanup);
-
- ca_ring = NULL;
- verify_ring = NULL;
-
- if (ca_cert) {
- L_AUTO_CLEANUP_VAR(struct l_key *, ca_key, tls_key_cleanup);
- ca_key = NULL;
-
- ca_ring = l_keyring_new();
- if (!ca_ring)
- return false;
-
- ca_key = l_key_new(L_KEY_RSA, ca_cert->asn1, ca_cert->size);
- if (!ca_key || !l_keyring_link(ca_ring, ca_key))
- return false;
- }
-
- verify_ring = l_keyring_new();
- if (!verify_ring)
- return false;
-
- /*
- * If a CA cert was supplied, restrict verify_ring now so
- * everything else in certchain is validated against the CA.
- * Otherwise, verify_ring will be restricted after the root of
- * certchain is added to verify_ring by
- * tls_cert_verify_with_keyring().
- */
- if (ca_ring && !l_keyring_restrict(verify_ring,
- L_KEYRING_RESTRICT_ASYM_CHAIN,
- ca_ring)) {
- return false;
- }
-
- return tls_cert_verify_with_keyring(certchain, verify_ring, ca_cert,
- ca_ring);
-}
-
-void tls_cert_free_certchain(struct tls_cert *cert)
-{
- struct tls_cert *next;
-
- while (cert) {
- next = cert->issuer;
- l_free(cert);
- cert = next;
- }
-}
-
-enum tls_cert_key_type tls_cert_get_pubkey_type(struct tls_cert *cert)
-{
- const uint8_t *key_type;
- size_t key_type_len;
- int i;
-
- key_type = asn1_der_find_elem_by_path(cert->asn1, cert->size,
- ASN1_ID_OID, &key_type_len,
- X509_CERTIFICATE_POS,
- X509_TBSCERTIFICATE_POS,
- X509_TBSCERT_SUBJECT_KEY_POS,
- X509_SUBJECT_KEY_ALGORITHM_POS,
- X509_ALGORITHM_ID_ALGORITHM_POS,
- -1);
- if (!key_type)
- return TLS_CERT_KEY_UNKNOWN;
-
- for (i = 0; i < (int) L_ARRAY_SIZE(pkcs1_encryption_oids); i++)
- if (asn1_oid_eq(&pkcs1_encryption_oids[i].oid,
- key_type_len, key_type))
- break;
-
- if (i == L_ARRAY_SIZE(pkcs1_encryption_oids))
- return TLS_CERT_KEY_UNKNOWN;
-
- return pkcs1_encryption_oids[i].key_type;
-}
-
LIB_EXPORT bool l_tls_set_debug(struct l_tls *tls, l_tls_debug_cb_t function,
void *user_data, l_tls_destroy_cb_t destroy)
{
diff --git a/ell/tls.h b/ell/tls.h
index 893fd63..5008200 100644
--- a/ell/tls.h
+++ b/ell/tls.h
@@ -87,7 +87,7 @@ void l_tls_write(struct l_tls *tls, const uint8_t *data, size_t len);
void l_tls_handle_rx(struct l_tls *tls, const uint8_t *data, size_t len);
/* If peer is to be authenticated, supply the CA certificate */
-void l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path);
+bool l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path);
/*
* If we are to be authenticated, supply our certificate, private key and
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/5] pem: Return an l_key from l_pem_load_private_key
2018-11-08 12:06 [PATCH 1/5] cert: Add cert.c for certificate types and routines Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code Andrew Zaborowski
@ 2018-11-08 12:06 ` Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 4/5] tools: Update certchain-verify to new certificate API Andrew Zaborowski
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 12:06 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 5223 bytes --]
Change l_pem_load_private_key to directly return the l_key object
instead of the byte buffer (and length) so there's no need to call
the l_key constructor seprately.
---
ell/pem.c | 36 ++++++++++++++++++++++--------------
ell/pem.h | 6 ++++--
ell/tls.c | 18 +++++-------------
3 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/ell/pem.c b/ell/pem.c
index f4aa7e2..98422e0 100644
--- a/ell/pem.c
+++ b/ell/pem.c
@@ -238,7 +238,6 @@ LIB_EXPORT struct l_cert *l_pem_load_certificate(const char *filename)
* @filename: path string to the PEM file to load
* @passphrase: private key encryption passphrase or NULL for unencrypted
* @encrypted: receives indication whether the file was encrypted if non-NULL
- * @len: receives the length of the returned buffer
*
* Load the PEM encoded RSA Private Key file at @filename. If it is an
* encrypted private key and @passphrase was non-NULL, the file is
@@ -247,20 +246,22 @@ LIB_EXPORT struct l_cert *l_pem_load_certificate(const char *filename)
* success case and on error when NULL is returned. This can be used to
* check if a passphrase is required without prior information.
*
- * Returns: Buffer containing raw DER data for the private key or NULL, to
- * be freed with l_free.
+ * Returns: An l_key object to be freed with an l_key_free* function,
+ * or NULL.
**/
-LIB_EXPORT uint8_t *l_pem_load_private_key(const char *filename,
+LIB_EXPORT struct l_key *l_pem_load_private_key(const char *filename,
const char *passphrase,
- bool *encrypted, size_t *len)
+ bool *encrypted)
{
uint8_t *content;
char *label;
+ size_t len;
+ struct l_key *pkey = NULL;
if (encrypted)
*encrypted = false;
- content = l_pem_load_file(filename, 0, &label, len);
+ content = l_pem_load_file(filename, 0, &label, &len);
if (!content)
return NULL;
@@ -289,7 +290,7 @@ LIB_EXPORT uint8_t *l_pem_load_private_key(const char *filename,
goto err;
/* Technically this is BER, not limited to DER */
- key_info = asn1_der_find_elem(content, *len, 0, &tag,
+ key_info = asn1_der_find_elem(content, len, 0, &tag,
&key_info_len);
if (!key_info || tag != ASN1_ID_SEQUENCE)
goto err;
@@ -305,7 +306,7 @@ LIB_EXPORT uint8_t *l_pem_load_private_key(const char *filename,
(data_len & 7) != 0)
goto err;
- if (asn1_der_find_elem(content, *len, 2, &tag, &tmp_len))
+ if (asn1_der_find_elem(content, len, 2, &tag, &tmp_len))
goto err;
alg = pkcs5_cipher_from_alg_id(alg_id, alg_id_len, passphrase);
@@ -321,8 +322,10 @@ LIB_EXPORT uint8_t *l_pem_load_private_key(const char *filename,
}
l_cipher_free(alg);
+ memset(content, 0, len);
l_free(content);
content = decrypted;
+ len = data_len;
/*
* Strip padding as defined in RFC8018 (for PKCS#5 v1) or
@@ -337,7 +340,7 @@ LIB_EXPORT uint8_t *l_pem_load_private_key(const char *filename,
if (content[data_len - 1 - i] != content[data_len - 1])
goto err;
- *len = data_len - content[data_len - 1];
+ len = data_len - content[data_len - 1];
goto done;
}
@@ -350,12 +353,17 @@ LIB_EXPORT uint8_t *l_pem_load_private_key(const char *filename,
*/
/* Label not known */
-err:
- l_free(content);
- content = NULL;
+ goto err;
done:
- l_free(label);
+ pkey = l_key_new(L_KEY_RSA, content, len);
- return content;
+err:
+ if (content) {
+ memset(content, 0, len);
+ l_free(content);
+ }
+
+ l_free(label);
+ return pkey;
}
diff --git a/ell/pem.h b/ell/pem.h
index 9629ef2..7f0d27a 100644
--- a/ell/pem.h
+++ b/ell/pem.h
@@ -27,6 +27,7 @@
extern "C" {
#endif
+#include <ell/key.h>
#include <ell/cert.h>
uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
@@ -36,8 +37,9 @@ uint8_t *l_pem_load_file(const char *filename, int index,
struct l_cert *l_pem_load_certificate(const char *filename);
-uint8_t *l_pem_load_private_key(const char *filename, const char *passphrase,
- bool *encrypted, size_t *len);
+struct l_key *l_pem_load_private_key(const char *filename,
+ const char *passphrase,
+ bool *encrypted);
#ifdef __cplusplus
}
diff --git a/ell/tls.c b/ell/tls.c
index 6bea5dc..7433524 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2473,23 +2473,15 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
}
if (priv_key_path) {
- uint8_t *priv_key;
bool is_public = true;
- priv_key = l_pem_load_private_key(priv_key_path,
+ tls->priv_key = l_pem_load_private_key(priv_key_path,
priv_key_passphrase,
- NULL,
- &tls->priv_key_size);
- TLS_DEBUG("l_pem_load_private_key returned %p, size %zi",
- priv_key, tls->priv_key_size);
-
- if (!priv_key)
+ NULL);
+ if (!tls->priv_key) {
+ TLS_DEBUG("Error loading %s", priv_key_path);
return false;
-
- tls->priv_key = l_key_new(L_KEY_RSA, priv_key,
- tls->priv_key_size);
- memset(priv_key, 0, tls->priv_key_size);
- l_free(priv_key);
+ }
if (!l_key_get_info(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
L_CHECKSUM_NONE, &tls->priv_key_size,
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] tools: Update certchain-verify to new certificate API
2018-11-08 12:06 [PATCH 1/5] cert: Add cert.c for certificate types and routines Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 3/5] pem: Return an l_key from l_pem_load_private_key Andrew Zaborowski
@ 2018-11-08 12:06 ` Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 5/5] unit: Update tests " Andrew Zaborowski
2018-11-08 17:39 ` [PATCH 1/5] cert: Add cert.c for certificate types and routines Denis Kenzior
4 siblings, 0 replies; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 12:06 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]
---
tools/certchain-verify.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/certchain-verify.c b/tools/certchain-verify.c
index 1800f68..d61c722 100644
--- a/tools/certchain-verify.c
+++ b/tools/certchain-verify.c
@@ -35,7 +35,7 @@
#include <ell/ell.h>
#include "ell/tls-private.h"
-static int load_cert_chain(const char *file, struct tls_cert **certchain)
+static int load_cert_chain(const char *file, struct l_cert **certchain)
{
int fd;
struct stat st;
@@ -70,7 +70,7 @@ static int load_cert_chain(const char *file, struct tls_cert **certchain)
goto close_file;
}
- err = tls_cert_from_certificate_list(data, st.st_size, certchain);
+ err = tls_parse_certificate_list(data, st.st_size, certchain);
if (err < 0)
fprintf(stderr, "Could not parse certificate list: %s\n",
strerror(-err));
@@ -95,8 +95,8 @@ static void usage(const char *bin)
int main(int argc, char *argv[])
{
int status = EXIT_FAILURE;
- struct tls_cert *certchain;
- struct tls_cert *ca_cert;
+ struct l_cert *certchain;
+ struct l_cert *ca_cert;
int err;
if (argc != 3) {
@@ -116,13 +116,13 @@ int main(int argc, char *argv[])
goto done;
}
- ca_cert = tls_cert_load_file(argv[1]);
+ ca_cert = l_pem_load_certificate(argv[1]);
if (!ca_cert) {
fprintf(stderr, "Unable to load CA certifiate\n");
goto free_certchain;
}
- if (!tls_cert_verify_certchain(certchain, ca_cert)) {
+ if (!l_cert_verify_certchain(certchain, ca_cert)) {
fprintf(stderr, "Verification failed\n");
goto free_cacert;
}
@@ -131,9 +131,9 @@ int main(int argc, char *argv[])
status = EXIT_SUCCESS;
free_cacert:
- l_free(ca_cert);
+ l_cert_free(ca_cert);
free_certchain:
- tls_cert_free_certchain(certchain);
+ l_cert_free(certchain);
done:
return status;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] unit: Update tests to new certificate API
2018-11-08 12:06 [PATCH 1/5] cert: Add cert.c for certificate types and routines Andrew Zaborowski
` (2 preceding siblings ...)
2018-11-08 12:06 ` [PATCH 4/5] tools: Update certchain-verify to new certificate API Andrew Zaborowski
@ 2018-11-08 12:06 ` Andrew Zaborowski
2018-11-08 23:27 ` Mat Martineau
2018-11-08 17:39 ` [PATCH 1/5] cert: Add cert.c for certificate types and routines Denis Kenzior
4 siblings, 1 reply; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 12:06 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 10302 bytes --]
---
unit/test-key.c | 89 ++++++++++++++++++++-----------------------------
unit/test-pem.c | 57 ++++++++++++++++++-------------
unit/test-tls.c | 24 ++++++-------
3 files changed, 83 insertions(+), 87 deletions(-)
diff --git a/unit/test-key.c b/unit/test-key.c
index afe4103..5f65793 100644
--- a/unit/test-key.c
+++ b/unit/test-key.c
@@ -394,23 +394,20 @@ static void test_trusted_keyring(const void *data)
{
struct l_keyring *ring;
struct l_keyring *trust;
- uint8_t *cacert;
- size_t cacertlen;
- uint8_t *cert;
- size_t certlen;
+ struct l_cert *cacert;
+ struct l_cert *cert;
struct l_key *cakey;
struct l_key *key;
bool success;
- cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem", &cacertlen);
+ cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
assert(cacert);
- cert = l_pem_load_certificate(CERTDIR "cert-server.pem",
- &certlen);
+ cert = l_pem_load_certificate(CERTDIR "cert-server.pem");
assert(cert);
- cakey = l_key_new(L_KEY_RSA, cacert, cacertlen);
+ cakey = l_cert_get_pubkey(cacert);
assert(cakey);
- key = l_key_new(L_KEY_RSA, cert, certlen);
+ key = l_cert_get_pubkey(cert);
assert(key);
trust = l_keyring_new();
@@ -431,39 +428,34 @@ static void test_trusted_keyring(const void *data)
l_keyring_free(ring);
l_key_free(cakey);
l_key_free(key);
- l_free(cacert);
- l_free(cert);
+ l_cert_free(cacert);
+ l_cert_free(cert);
}
static void test_trust_chain(const void *data)
{
struct l_keyring *ring;
struct l_keyring *trust;
- uint8_t *cacert;
- size_t cacertlen;
- uint8_t *intcert;
- size_t intcertlen;
- uint8_t *cert;
- size_t certlen;
+ struct l_cert *cacert;
+ struct l_cert *intcert;
+ struct l_cert *cert;
struct l_key *cakey;
struct l_key *intkey;
struct l_key *key;
bool success;
- cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem", &cacertlen);
+ cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
assert(cacert);
- intcert = l_pem_load_certificate(CERTDIR "cert-intca.pem",
- &intcertlen);
+ intcert = l_pem_load_certificate(CERTDIR "cert-intca.pem");
assert(intcert);
- cert = l_pem_load_certificate(CERTDIR "cert-entity-int.pem",
- &certlen);
+ cert = l_pem_load_certificate(CERTDIR "cert-entity-int.pem");
assert(cert);
- cakey = l_key_new(L_KEY_RSA, cacert, cacertlen);
+ cakey = l_cert_get_pubkey(cacert);
assert(cakey);
- intkey = l_key_new(L_KEY_RSA, intcert, intcertlen);
+ intkey = l_cert_get_pubkey(intcert);
assert(intkey);
- key = l_key_new(L_KEY_RSA, cert, certlen);
+ key = l_cert_get_pubkey(cert);
assert(key);
trust = l_keyring_new();
@@ -492,18 +484,15 @@ static void test_trust_chain(const void *data)
l_key_free(cakey);
l_key_free(intkey);
l_key_free(key);
- l_free(cacert);
- l_free(intcert);
- l_free(cert);
+ l_cert_free(cacert);
+ l_cert_free(intcert);
+ l_cert_free(cert);
}
static void test_key_crypto(const void *data)
{
- uint8_t *cert;
- size_t certlen;
- uint8_t *pubcert;
- size_t pubcertlen;
- struct l_key *key;
+ struct l_key *privkey;
+ struct l_cert *cert;
struct l_key *pubkey;
bool is_public;
size_t keybits;
@@ -514,24 +503,21 @@ static void test_key_crypto(const void *data)
int hash = L_CHECKSUM_NONE;
int rsa = L_KEY_RSA_PKCS1_V1_5;
- cert = l_pem_load_private_key(CERTDIR "cert-client-key-pkcs8.pem",
- NULL, NULL, &certlen);
+ privkey = l_pem_load_private_key(CERTDIR "cert-client-key-pkcs8.pem",
+ NULL, NULL);
+ assert(privkey);
+ cert = l_pem_load_certificate(CERTDIR "cert-client.pem");
assert(cert);
- pubcert = l_pem_load_certificate(CERTDIR "cert-client.pem",
- &pubcertlen);
- assert(pubcert);
- key = l_key_new(L_KEY_RSA, cert, certlen);
- assert(key);
- pubkey = l_key_new(L_KEY_RSA, pubcert, pubcertlen);
+ pubkey = l_cert_get_pubkey(cert);
assert(pubkey);
- success = l_key_get_info(key, rsa, hash, &keybits, &is_public);
+ success = l_key_get_info(privkey, rsa, hash, &keybits, &is_public);
assert(success);
assert(keybits == 2048);
assert(!is_public);
- success = l_key_get_info(key, rsa, L_CHECKSUM_NONE, &keybits,
+ success = l_key_get_info(privkey, rsa, L_CHECKSUM_NONE, &keybits,
&is_public);
assert(success);
assert(keybits == 2048);
@@ -554,14 +540,14 @@ static void test_key_crypto(const void *data)
sizeof(ciphertext), sizeof(decrypted));
assert(len < 0);
- len = l_key_decrypt(key, rsa, hash, ciphertext, decrypted,
+ len = l_key_decrypt(privkey, rsa, hash, ciphertext, decrypted,
sizeof(ciphertext), sizeof(decrypted));
assert(len == (ssize_t)strlen(plaintext));
assert(strcmp(plaintext, (char *)decrypted) == 0);
/* Decrypt reference ciphertext */
memset(decrypted, 0, sizeof(decrypted));
- len = l_key_decrypt(key, rsa, hash, reference_ciphertext, decrypted,
+ len = l_key_decrypt(privkey, rsa, hash, reference_ciphertext, decrypted,
sizeof(reference_ciphertext),
sizeof(decrypted));
assert(len == (ssize_t)strlen(plaintext));
@@ -571,7 +557,7 @@ static void test_key_crypto(const void *data)
memset(decrypted, 0, sizeof(decrypted));
memcpy(ciphertext, reference_ciphertext, sizeof(ciphertext));
ciphertext[0] = ciphertext[0] ^ (uint8_t)0xFF;
- len = l_key_decrypt(key, rsa, hash, ciphertext, decrypted,
+ len = l_key_decrypt(privkey, rsa, hash, ciphertext, decrypted,
sizeof(ciphertext),
sizeof(decrypted));
assert(len < 0);
@@ -581,7 +567,7 @@ static void test_key_crypto(const void *data)
strlen(plaintext), sizeof(ciphertext));
assert(len < 0);
- len = l_key_sign(key, rsa, hash, plaintext, ciphertext,
+ len = l_key_sign(privkey, rsa, hash, plaintext, ciphertext,
strlen(plaintext), sizeof(ciphertext));
assert(len == sizeof(ciphertext));
@@ -589,7 +575,7 @@ static void test_key_crypto(const void *data)
strlen(plaintext), sizeof(ciphertext));
assert(success);
- success = l_key_verify(key, rsa, hash, plaintext, ciphertext,
+ success = l_key_verify(privkey, rsa, hash, plaintext, ciphertext,
strlen(plaintext), sizeof(ciphertext));
assert(success);
@@ -600,14 +586,13 @@ static void test_key_crypto(const void *data)
/* Corrupt signature */
ciphertext[42] = ciphertext[52] ^ (uint8_t)0xFF;
- success = l_key_verify(key, rsa, hash, plaintext, ciphertext,
+ success = l_key_verify(privkey, rsa, hash, plaintext, ciphertext,
strlen(plaintext), sizeof(ciphertext));
assert(!success);
- l_key_free(key);
+ l_key_free(privkey);
l_key_free(pubkey);
- l_free(cert);
- l_free(pubcert);
+ l_cert_free(cert);
}
int main(int argc, char *argv[])
diff --git a/unit/test-pem.c b/unit/test-pem.c
index 3325e54..b37a29e 100644
--- a/unit/test-pem.c
+++ b/unit/test-pem.c
@@ -91,36 +91,47 @@ static void test_encrypted_pkey(const void *data)
{
const char *encrypted_pem = data;
const char *plaintext_pem = CERTDIR "cert-client-key-pkcs8.pem";
- bool encrypted;
- size_t size1, size2;
- uint8_t *pkey1, *pkey2;
+ bool is_encrypted;
+ size_t size;
+ uint8_t encrypted1[256], encrypted2[256], plaintext[256];
+ struct l_key *pkey1, *pkey2;
+ bool is_public;
- encrypted = false;
- assert(!l_pem_load_private_key(encrypted_pem, NULL,
- &encrypted, &size1));
- assert(encrypted);
+ is_encrypted = false;
+ assert(!l_pem_load_private_key(encrypted_pem, NULL, &is_encrypted));
+ assert(is_encrypted);
- encrypted = false;
+ is_encrypted = false;
assert(!l_pem_load_private_key(encrypted_pem, "wrong-passwd",
- &encrypted, &size1));
- assert(encrypted);
+ &is_encrypted));
+ assert(is_encrypted);
- encrypted = false;
- pkey1 = l_pem_load_private_key(encrypted_pem, "abc",
- &encrypted, &size1);
+ is_encrypted = false;
+ pkey1 = l_pem_load_private_key(encrypted_pem, "abc", &is_encrypted);
assert(pkey1);
- assert(encrypted);
+ assert(is_encrypted);
- pkey2 = l_pem_load_private_key(plaintext_pem, NULL,
- &encrypted, &size2);
+ pkey2 = l_pem_load_private_key(plaintext_pem, NULL, &is_encrypted);
assert(pkey2);
- assert(!encrypted);
-
- assert(size1 == size2);
- assert(!memcmp(pkey1, pkey2, size1));
-
- l_free(pkey1);
- l_free(pkey2);
+ assert(!is_encrypted);
+
+ /*
+ * l_key_extract doesn't work for private keys so compare encrypt
+ * results instead of key exponent.
+ */
+ memset(plaintext, 42, 256);
+ assert(l_key_get_info(pkey1, L_KEY_RSA_RAW, L_CHECKSUM_NONE,
+ &size, &is_public));
+ assert(size == 2048);
+ assert(!is_public);
+ assert(l_key_encrypt(pkey1, L_KEY_RSA_RAW, L_CHECKSUM_NONE,
+ plaintext, encrypted1, 256, 256) == 256);
+ assert(l_key_encrypt(pkey2, L_KEY_RSA_RAW, L_CHECKSUM_NONE,
+ plaintext, encrypted2, 256, 256) == 256);
+ assert(!memcmp(encrypted1, encrypted2, 256));
+
+ l_key_free(pkey1);
+ l_key_free(pkey2);
}
int main(int argc, char *argv[])
diff --git a/unit/test-tls.c b/unit/test-tls.c
index d969ee7..5dd6569 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -213,28 +213,28 @@ static void test_tls12_prf(const void *data)
static void test_certificates(const void *data)
{
- struct tls_cert *cert;
- struct tls_cert *cacert;
- struct tls_cert *wrongca;
+ struct l_cert *cert;
+ struct l_cert *cacert;
+ struct l_cert *wrongca;
- cert = tls_cert_load_file(CERTDIR "cert-server.pem");
+ cert = l_pem_load_certificate(CERTDIR "cert-server.pem");
assert(cert);
- cacert = tls_cert_load_file(CERTDIR "cert-ca.pem");
+ cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
assert(cacert);
- wrongca = tls_cert_load_file(CERTDIR "cert-intca.pem");
+ wrongca = l_pem_load_certificate(CERTDIR "cert-intca.pem");
assert(wrongca);
- assert(!tls_cert_verify_certchain(cert, wrongca));
+ assert(!l_cert_verify_certchain(cert, wrongca));
- assert(tls_cert_verify_certchain(cert, cacert));
+ assert(l_cert_verify_certchain(cert, cacert));
- assert(tls_cert_verify_certchain(cert, NULL));
+ assert(l_cert_verify_certchain(cert, NULL));
- l_free(cert);
- l_free(cacert);
- l_free(wrongca);
+ l_cert_free(cert);
+ l_cert_free(cacert);
+ l_cert_free(wrongca);
}
struct tls_conn_test {
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] cert: Add cert.c for certificate types and routines
2018-11-08 12:06 [PATCH 1/5] cert: Add cert.c for certificate types and routines Andrew Zaborowski
` (3 preceding siblings ...)
2018-11-08 12:06 ` [PATCH 5/5] unit: Update tests " Andrew Zaborowski
@ 2018-11-08 17:39 ` Denis Kenzior
2018-11-08 23:22 ` Andrew Zaborowski
4 siblings, 1 reply; 14+ messages in thread
From: Denis Kenzior @ 2018-11-08 17:39 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 12107 bytes --]
Hi Andrew,
On 11/08/2018 06:06 AM, Andrew Zaborowski wrote:
> Create a new file for all the generic X.509 certificate utilities so far
> living in tls.c and make the l_cert type a public object. There are
> only minor changes from the tls.c code to add accessors, otherwise the
> intention is not to make the code perfect and only move the existing
> code so that for example pem.c can start returning l_cert objects
> directly.
> ---
> Makefile.am | 6 +-
> ell/cert.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> ell/cert.h | 57 ++++++++++
> ell/ell.h | 1 +
> ell/ell.sym | 8 ++
> 5 files changed, 376 insertions(+), 2 deletions(-)
> create mode 100644 ell/cert.c
> create mode 100644 ell/cert.h
>
<snip>
> diff --git a/ell/cert.c b/ell/cert.c
> new file mode 100644
> index 0000000..82ea005
> --- /dev/null
> +++ b/ell/cert.c
> @@ -0,0 +1,306 @@
> +/*
> + * Embedded Linux library
> + *
> + * Copyright (C) 2018 Intel Corporation. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <string.h>
> +
> +#include "private.h"
> +#include "key.h"
> +#include "asn1-private.h"
> +#include "cert.h"
> +
> +#define X509_CERTIFICATE_POS 0
> +#define X509_TBSCERTIFICATE_POS 0
> +#define X509_TBSCERT_VERSION_POS 0
> +#define X509_TBSCERT_SERIAL_POS 1
> +#define X509_TBSCERT_SIGNATURE_POS 2
> +#define X509_ALGORITHM_ID_ALGORITHM_POS 0
> +#define X509_ALGORITHM_ID_PARAMS_POS 1
> +#define X509_TBSCERT_ISSUER_DN_POS 3
> +#define X509_TBSCERT_VALIDITY_POS 4
> +#define X509_TBSCERT_SUBJECT_DN_POS 5
> +#define X509_TBSCERT_SUBJECT_KEY_POS 6
> +#define X509_SUBJECT_KEY_ALGORITHM_POS 0
> +#define X509_SUBJECT_KEY_VALUE_POS 1
> +#define X509_TBSCERT_ISSUER_UID_POS 7
> +#define X509_TBSCERT_SUBJECT_UID_POS 8
> +#define X509_TBSCERT_EXTENSIONS_POS 9
> +#define X509_SIGNATURE_ALGORITHM_POS 1
> +#define X509_SIGNATURE_VALUE_POS 2
> +
> +struct l_cert {
> + struct l_cert *issuer;
> + struct l_cert *issued;
So I don't really like this dual-purpose structure. l_cert is used both
as a standalone certificate and a certchain, and that creates weird
situations where the caller isn't quite sure when l_cert_free should be
called, especially when l_cert_link* is involved. Not without looking
at the implementation anyway, and that's undesirable from an API
simplicity / nice-to-use perspective.
> + enum l_cert_key_type pubkey_type;
> + size_t asn1_len;
> + uint8_t asn1[0];
> +};
> +
> +static const struct pkcs1_encryption_oid {
> + enum l_cert_key_type key_type;
> + struct asn1_oid oid;
> +} pkcs1_encryption_oids[] = {
> + { /* rsaEncryption */
> + L_CERT_KEY_RSA,
> + { 9, { 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01 } },
> + },
> +};
> +
> +static bool cert_set_pubkey_type(struct l_cert *cert)
> +{
> + const uint8_t *key_type;
> + size_t key_type_len;
> + int i;
> +
> + key_type = asn1_der_find_elem_by_path(cert->asn1, cert->asn1_len,
> + ASN1_ID_OID, &key_type_len,
> + X509_CERTIFICATE_POS,
> + X509_TBSCERTIFICATE_POS,
> + X509_TBSCERT_SUBJECT_KEY_POS,
> + X509_SUBJECT_KEY_ALGORITHM_POS,
> + X509_ALGORITHM_ID_ALGORITHM_POS,
> + -1);
> + if (!key_type)
> + return false;
> +
> + for (i = 0; i < (int) L_ARRAY_SIZE(pkcs1_encryption_oids); i++)
> + if (asn1_oid_eq(&pkcs1_encryption_oids[i].oid,
> + key_type_len, key_type))
> + break;
So the intent of l_cert is to only store public keys, right? Not
private keys.
> +
> + if (i == L_ARRAY_SIZE(pkcs1_encryption_oids))
> + cert->pubkey_type = L_CERT_KEY_UNKNOWN;
> + else
> + cert->pubkey_type = pkcs1_encryption_oids[i].key_type;
It is unclear to me how you're making sure pubkey_type enumeration
actually matches up to key_type. There's no initializer on the enum, so
the compiler can be free to choose anything?
> +
> + return true;
> +}
> +
> +LIB_EXPORT struct l_cert *l_cert_new_from_der(const uint8_t *buf,
> + size_t buf_len)
> +{
> + const uint8_t *seq = buf;
> + size_t seq_len = buf_len;
> + size_t content_len;
> + struct l_cert *cert;
> +
> + /* Sanity check: outer element is a SEQUENCE */
> + if (seq_len-- < 1 || *seq++ != ASN1_ID_SEQUENCE)
> + return NULL;
> +
> + /* Sanity check: the SEQUENCE spans the whole buffer */
> + content_len = asn1_parse_definite_length(&seq, &seq_len);
Is it me, or is asn1_parse_definite_length not doing proper boundary
checking? For example, if buf_len is 1, then doesn't this blow up?
> + if (content_len < 64 || content_len != seq_len)
> + return NULL;
> +
> + /*
> + * We could require the signature algorithm and the key algorithm
> + * to be one of our supported types here but instead we only
> + * require that when the user wants to verify this certificate or
> + * get the public key respectively.
> + */
> +
> + cert = l_malloc(sizeof(struct l_cert) + buf_len);
> + cert->issuer = NULL;
> + cert->issued = NULL;
> + cert->asn1_len = buf_len;
> + memcpy(cert->asn1, buf, buf_len);
> +
> + /* Sanity check: structure is correct up to the Public Key Algorithm */
> + if (!cert_set_pubkey_type(cert)) {
> + l_free(cert);
> + return NULL;
> + }
> +
> + return cert;
> +}
> +
> +LIB_EXPORT void l_cert_free(struct l_cert *cert)
> +{
> + while (cert) {
> + struct l_cert *next = cert->issuer;
> +
> + l_free(cert);
> + cert = next;
> + }
> +}
> +
> +LIB_EXPORT bool l_cert_link_issuer(struct l_cert *cert, struct l_cert *issuer)
> +{
> + if (!cert)
> + return false;
> +
> + if (issuer && (issuer->issued || cert->issuer))
> + return false;
> +
> + if (cert->issuer)
> + cert->issuer->issued = NULL;
> +
> + cert->issuer = issuer;
> + return true;
> +}
So it is completely unclear to the caller that l_cert_link_issuer takes
ownership of cert. The caller can mistakenly call l_cert_free on the
original cert and get a fun result...
> +
> +LIB_EXPORT struct l_cert *l_cert_get_issuer(struct l_cert *cert)
> +{
> + return cert->issuer;
> +}
> +
> +LIB_EXPORT const uint8_t *l_cert_get_der_data(struct l_cert *cert,
> + size_t *out_len)
> +{
> + *out_len = cert->asn1_len;
> + return cert->asn1;
> +}
Personally I would just do something like:
struct l_cert {
enum l_cert_key_type pubkey_type;
size_t asn1_len;
uint8_t asn1[0];
};
struct certchain_link {
struct l_cert cert;
struct certchain_link *issuer;
struct certchain_link *child;
};
struct l_certchain {
struct certchain_link *root;
struct certchain_link *key;
};
and consider dropping link_issuer completely. Just build a certchain
from the raw data directly, perhaps as a private constructor for now.
> +
> +LIB_EXPORT bool l_cert_find_certchain(struct l_cert *cert,
> + struct l_cert *ca_cert)
> +{
> + return true;
> +}
> +
> +static void cert_key_cleanup(struct l_key **p)
> +{
> + l_key_free_norevoke(*p);
> +}
> +
> +static bool cert_verify_with_keyring(struct l_cert *cert,
> + struct l_keyring *ring,
> + struct l_cert *root,
> + struct l_keyring *trusted)
> +{
> + if (!cert)
> + return true;
> +
> + if (cert->pubkey_type != L_CERT_KEY_RSA)
> + return false;
> +
> + /*
> + * RFC5246 7.4.2:
> + * "Because certificate validation requires that root keys be
> + * distributed independently, the self-signed certificate that
> + * specifies the root certificate authority MAY be omitted from
> + * the chain, under the assumption that the remote end must
> + * already possess it in order to validate it in any case."
> + */
> + if (!cert->issuer && root && cert->asn1_len == root->asn1_len &&
> + !memcmp(cert->asn1, root->asn1, root->asn1_len))
> + return true;
> +
> + if (cert_verify_with_keyring(cert->issuer, ring, root, trusted)) {
> + L_AUTO_CLEANUP_VAR(struct l_key *, key, cert_key_cleanup);
> +
> + key = l_key_new(L_KEY_RSA, cert->asn1, cert->asn1_len);
> + if (!key)
> + return false;
> +
> + if (!l_keyring_link(ring, key))
> + return false;
> +
> + if (trusted || cert->issuer)
> + return true;
> +
> + /*
> + * If execution reaches this point, it's known that:
> + * * No trusted root key was supplied, so the chain is only
> + * being checked against its own root
> + * * The keyring 'ring' is not restricted yet
> + * * The chain's root cert was just linked in to the
> + * previously empty keyring 'ring'.
> + *
> + * By restricting 'ring' now, the rest of the certs in
> + * the chain will have their signature validated using 'key'
> + * as the root.
> + */
> + return l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN,
> + trusted);
> + }
> +
> + return false;
So we're still doing the recursive verify. It might makes things
clearer if you added l_cert* stub classes first. Then a separate commit
moving the recursive implementation from tls.c to cert.c. Then a
transform from recursive to iterative can be done at a later date.
> +}
> +
> +static void cert_keyring_cleanup(struct l_keyring **p)
> +{
> + l_keyring_free(*p);
> +}
> +
> +LIB_EXPORT bool l_cert_verify_certchain(struct l_cert *certchain,
> + struct l_cert *ca_cert)
> +{
> + L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring,
> + cert_keyring_cleanup) = NULL;
> + L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
> + cert_keyring_cleanup) = NULL;
> +
> + if (!certchain || certchain->pubkey_type != L_CERT_KEY_RSA)
> + return false;
> +
> + if (ca_cert && ca_cert->pubkey_type != L_CERT_KEY_RSA)
> + return false;
> +
> + if (ca_cert) {
> + L_AUTO_CLEANUP_VAR(struct l_key *, ca_key, cert_key_cleanup);
> + ca_key = NULL;
> +
> + ca_ring = l_keyring_new();
> + if (!ca_ring)
> + return false;
> +
> + ca_key = l_key_new(L_KEY_RSA, ca_cert->asn1, ca_cert->asn1_len);
> + if (!ca_key || !l_keyring_link(ca_ring, ca_key))
> + return false;
> + }
> +
> + verify_ring = l_keyring_new();
> + if (!verify_ring)
> + return false;
> +
> + /*
> + * If a CA cert was supplied, restrict verify_ring now so
> + * everything else in certchain is validated against the CA.
> + * Otherwise, verify_ring will be restricted after the root of
> + * certchain is added to verify_ring by
> + * cert_verify_with_keyring().
> + */
> + if (ca_ring && !l_keyring_restrict(verify_ring,
> + L_KEYRING_RESTRICT_ASYM_CHAIN,
> + ca_ring)) {
> + return false;
> + }
> +
> + return cert_verify_with_keyring(certchain, verify_ring, ca_cert,
> + ca_ring);
> +}
> +
> +LIB_EXPORT enum l_cert_key_type l_cert_get_pubkey_type(struct l_cert *cert)
> +{
> + return cert->pubkey_type;
> +}
> +
> +LIB_EXPORT struct l_key *l_cert_get_pubkey(struct l_cert *cert)
> +{
> + /* Use kernel's ASN.1 certificate parser to find the key data for us */
> + if (cert->pubkey_type == L_CERT_KEY_RSA)
> + return l_key_new(L_KEY_RSA, cert->asn1, cert->asn1_len);
> +
> + return NULL;
> +}
Regards,
-Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code
2018-11-08 12:06 ` [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code Andrew Zaborowski
@ 2018-11-08 23:20 ` Mat Martineau
0 siblings, 0 replies; 14+ messages in thread
From: Mat Martineau @ 2018-11-08 23:20 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 26078 bytes --]
Andrew -
On Thu, 8 Nov 2018, Andrew Zaborowski wrote:
> Drop the struct tls_cert utilities and use the l_cert type for all the
> certificate operations. tls_cert_from_certificate_list is renamed to
> tls_parse_certificate_list and moved up in the file. l_tls_set_cacert
> return type is changed to bool so that errors in loading CA certificate
> are returned immediately instead of failing the handshake later on.
> Since this just changes a void to a bool it shouldn't break ell user
> builds. l_pem_load_certificate is changed to return the l_cert object
> instead of a byte buffer so there's no longer to call the l_cert
> constructor separately.
> ---
> ell/pem.c | 8 +-
> ell/pem.h | 4 +-
> ell/tls-private.h | 38 +---
> ell/tls-record.c | 1 +
> ell/tls.c | 455 +++++++++++++---------------------------------
> ell/tls.h | 2 +-
> 6 files changed, 138 insertions(+), 370 deletions(-)
>
> diff --git a/ell/pem.c b/ell/pem.c
> index e24dd7d..f4aa7e2 100644
> --- a/ell/pem.c
> +++ b/ell/pem.c
> @@ -213,13 +213,13 @@ LIB_EXPORT uint8_t *l_pem_load_file(const char *filename, int index,
> return result;
> }
>
> -LIB_EXPORT uint8_t *l_pem_load_certificate(const char *filename, size_t *len)
> +LIB_EXPORT struct l_cert *l_pem_load_certificate(const char *filename)
> {
> uint8_t *content;
> char *label;
> + size_t len;
>
> - content = l_pem_load_file(filename, 0, &label, len);
> -
> + content = l_pem_load_file(filename, 0, &label, &len);
> if (!content)
> return NULL;
>
> @@ -230,7 +230,7 @@ LIB_EXPORT uint8_t *l_pem_load_certificate(const char *filename, size_t *len)
>
> l_free(label);
>
> - return content;
> + return l_cert_new_from_der(content, len);
Valgrind complains about test-key (with the full series applied) - looks
like you need to free 'content' before returning.
Mat
> }
>
> /**
> diff --git a/ell/pem.h b/ell/pem.h
> index 93282a3..9629ef2 100644
> --- a/ell/pem.h
> +++ b/ell/pem.h
> @@ -27,12 +27,14 @@
> extern "C" {
> #endif
>
> +#include <ell/cert.h>
> +
> uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
> int index, char **type_label, size_t *out_len);
> uint8_t *l_pem_load_file(const char *filename, int index,
> char **type_label, size_t *len);
>
> -uint8_t *l_pem_load_certificate(const char *filename, size_t *len);
> +struct l_cert *l_pem_load_certificate(const char *filename);
>
> uint8_t *l_pem_load_private_key(const char *filename, const char *passphrase,
> bool *encrypted, size_t *len);
> diff --git a/ell/tls-private.h b/ell/tls-private.h
> index 4f680a0..2257041 100644
> --- a/ell/tls-private.h
> +++ b/ell/tls-private.h
> @@ -28,8 +28,6 @@
> #define TLS_VERSION TLS_V12
> #define TLS_MIN_VERSION TLS_V10
>
> -struct tls_cert;
> -
> enum tls_cipher_type {
> TLS_CIPHER_STREAM,
> TLS_CIPHER_BLOCK,
> @@ -60,7 +58,7 @@ struct tls_key_exchange_algorithm {
>
> bool certificate_check;
>
> - bool (*validate_cert_key_type)(struct tls_cert *cert);
> + bool (*validate_cert_key_type)(struct l_cert *cert);
>
> bool (*send_client_key_exchange)(struct l_tls *tls);
> void (*handle_client_key_exchange)(struct l_tls *tls,
> @@ -142,8 +140,8 @@ struct l_tls {
> l_tls_destroy_cb_t debug_destroy;
> void *debug_data;
>
> - char *ca_cert_path;
> - char *cert_path;
> + struct l_cert *ca_cert;
> + struct l_cert *cert;
> struct l_key *priv_key;
> size_t priv_key_size;
>
> @@ -169,7 +167,7 @@ struct l_tls {
> uint16_t negotiated_version;
> bool cert_requested, cert_sent;
> bool peer_authenticated;
> - struct tls_cert *peer_cert;
> + struct l_cert *peer_cert;
> struct l_key *peer_pubkey;
> size_t peer_pubkey_size;
> enum handshake_hash_type signature_hash;
> @@ -226,32 +224,8 @@ void tls_tx_record(struct l_tls *tls, enum tls_content_type type,
> bool tls_handle_message(struct l_tls *tls, const uint8_t *message,
> int len, enum tls_content_type type, uint16_t version);
>
> -/* X509 Certificates and Certificate Chains */
> -
> -struct tls_cert {
> - size_t size;
> - struct tls_cert *issuer;
> - uint8_t asn1[0];
> -};
> -
> -enum tls_cert_key_type {
> - TLS_CERT_KEY_RSA,
> - TLS_CERT_KEY_UNKNOWN,
> -};
> -
> -struct tls_cert *tls_cert_load_file(const char *filename);
> -int tls_cert_from_certificate_list(const void *data, size_t len,
> - struct tls_cert **out_certchain);
> -
> -bool tls_cert_find_certchain(struct tls_cert *cert,
> - const char *cacert_filename);
> -
> -bool tls_cert_verify_certchain(struct tls_cert *certchain,
> - struct tls_cert *ca_cert);
> -
> -void tls_cert_free_certchain(struct tls_cert *cert);
> -
> -enum tls_cert_key_type tls_cert_get_pubkey_type(struct tls_cert *cert);
> +int tls_parse_certificate_list(const void *data, size_t len,
> + struct l_cert **out_certchain);
>
> void tls_prf_get_bytes(struct l_tls *tls,
> enum l_checksum_type type, size_t hash_len,
> diff --git a/ell/tls-record.c b/ell/tls-record.c
> index 8038049..410f194 100644
> --- a/ell/tls-record.c
> +++ b/ell/tls-record.c
> @@ -29,6 +29,7 @@
> #include "tls.h"
> #include "checksum.h"
> #include "cipher.h"
> +#include "cert.h"
> #include "tls-private.h"
> #include "random.h"
>
> diff --git a/ell/tls.c b/ell/tls.c
> index 4b37785..6bea5dc 100644
> --- a/ell/tls.c
> +++ b/ell/tls.c
> @@ -35,6 +35,7 @@
> #include "cipher.h"
> #include "random.h"
> #include "pem.h"
> +#include "cert.h"
> #include "tls-private.h"
> #include "key.h"
> #include "asn1-private.h"
> @@ -176,7 +177,7 @@ static void tls_reset_handshake(struct l_tls *tls)
>
> memset(tls->pending.key_block, 0, sizeof(tls->pending.key_block));
>
> - l_free(tls->peer_cert);
> + l_cert_free(tls->peer_cert);
> l_key_free(tls->peer_pubkey);
>
> tls->peer_cert = NULL;
> @@ -336,9 +337,9 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
> static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
> tls_get_hash_t get_hash);
>
> -static bool tls_rsa_validate_cert_key(struct tls_cert *cert)
> +static bool tls_rsa_validate_cert_key(struct l_cert *cert)
> {
> - return tls_cert_get_pubkey_type(cert) == TLS_CERT_KEY_RSA;
> + return l_cert_get_pubkey_type(cert) == L_CERT_KEY_RSA;
> }
>
> static struct tls_key_exchange_algorithm tls_rsa = {
> @@ -611,6 +612,55 @@ static void pkcs1_write_digest_info(enum l_checksum_type type,
> *out_len += hash_len;
> }
>
> +int tls_parse_certificate_list(const void *data, size_t len,
> + struct l_cert **out_certchain)
> +{
> + const uint8_t *buf = data;
> + struct l_cert *certchain = NULL, *tail;
> +
> + while (len) {
> + struct l_cert *cert;
> + size_t cert_len;
> +
> + if (len < 3)
> + goto decode_error;
> +
> + cert_len = *buf++ << 16;
> + cert_len |= *buf++ << 8;
> + cert_len |= *buf++ << 0;
> +
> + if (cert_len + 3 > len)
> + goto decode_error;
> +
> + cert = l_cert_new_from_der(buf, cert_len);
> + if (!cert)
> + goto decode_error;
> +
> + if (!certchain)
> + certchain = cert;
> + else if (!l_cert_link_issuer(tail, cert)) {
> + l_cert_free(cert);
> + goto decode_error;
> + }
> +
> + tail = cert;
> +
> + buf += cert_len;
> + len -= cert_len + 3;
> + }
> +
> + if (out_certchain)
> + *out_certchain = certchain;
> + else
> + l_cert_free(certchain);
> +
> + return 0;
> +
> +decode_error:
> + l_cert_free(certchain);
> + return -EBADMSG;
> +}
> +
> static void tls_send_alert(struct l_tls *tls, bool fatal,
> enum l_tls_alert_desc alert_desc)
> {
> @@ -743,36 +793,25 @@ static void tls_send_server_hello(struct l_tls *tls)
> static bool tls_send_certificate(struct l_tls *tls)
> {
> uint8_t *buf, *ptr;
> - struct tls_cert *cert, *i;
> + struct l_cert *i;
> size_t total;
>
> - if (tls->cert_path)
> - cert = tls_cert_load_file(tls->cert_path);
> - else
> - cert = NULL;
> -
> - if (tls->server && !cert) {
> + if (tls->server && !tls->cert) {
> TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
> - "Loading server certificate %s failed",
> - tls->cert_path);
> + "Certificate needed in server mode");
> return false;
> }
>
> - if (cert && !tls_cert_find_certchain(cert, tls->ca_cert_path)) {
> - if (tls->server) {
> - TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
> - TLS_ALERT_UNKNOWN_CA,
> - "Can't find certificate chain local "
> - "CA cert %s", tls->ca_cert_path);
> -
> - return false;
> - } else
> - cert = NULL;
> + if (tls->cert && !l_cert_find_certchain(tls->cert, tls->ca_cert)) {
> + TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_UNKNOWN_CA,
> + "Can't find certificate chain to local "
> + "CA cert");
> + return false;
> }
>
> /* TODO: might want check this earlier and exclude the cipher suite */
> - if (cert && !tls->pending.cipher_suite->key_xchg->
> - validate_cert_key_type(cert)) {
> + if (tls->cert && !tls->pending.cipher_suite->key_xchg->
> + validate_cert_key_type(tls->cert)) {
> TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_CERT_UNKNOWN,
> "Local certificate has key type incompatible "
> "with pending cipher suite %s",
> @@ -799,8 +838,12 @@ static bool tls_send_certificate(struct l_tls *tls)
> */
>
> total = 0;
> - for (i = cert; i; i = i->issuer)
> - total += 3 + i->size;
> + for (i = tls->cert; i; i = l_cert_get_issuer(i)) {
> + size_t der_len;
> +
> + l_cert_get_der_data(i, &der_len);
> + total += 3 + der_len;
> + }
>
> buf = l_malloc(128 + total);
> ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
> @@ -811,24 +854,26 @@ static bool tls_send_certificate(struct l_tls *tls)
> *ptr++ = total >> 8;
> *ptr++ = total >> 0;
>
> - for (i = cert; i; i = i->issuer) {
> - *ptr++ = i->size >> 16;
> - *ptr++ = i->size >> 8;
> - *ptr++ = i->size >> 0;
> + for (i = tls->cert; i; i = l_cert_get_issuer(i)) {
> + const uint8_t *der;
> + size_t der_len;
>
> - memcpy(ptr, i->asn1, i->size);
> - ptr += i->size;
> + der = l_cert_get_der_data(i, &der_len);
> + *ptr++ = der_len >> 16;
> + *ptr++ = der_len >> 8;
> + *ptr++ = der_len >> 0;
> +
> + memcpy(ptr, der, der_len);
> + ptr += der_len;
> }
>
> tls_tx_handshake(tls, TLS_CERTIFICATE, buf, ptr - buf);
>
> l_free(buf);
>
> - if (cert)
> + if (tls->cert)
> tls->cert_sent = true;
>
> - l_free(cert);
> -
> return true;
> }
>
> @@ -1413,7 +1458,7 @@ static void tls_handle_client_hello(struct l_tls *tls,
> * TODO: filter supported cipher suites by the certificate/key
> * type that was submitted by tls_set_auth_data() if any.
> * Perhaps just call cipher_suite->verify_cert_type() on each
> - * cipher suite passing a pre-parsed certificate ASN.1 struct.
> + * cipher suite passing tls->cert.
> */
> tls->pending.cipher_suite =
> tls_find_cipher_suite(cipher_suites);
> @@ -1457,21 +1502,20 @@ static void tls_handle_client_hello(struct l_tls *tls,
>
> tls_send_server_hello(tls);
>
> - if (tls->pending.cipher_suite->key_xchg->certificate_check &&
> - tls->cert_path)
> + if (tls->pending.cipher_suite->key_xchg->certificate_check && tls->cert)
> if (!tls_send_certificate(tls))
> return;
>
> /* TODO: don't bother if configured to not authenticate client */
> if (tls->pending.cipher_suite->key_xchg->certificate_check &&
> - tls->ca_cert_path)
> + tls->ca_cert)
> if (!tls_send_certificate_request(tls))
> return;
>
> tls_send_server_hello_done(tls);
>
> if (tls->pending.cipher_suite->key_xchg->certificate_check &&
> - tls->ca_cert_path)
> + tls->ca_cert)
> TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE);
> else
> TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
> @@ -1570,8 +1614,7 @@ static void tls_handle_certificate(struct l_tls *tls,
> const uint8_t *buf, size_t len)
> {
> size_t total;
> - struct tls_cert *certchain = NULL;
> - struct tls_cert *ca_cert = NULL;
> + struct l_cert *certchain = NULL;
> bool dummy;
>
> if (len < 3)
> @@ -1584,7 +1627,7 @@ static void tls_handle_certificate(struct l_tls *tls,
> if (total + 3 != len)
> goto decode_error;
>
> - if (tls_cert_from_certificate_list(buf, total, &certchain) < 0) {
> + if (tls_parse_certificate_list(buf, total, &certchain) < 0) {
> TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
> "Error decoding peer certificate chain");
>
> @@ -1616,22 +1659,11 @@ static void tls_handle_certificate(struct l_tls *tls,
> * Validate the certificate chain's consistency and validate it
> * against our CA if we have any.
> */
> -
> - if (tls->ca_cert_path) {
> - ca_cert = tls_cert_load_file(tls->ca_cert_path);
> - if (!ca_cert) {
> - TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
> - TLS_ALERT_BAD_CERT,
> - "Can't load %s", tls->ca_cert_path);
> -
> - goto done;
> - }
> - }
> -
> - if (!tls_cert_verify_certchain(certchain, ca_cert)) {
> + if (!l_cert_verify_certchain(certchain, tls->ca_cert)) {
> TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
> - "Peer certchain verification failed against "
> - "CA cert %s", tls->ca_cert_path);
> + "Peer certchain verification failed "
> + "consistency check%s",
> + tls->ca_cert ? " or against local CA cert" : "");
>
> goto done;
> }
> @@ -1654,13 +1686,11 @@ static void tls_handle_certificate(struct l_tls *tls,
>
> /* Save the end-entity cert and free the rest of the chain */
> tls->peer_cert = certchain;
> - tls_cert_free_certchain(certchain->issuer);
> - certchain->issuer = NULL;
> - certchain = NULL;
> -
> - tls->peer_pubkey = l_key_new(L_KEY_RSA, tls->peer_cert->asn1,
> - tls->peer_cert->size);
> + certchain = l_cert_get_issuer(certchain);
> + if (certchain)
> + l_cert_link_issuer(tls->peer_cert, NULL);
>
> + tls->peer_pubkey = l_cert_get_pubkey(tls->peer_cert);
> if (!tls->peer_pubkey) {
> TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
> "Error loading peer public key to kernel");
> @@ -1691,10 +1721,7 @@ decode_error:
> "TLS_CERTIFICATE decode error");
>
> done:
> - if (ca_cert)
> - l_free(ca_cert);
> -
> - tls_cert_free_certchain(certchain);
> + l_cert_free(certchain);
> }
>
> static void tls_handle_certificate_request(struct l_tls *tls,
> @@ -1946,8 +1973,8 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
> * - If we received an (expected) Certificate Verify, we must have
> * sent a Certificate Request.
> * - If we sent a Certificate Request that's because
> - * tls->ca_cert_path is non-NULL.
> - * - If tls->ca_cert_path is non-NULL then tls_handle_certificate
> + * tls->ca_cert is non-NULL.
> + * - If tls->ca_cert is non-NULL then tls_handle_certificate
> * will have checked the whole certificate chain to be valid and
> * additionally trusted by our CA if known.
> * - Additionally cipher_suite->key_xchg->verify has just confirmed
> @@ -2159,7 +2186,7 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
> /*
> * On the client, the server's certificate is only now
> * verified, based on the following logic:
> - * - tls->ca_cert_path is non-NULL so tls_handle_certificate
> + * - tls->ca_cert is non-NULL so tls_handle_certificate
> * (always called on the client) must have veritifed the
> * server's certificate chain to be valid and additionally
> * trusted by our CA.
> @@ -2172,8 +2199,7 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
> * message (either should be enough).
> */
> if (!tls->server && tls->cipher_suite[0]->key_xchg->
> - certificate_check &&
> - tls->ca_cert_path)
> + certificate_check && tls->ca_cert)
> tls->peer_authenticated = true;
>
> tls_finished(tls);
> @@ -2400,17 +2426,24 @@ LIB_EXPORT void l_tls_close(struct l_tls *tls)
> TLS_DISCONNECT(TLS_ALERT_CLOSE_NOTIFY, 0, "Closing session");
> }
>
> -LIB_EXPORT void l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path)
> +LIB_EXPORT bool l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path)
> {
> TLS_DEBUG("ca-cert-path=%s", ca_cert_path);
>
> - if (tls->ca_cert_path) {
> - l_free(tls->ca_cert_path);
> - tls->ca_cert_path = NULL;
> + if (tls->ca_cert) {
> + l_cert_free(tls->ca_cert);
> + tls->ca_cert = NULL;
> }
>
> - if (ca_cert_path)
> - tls->ca_cert_path = l_strdup(ca_cert_path);
> + if (ca_cert_path) {
> + tls->ca_cert = l_pem_load_certificate(ca_cert_path);
> + if (!tls->ca_cert) {
> + TLS_DEBUG("Error loading %s", ca_cert_path);
> + return false;
> + }
> + }
> +
> + return true;
> }
>
> LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
> @@ -2420,9 +2453,9 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
> TLS_DEBUG("cert-path=%s priv-key-path=%s priv-key-passphrase=%p",
> cert_path, priv_key_path, priv_key_passphrase);
>
> - if (tls->cert_path) {
> - l_free(tls->cert_path);
> - tls->cert_path = NULL;
> + if (tls->cert) {
> + l_cert_free(tls->cert);
> + tls->cert = NULL;
> }
>
> if (tls->priv_key) {
> @@ -2431,6 +2464,14 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
> tls->priv_key_size = 0;
> }
>
> + if (cert_path) {
> + tls->cert = l_pem_load_certificate(cert_path);
> + if (!tls->cert) {
> + TLS_DEBUG("Error loading %s", cert_path);
> + return false;
> + }
> + }
> +
> if (priv_key_path) {
> uint8_t *priv_key;
> bool is_public = true;
> @@ -2463,9 +2504,6 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
> tls->priv_key_size /= 8;
> }
>
> - if (cert_path)
> - tls->cert_path = l_strdup(cert_path);
> -
> return true;
> }
>
> @@ -2546,253 +2584,6 @@ const char *tls_handshake_state_to_str(enum tls_handshake_state state)
> return buf;
> }
>
> -/* X509 Certificates and Certificate Chains */
> -
> -#define X509_CERTIFICATE_POS 0
> -#define X509_TBSCERTIFICATE_POS 0
> -#define X509_TBSCERT_VERSION_POS 0
> -#define X509_TBSCERT_SERIAL_POS 1
> -#define X509_TBSCERT_SIGNATURE_POS 2
> -#define X509_ALGORITHM_ID_ALGORITHM_POS 0
> -#define X509_ALGORITHM_ID_PARAMS_POS 1
> -#define X509_TBSCERT_ISSUER_DN_POS 3
> -#define X509_TBSCERT_VALIDITY_POS 4
> -#define X509_TBSCERT_SUBJECT_DN_POS 5
> -#define X509_TBSCERT_SUBJECT_KEY_POS 6
> -#define X509_SUBJECT_KEY_ALGORITHM_POS 0
> -#define X509_SUBJECT_KEY_VALUE_POS 1
> -#define X509_TBSCERT_ISSUER_UID_POS 7
> -#define X509_TBSCERT_SUBJECT_UID_POS 8
> -#define X509_TBSCERT_EXTENSIONS_POS 9
> -#define X509_SIGNATURE_ALGORITHM_POS 1
> -#define X509_SIGNATURE_VALUE_POS 2
> -
> -struct tls_cert *tls_cert_load_file(const char *filename)
> -{
> - uint8_t *der;
> - size_t len;
> - struct tls_cert *cert;
> -
> - der = l_pem_load_certificate(filename, &len);
> - if (!der)
> - return NULL;
> -
> - if (!len || der[0] != ASN1_ID_SEQUENCE) {
> - l_free(der);
> - return NULL;
> - }
> -
> - cert = l_malloc(sizeof(struct tls_cert) + len);
> - cert->size = len;
> - cert->issuer = NULL;
> - memcpy(cert->asn1, der, len);
> -
> - l_free(der);
> -
> - return cert;
> -}
> -
> -int tls_cert_from_certificate_list(const void *data, size_t len,
> - struct tls_cert **out_certchain)
> -{
> - const uint8_t *buf = data;
> - struct tls_cert *certchain = NULL, **tail = &certchain;
> -
> - while (len) {
> - size_t cert_len;
> -
> - if (len < 3)
> - goto decode_error;
> -
> - cert_len = *buf++ << 16;
> - cert_len |= *buf++ << 8;
> - cert_len |= *buf++ << 0;
> -
> - if (cert_len + 3 > len)
> - goto decode_error;
> -
> - *tail = l_malloc(sizeof(struct tls_cert) + cert_len);
> - (*tail)->size = cert_len;
> - (*tail)->issuer = NULL;
> - memcpy((*tail)->asn1, buf, cert_len);
> -
> - tail = &(*tail)->issuer;
> -
> - buf += cert_len;
> - len -= cert_len + 3;
> - }
> -
> - if (out_certchain)
> - *out_certchain = certchain;
> -
> - return 0;
> -
> -decode_error:
> - tls_cert_free_certchain(certchain);
> - return -EBADMSG;
> -}
> -
> -bool tls_cert_find_certchain(struct tls_cert *cert,
> - const char *cacert_filename)
> -{
> - return true;
> -}
> -
> -static const struct pkcs1_encryption_oid {
> - enum tls_cert_key_type key_type;
> - struct asn1_oid oid;
> -} pkcs1_encryption_oids[] = {
> - { /* rsaEncryption */
> - TLS_CERT_KEY_RSA,
> - { 9, { 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01 } },
> - },
> -};
> -
> -static void tls_key_cleanup(struct l_key **p)
> -{
> - l_key_free_norevoke(*p);
> -}
> -
> -static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
> - struct l_keyring *ring,
> - struct tls_cert *root,
> - struct l_keyring *trusted)
> -{
> - if (!cert)
> - return true;
> -
> - /*
> - * RFC5246 7.4.2:
> - * "Because certificate validation requires that root keys be
> - * distributed independently, the self-signed certificate that
> - * specifies the root certificate authority MAY be omitted from
> - * the chain, under the assumption that the remote end must
> - * already possess it in order to validate it in any case."
> - */
> - if (!cert->issuer && root && cert->size == root->size &&
> - !memcmp(cert->asn1, root->asn1, root->size))
> - return true;
> -
> - if (tls_cert_verify_with_keyring(cert->issuer, ring, root, trusted)) {
> - L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup);
> -
> - key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
> - if (!key)
> - return false;
> -
> - if (!l_keyring_link(ring, key))
> - return false;
> -
> - if (trusted || cert->issuer)
> - return true;
> -
> - /*
> - * If execution reaches this point, it's known that:
> - * * No trusted root key was supplied, so the chain is only
> - * being checked against its own root
> - * * The keyring 'ring' is not restricted yet
> - * * The chain's root cert was just linked in to the
> - * previously empty keyring 'ring'.
> - *
> - * By restricting 'ring' now, the rest of the certs in
> - * the chain will have their signature validated using 'key'
> - * as the root.
> - */
> - return l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN,
> - trusted);
> - }
> -
> - return false;
> -}
> -
> -static void tls_keyring_cleanup(struct l_keyring **p)
> -{
> - l_keyring_free(*p);
> -}
> -
> -bool tls_cert_verify_certchain(struct tls_cert *certchain,
> - struct tls_cert *ca_cert)
> -{
> - L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
> - L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
> - tls_keyring_cleanup);
> -
> - ca_ring = NULL;
> - verify_ring = NULL;
> -
> - if (ca_cert) {
> - L_AUTO_CLEANUP_VAR(struct l_key *, ca_key, tls_key_cleanup);
> - ca_key = NULL;
> -
> - ca_ring = l_keyring_new();
> - if (!ca_ring)
> - return false;
> -
> - ca_key = l_key_new(L_KEY_RSA, ca_cert->asn1, ca_cert->size);
> - if (!ca_key || !l_keyring_link(ca_ring, ca_key))
> - return false;
> - }
> -
> - verify_ring = l_keyring_new();
> - if (!verify_ring)
> - return false;
> -
> - /*
> - * If a CA cert was supplied, restrict verify_ring now so
> - * everything else in certchain is validated against the CA.
> - * Otherwise, verify_ring will be restricted after the root of
> - * certchain is added to verify_ring by
> - * tls_cert_verify_with_keyring().
> - */
> - if (ca_ring && !l_keyring_restrict(verify_ring,
> - L_KEYRING_RESTRICT_ASYM_CHAIN,
> - ca_ring)) {
> - return false;
> - }
> -
> - return tls_cert_verify_with_keyring(certchain, verify_ring, ca_cert,
> - ca_ring);
> -}
> -
> -void tls_cert_free_certchain(struct tls_cert *cert)
> -{
> - struct tls_cert *next;
> -
> - while (cert) {
> - next = cert->issuer;
> - l_free(cert);
> - cert = next;
> - }
> -}
> -
> -enum tls_cert_key_type tls_cert_get_pubkey_type(struct tls_cert *cert)
> -{
> - const uint8_t *key_type;
> - size_t key_type_len;
> - int i;
> -
> - key_type = asn1_der_find_elem_by_path(cert->asn1, cert->size,
> - ASN1_ID_OID, &key_type_len,
> - X509_CERTIFICATE_POS,
> - X509_TBSCERTIFICATE_POS,
> - X509_TBSCERT_SUBJECT_KEY_POS,
> - X509_SUBJECT_KEY_ALGORITHM_POS,
> - X509_ALGORITHM_ID_ALGORITHM_POS,
> - -1);
> - if (!key_type)
> - return TLS_CERT_KEY_UNKNOWN;
> -
> - for (i = 0; i < (int) L_ARRAY_SIZE(pkcs1_encryption_oids); i++)
> - if (asn1_oid_eq(&pkcs1_encryption_oids[i].oid,
> - key_type_len, key_type))
> - break;
> -
> - if (i == L_ARRAY_SIZE(pkcs1_encryption_oids))
> - return TLS_CERT_KEY_UNKNOWN;
> -
> - return pkcs1_encryption_oids[i].key_type;
> -}
> -
> LIB_EXPORT bool l_tls_set_debug(struct l_tls *tls, l_tls_debug_cb_t function,
> void *user_data, l_tls_destroy_cb_t destroy)
> {
> diff --git a/ell/tls.h b/ell/tls.h
> index 893fd63..5008200 100644
> --- a/ell/tls.h
> +++ b/ell/tls.h
> @@ -87,7 +87,7 @@ void l_tls_write(struct l_tls *tls, const uint8_t *data, size_t len);
> void l_tls_handle_rx(struct l_tls *tls, const uint8_t *data, size_t len);
>
> /* If peer is to be authenticated, supply the CA certificate */
> -void l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path);
> +bool l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path);
>
> /*
> * If we are to be authenticated, supply our certificate, private key and
> --
> 2.19.1
>
> _______________________________________________
> ell mailing list
> ell(a)lists.01.org
> https://lists.01.org/mailman/listinfo/ell
>
--
Mat Martineau
Intel OTC
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] cert: Add cert.c for certificate types and routines
2018-11-08 17:39 ` [PATCH 1/5] cert: Add cert.c for certificate types and routines Denis Kenzior
@ 2018-11-08 23:22 ` Andrew Zaborowski
2018-11-09 1:05 ` Denis Kenzior
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 23:22 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 10934 bytes --]
Hi Denis,
On Thu, 8 Nov 2018 at 18:39, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/08/2018 06:06 AM, Andrew Zaborowski wrote:
> > +struct l_cert {
> > + struct l_cert *issuer;
> > + struct l_cert *issued;
>
> So I don't really like this dual-purpose structure. l_cert is used both
> as a standalone certificate and a certchain, and that creates weird
> situations where the caller isn't quite sure when l_cert_free should be
> called, especially when l_cert_link* is involved. Not without looking
> at the implementation anyway, and that's undesirable from an API
> simplicity / nice-to-use perspective.
Yep, that's the downside of this approach, the up side is less code
for both the implementation and the user. I'd understood you actually
preferred this approach but I'll happily switch to having separate
types.
>
> > + enum l_cert_key_type pubkey_type;
> > + size_t asn1_len;
> > + uint8_t asn1[0];
> > +};
> > +
> > +static const struct pkcs1_encryption_oid {
> > + enum l_cert_key_type key_type;
> > + struct asn1_oid oid;
> > +} pkcs1_encryption_oids[] = {
> > + { /* rsaEncryption */
> > + L_CERT_KEY_RSA,
> > + { 9, { 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01 } },
> > + },
> > +};
> > +
> > +static bool cert_set_pubkey_type(struct l_cert *cert)
> > +{
> > + const uint8_t *key_type;
> > + size_t key_type_len;
> > + int i;
> > +
> > + key_type = asn1_der_find_elem_by_path(cert->asn1, cert->asn1_len,
> > + ASN1_ID_OID, &key_type_len,
> > + X509_CERTIFICATE_POS,
> > + X509_TBSCERTIFICATE_POS,
> > + X509_TBSCERT_SUBJECT_KEY_POS,
> > + X509_SUBJECT_KEY_ALGORITHM_POS,
> > + X509_ALGORITHM_ID_ALGORITHM_POS,
> > + -1);
> > + if (!key_type)
> > + return false;
> > +
> > + for (i = 0; i < (int) L_ARRAY_SIZE(pkcs1_encryption_oids); i++)
> > + if (asn1_oid_eq(&pkcs1_encryption_oids[i].oid,
> > + key_type_len, key_type))
> > + break;
>
> So the intent of l_cert is to only store public keys, right? Not
> private keys.
Well, yes, the intent is to store the certificates which among other
things contain the subject's public key and do not contain a private
key.
>
> > +
> > + if (i == L_ARRAY_SIZE(pkcs1_encryption_oids))
> > + cert->pubkey_type = L_CERT_KEY_UNKNOWN;
> > + else
> > + cert->pubkey_type = pkcs1_encryption_oids[i].key_type;
>
> It is unclear to me how you're making sure pubkey_type enumeration
> actually matches up to key_type. There's no initializer on the enum, so
> the compiler can be free to choose anything?
Sorry, I don't understand what you mean here. The left side of the
'=' is of the same type as the right side. The right side will always
be L_CERT_KEY_RSA for now.
>
> > +
> > + return true;
> > +}
> > +
> > +LIB_EXPORT struct l_cert *l_cert_new_from_der(const uint8_t *buf,
> > + size_t buf_len)
> > +{
> > + const uint8_t *seq = buf;
> > + size_t seq_len = buf_len;
> > + size_t content_len;
> > + struct l_cert *cert;
> > +
> > + /* Sanity check: outer element is a SEQUENCE */
> > + if (seq_len-- < 1 || *seq++ != ASN1_ID_SEQUENCE)
> > + return NULL;
> > +
> > + /* Sanity check: the SEQUENCE spans the whole buffer */
> > + content_len = asn1_parse_definite_length(&seq, &seq_len);
>
> Is it me, or is asn1_parse_definite_length not doing proper boundary
> checking? For example, if buf_len is 1, then doesn't this blow up?
My bad, I think asn1_parse_definite_length intially was initially in a
.c file with a limited number of callers all of which had length
checks so there was no point repeating the check there, I'll fix that.
>
> > + if (content_len < 64 || content_len != seq_len)
> > + return NULL;
> > +
> > + /*
> > + * We could require the signature algorithm and the key algorithm
> > + * to be one of our supported types here but instead we only
> > + * require that when the user wants to verify this certificate or
> > + * get the public key respectively.
> > + */
> > +
> > + cert = l_malloc(sizeof(struct l_cert) + buf_len);
> > + cert->issuer = NULL;
> > + cert->issued = NULL;
> > + cert->asn1_len = buf_len;
> > + memcpy(cert->asn1, buf, buf_len);
> > +
> > + /* Sanity check: structure is correct up to the Public Key Algorithm */
> > + if (!cert_set_pubkey_type(cert)) {
> > + l_free(cert);
> > + return NULL;
> > + }
> > +
> > + return cert;
> > +}
> > +
> > +LIB_EXPORT void l_cert_free(struct l_cert *cert)
> > +{
> > + while (cert) {
> > + struct l_cert *next = cert->issuer;
> > +
> > + l_free(cert);
> > + cert = next;
> > + }
> > +}
> > +
> > +LIB_EXPORT bool l_cert_link_issuer(struct l_cert *cert, struct l_cert *issuer)
> > +{
> > + if (!cert)
> > + return false;
> > +
> > + if (issuer && (issuer->issued || cert->issuer))
> > + return false;
> > +
> > + if (cert->issuer)
> > + cert->issuer->issued = NULL;
> > +
> > + cert->issuer = issuer;
> > + return true;
> > +}
>
> So it is completely unclear to the caller that l_cert_link_issuer takes
> ownership of cert. The caller can mistakenly call l_cert_free on the
> original cert and get a fun result...
Yep, that's why I mentioned refcounting in a conversation we had.
I'll change to using a separate type for certificate chains so this
will go away.
(Although, with the shape of a lot of ell API you probably wouldn't be
assuming anything about l_cert_link_issuer without checking the code.
I'll make sure to add doxygen comments to the next iteration of
cert.c)
>
> > +
> > +LIB_EXPORT struct l_cert *l_cert_get_issuer(struct l_cert *cert)
> > +{
> > + return cert->issuer;
> > +}
> > +
> > +LIB_EXPORT const uint8_t *l_cert_get_der_data(struct l_cert *cert,
> > + size_t *out_len)
> > +{
> > + *out_len = cert->asn1_len;
> > + return cert->asn1;
> > +}
>
> Personally I would just do something like:
>
> struct l_cert {
> enum l_cert_key_type pubkey_type;
> size_t asn1_len;
> uint8_t asn1[0];
> };
>
> struct certchain_link {
> struct l_cert cert;
> struct certchain_link *issuer;
> struct certchain_link *child;
> };
>
> struct l_certchain {
> struct certchain_link *root;
> struct certchain_link *key;
> };
>
> and consider dropping link_issuer completely. Just build a certchain
> from the raw data directly, perhaps as a private constructor for now.
Could also just do:
struct l_certchain {
struct l_queue *certs;
};
for simplicity. We only have one use case for iterating from root to
leaf so we can reverse the queue at that time. We maybe simply want
two public methods, l_certchain_foreach_from_root and
l_certchain_foreach_from_leaf.
As for the constructor we already need to parse two different "raw"
formats both of which are defined in pretty distant specs so I don't
think cert.c should know them, I'd rather have a l_certchain_link_cert
or similar.
>
> > +
> > +LIB_EXPORT bool l_cert_find_certchain(struct l_cert *cert,
> > + struct l_cert *ca_cert)
> > +{
> > + return true;
> > +}
> > +
> > +static void cert_key_cleanup(struct l_key **p)
> > +{
> > + l_key_free_norevoke(*p);
> > +}
> > +
> > +static bool cert_verify_with_keyring(struct l_cert *cert,
> > + struct l_keyring *ring,
> > + struct l_cert *root,
> > + struct l_keyring *trusted)
> > +{
> > + if (!cert)
> > + return true;
> > +
> > + if (cert->pubkey_type != L_CERT_KEY_RSA)
> > + return false;
> > +
> > + /*
> > + * RFC5246 7.4.2:
> > + * "Because certificate validation requires that root keys be
> > + * distributed independently, the self-signed certificate that
> > + * specifies the root certificate authority MAY be omitted from
> > + * the chain, under the assumption that the remote end must
> > + * already possess it in order to validate it in any case."
> > + */
> > + if (!cert->issuer && root && cert->asn1_len == root->asn1_len &&
> > + !memcmp(cert->asn1, root->asn1, root->asn1_len))
> > + return true;
> > +
> > + if (cert_verify_with_keyring(cert->issuer, ring, root, trusted)) {
> > + L_AUTO_CLEANUP_VAR(struct l_key *, key, cert_key_cleanup);
> > +
> > + key = l_key_new(L_KEY_RSA, cert->asn1, cert->asn1_len);
> > + if (!key)
> > + return false;
> > +
> > + if (!l_keyring_link(ring, key))
> > + return false;
> > +
> > + if (trusted || cert->issuer)
> > + return true;
> > +
> > + /*
> > + * If execution reaches this point, it's known that:
> > + * * No trusted root key was supplied, so the chain is only
> > + * being checked against its own root
> > + * * The keyring 'ring' is not restricted yet
> > + * * The chain's root cert was just linked in to the
> > + * previously empty keyring 'ring'.
> > + *
> > + * By restricting 'ring' now, the rest of the certs in
> > + * the chain will have their signature validated using 'key'
> > + * as the root.
> > + */
> > + return l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN,
> > + trusted);
> > + }
> > +
> > + return false;
>
> So we're still doing the recursive verify. It might makes things
> clearer if you added l_cert* stub classes first. Then a separate commit
> moving the recursive implementation from tls.c to cert.c.
Ugh, that would be a commit for each method being moved? There isn't
really much to review if we're just moving the code, however not
breaking TLS between the multiple commits could be really complicated.
Best regards
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] unit: Update tests to new certificate API
2018-11-08 12:06 ` [PATCH 5/5] unit: Update tests " Andrew Zaborowski
@ 2018-11-08 23:27 ` Mat Martineau
2018-11-08 23:30 ` Andrew Zaborowski
0 siblings, 1 reply; 14+ messages in thread
From: Mat Martineau @ 2018-11-08 23:27 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 11289 bytes --]
Andrew,
On Thu, 8 Nov 2018, Andrew Zaborowski wrote:
> ---
> unit/test-key.c | 89 ++++++++++++++++++++-----------------------------
> unit/test-pem.c | 57 ++++++++++++++++++-------------
> unit/test-tls.c | 24 ++++++-------
> 3 files changed, 83 insertions(+), 87 deletions(-)
>
> diff --git a/unit/test-key.c b/unit/test-key.c
> index afe4103..5f65793 100644
> --- a/unit/test-key.c
> +++ b/unit/test-key.c
> @@ -394,23 +394,20 @@ static void test_trusted_keyring(const void *data)
> {
> struct l_keyring *ring;
> struct l_keyring *trust;
> - uint8_t *cacert;
> - size_t cacertlen;
> - uint8_t *cert;
> - size_t certlen;
> + struct l_cert *cacert;
> + struct l_cert *cert;
> struct l_key *cakey;
> struct l_key *key;
> bool success;
>
> - cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem", &cacertlen);
> + cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
> assert(cacert);
> - cert = l_pem_load_certificate(CERTDIR "cert-server.pem",
> - &certlen);
> + cert = l_pem_load_certificate(CERTDIR "cert-server.pem");
> assert(cert);
>
> - cakey = l_key_new(L_KEY_RSA, cacert, cacertlen);
> + cakey = l_cert_get_pubkey(cacert);
> assert(cakey);
> - key = l_key_new(L_KEY_RSA, cert, certlen);
> + key = l_cert_get_pubkey(cert);
> assert(key);
>
> trust = l_keyring_new();
> @@ -431,39 +428,34 @@ static void test_trusted_keyring(const void *data)
> l_keyring_free(ring);
> l_key_free(cakey);
> l_key_free(key);
> - l_free(cacert);
> - l_free(cert);
> + l_cert_free(cacert);
> + l_cert_free(cert);
> }
>
> static void test_trust_chain(const void *data)
> {
> struct l_keyring *ring;
> struct l_keyring *trust;
> - uint8_t *cacert;
> - size_t cacertlen;
> - uint8_t *intcert;
> - size_t intcertlen;
> - uint8_t *cert;
> - size_t certlen;
> + struct l_cert *cacert;
> + struct l_cert *intcert;
> + struct l_cert *cert;
> struct l_key *cakey;
> struct l_key *intkey;
> struct l_key *key;
> bool success;
>
> - cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem", &cacertlen);
> + cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
> assert(cacert);
> - intcert = l_pem_load_certificate(CERTDIR "cert-intca.pem",
> - &intcertlen);
> + intcert = l_pem_load_certificate(CERTDIR "cert-intca.pem");
> assert(intcert);
> - cert = l_pem_load_certificate(CERTDIR "cert-entity-int.pem",
> - &certlen);
> + cert = l_pem_load_certificate(CERTDIR "cert-entity-int.pem");
> assert(cert);
>
> - cakey = l_key_new(L_KEY_RSA, cacert, cacertlen);
> + cakey = l_cert_get_pubkey(cacert);
> assert(cakey);
> - intkey = l_key_new(L_KEY_RSA, intcert, intcertlen);
> + intkey = l_cert_get_pubkey(intcert);
> assert(intkey);
> - key = l_key_new(L_KEY_RSA, cert, certlen);
> + key = l_cert_get_pubkey(cert);
> assert(key);
>
> trust = l_keyring_new();
> @@ -492,18 +484,15 @@ static void test_trust_chain(const void *data)
> l_key_free(cakey);
> l_key_free(intkey);
> l_key_free(key);
> - l_free(cacert);
> - l_free(intcert);
> - l_free(cert);
> + l_cert_free(cacert);
> + l_cert_free(intcert);
> + l_cert_free(cert);
> }
>
> static void test_key_crypto(const void *data)
> {
> - uint8_t *cert;
> - size_t certlen;
> - uint8_t *pubcert;
> - size_t pubcertlen;
> - struct l_key *key;
> + struct l_key *privkey;
> + struct l_cert *cert;
> struct l_key *pubkey;
> bool is_public;
> size_t keybits;
> @@ -514,24 +503,21 @@ static void test_key_crypto(const void *data)
> int hash = L_CHECKSUM_NONE;
> int rsa = L_KEY_RSA_PKCS1_V1_5;
>
> - cert = l_pem_load_private_key(CERTDIR "cert-client-key-pkcs8.pem",
> - NULL, NULL, &certlen);
> + privkey = l_pem_load_private_key(CERTDIR "cert-client-key-pkcs8.pem",
> + NULL, NULL);
> + assert(privkey);
> + cert = l_pem_load_certificate(CERTDIR "cert-client.pem");
> assert(cert);
> - pubcert = l_pem_load_certificate(CERTDIR "cert-client.pem",
> - &pubcertlen);
> - assert(pubcert);
>
> - key = l_key_new(L_KEY_RSA, cert, certlen);
> - assert(key);
> - pubkey = l_key_new(L_KEY_RSA, pubcert, pubcertlen);
> + pubkey = l_cert_get_pubkey(cert);
> assert(pubkey);
>
> - success = l_key_get_info(key, rsa, hash, &keybits, &is_public);
> + success = l_key_get_info(privkey, rsa, hash, &keybits, &is_public);
> assert(success);
> assert(keybits == 2048);
> assert(!is_public);
>
> - success = l_key_get_info(key, rsa, L_CHECKSUM_NONE, &keybits,
> + success = l_key_get_info(privkey, rsa, L_CHECKSUM_NONE, &keybits,
> &is_public);
> assert(success);
> assert(keybits == 2048);
> @@ -554,14 +540,14 @@ static void test_key_crypto(const void *data)
> sizeof(ciphertext), sizeof(decrypted));
> assert(len < 0);
>
> - len = l_key_decrypt(key, rsa, hash, ciphertext, decrypted,
> + len = l_key_decrypt(privkey, rsa, hash, ciphertext, decrypted,
> sizeof(ciphertext), sizeof(decrypted));
> assert(len == (ssize_t)strlen(plaintext));
> assert(strcmp(plaintext, (char *)decrypted) == 0);
>
> /* Decrypt reference ciphertext */
> memset(decrypted, 0, sizeof(decrypted));
> - len = l_key_decrypt(key, rsa, hash, reference_ciphertext, decrypted,
> + len = l_key_decrypt(privkey, rsa, hash, reference_ciphertext, decrypted,
> sizeof(reference_ciphertext),
> sizeof(decrypted));
> assert(len == (ssize_t)strlen(plaintext));
> @@ -571,7 +557,7 @@ static void test_key_crypto(const void *data)
> memset(decrypted, 0, sizeof(decrypted));
> memcpy(ciphertext, reference_ciphertext, sizeof(ciphertext));
> ciphertext[0] = ciphertext[0] ^ (uint8_t)0xFF;
> - len = l_key_decrypt(key, rsa, hash, ciphertext, decrypted,
> + len = l_key_decrypt(privkey, rsa, hash, ciphertext, decrypted,
> sizeof(ciphertext),
> sizeof(decrypted));
> assert(len < 0);
> @@ -581,7 +567,7 @@ static void test_key_crypto(const void *data)
> strlen(plaintext), sizeof(ciphertext));
> assert(len < 0);
>
> - len = l_key_sign(key, rsa, hash, plaintext, ciphertext,
> + len = l_key_sign(privkey, rsa, hash, plaintext, ciphertext,
> strlen(plaintext), sizeof(ciphertext));
> assert(len == sizeof(ciphertext));
>
> @@ -589,7 +575,7 @@ static void test_key_crypto(const void *data)
> strlen(plaintext), sizeof(ciphertext));
> assert(success);
>
> - success = l_key_verify(key, rsa, hash, plaintext, ciphertext,
> + success = l_key_verify(privkey, rsa, hash, plaintext, ciphertext,
> strlen(plaintext), sizeof(ciphertext));
> assert(success);
>
> @@ -600,14 +586,13 @@ static void test_key_crypto(const void *data)
>
> /* Corrupt signature */
> ciphertext[42] = ciphertext[52] ^ (uint8_t)0xFF;
> - success = l_key_verify(key, rsa, hash, plaintext, ciphertext,
> + success = l_key_verify(privkey, rsa, hash, plaintext, ciphertext,
> strlen(plaintext), sizeof(ciphertext));
> assert(!success);
>
> - l_key_free(key);
> + l_key_free(privkey);
> l_key_free(pubkey);
> - l_free(cert);
> - l_free(pubcert);
> + l_cert_free(cert);
> }
>
> int main(int argc, char *argv[])
> diff --git a/unit/test-pem.c b/unit/test-pem.c
> index 3325e54..b37a29e 100644
> --- a/unit/test-pem.c
> +++ b/unit/test-pem.c
> @@ -91,36 +91,47 @@ static void test_encrypted_pkey(const void *data)
> {
> const char *encrypted_pem = data;
> const char *plaintext_pem = CERTDIR "cert-client-key-pkcs8.pem";
> - bool encrypted;
> - size_t size1, size2;
> - uint8_t *pkey1, *pkey2;
> + bool is_encrypted;
> + size_t size;
> + uint8_t encrypted1[256], encrypted2[256], plaintext[256];
> + struct l_key *pkey1, *pkey2;
> + bool is_public;
>
> - encrypted = false;
> - assert(!l_pem_load_private_key(encrypted_pem, NULL,
> - &encrypted, &size1));
> - assert(encrypted);
> + is_encrypted = false;
> + assert(!l_pem_load_private_key(encrypted_pem, NULL, &is_encrypted));
> + assert(is_encrypted);
>
> - encrypted = false;
> + is_encrypted = false;
> assert(!l_pem_load_private_key(encrypted_pem, "wrong-passwd",
> - &encrypted, &size1));
> - assert(encrypted);
> + &is_encrypted));
> + assert(is_encrypted);
>
> - encrypted = false;
> - pkey1 = l_pem_load_private_key(encrypted_pem, "abc",
> - &encrypted, &size1);
> + is_encrypted = false;
> + pkey1 = l_pem_load_private_key(encrypted_pem, "abc", &is_encrypted);
> assert(pkey1);
This assert is failing for me with a kernel that doesn't support RSA keys
(v4.18.17). Could you add an l_key_is_supported() check like test-key.c to
skip tests that depend on RSA key support for kernels that don't have the
necessary feature?
Mat
> - assert(encrypted);
> + assert(is_encrypted);
>
> - pkey2 = l_pem_load_private_key(plaintext_pem, NULL,
> - &encrypted, &size2);
> + pkey2 = l_pem_load_private_key(plaintext_pem, NULL, &is_encrypted);
> assert(pkey2);
> - assert(!encrypted);
> -
> - assert(size1 == size2);
> - assert(!memcmp(pkey1, pkey2, size1));
> -
> - l_free(pkey1);
> - l_free(pkey2);
> + assert(!is_encrypted);
> +
> + /*
> + * l_key_extract doesn't work for private keys so compare encrypt
> + * results instead of key exponent.
> + */
> + memset(plaintext, 42, 256);
> + assert(l_key_get_info(pkey1, L_KEY_RSA_RAW, L_CHECKSUM_NONE,
> + &size, &is_public));
> + assert(size == 2048);
> + assert(!is_public);
> + assert(l_key_encrypt(pkey1, L_KEY_RSA_RAW, L_CHECKSUM_NONE,
> + plaintext, encrypted1, 256, 256) == 256);
> + assert(l_key_encrypt(pkey2, L_KEY_RSA_RAW, L_CHECKSUM_NONE,
> + plaintext, encrypted2, 256, 256) == 256);
> + assert(!memcmp(encrypted1, encrypted2, 256));
> +
> + l_key_free(pkey1);
> + l_key_free(pkey2);
> }
>
> int main(int argc, char *argv[])
> diff --git a/unit/test-tls.c b/unit/test-tls.c
> index d969ee7..5dd6569 100644
> --- a/unit/test-tls.c
> +++ b/unit/test-tls.c
> @@ -213,28 +213,28 @@ static void test_tls12_prf(const void *data)
>
> static void test_certificates(const void *data)
> {
> - struct tls_cert *cert;
> - struct tls_cert *cacert;
> - struct tls_cert *wrongca;
> + struct l_cert *cert;
> + struct l_cert *cacert;
> + struct l_cert *wrongca;
>
> - cert = tls_cert_load_file(CERTDIR "cert-server.pem");
> + cert = l_pem_load_certificate(CERTDIR "cert-server.pem");
> assert(cert);
>
> - cacert = tls_cert_load_file(CERTDIR "cert-ca.pem");
> + cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
> assert(cacert);
>
> - wrongca = tls_cert_load_file(CERTDIR "cert-intca.pem");
> + wrongca = l_pem_load_certificate(CERTDIR "cert-intca.pem");
> assert(wrongca);
>
> - assert(!tls_cert_verify_certchain(cert, wrongca));
> + assert(!l_cert_verify_certchain(cert, wrongca));
>
> - assert(tls_cert_verify_certchain(cert, cacert));
> + assert(l_cert_verify_certchain(cert, cacert));
>
> - assert(tls_cert_verify_certchain(cert, NULL));
> + assert(l_cert_verify_certchain(cert, NULL));
>
> - l_free(cert);
> - l_free(cacert);
> - l_free(wrongca);
> + l_cert_free(cert);
> + l_cert_free(cacert);
> + l_cert_free(wrongca);
> }
>
> struct tls_conn_test {
> --
> 2.19.1
>
> _______________________________________________
> ell mailing list
> ell(a)lists.01.org
> https://lists.01.org/mailman/listinfo/ell
>
--
Mat Martineau
Intel OTC
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] unit: Update tests to new certificate API
2018-11-08 23:27 ` Mat Martineau
@ 2018-11-08 23:30 ` Andrew Zaborowski
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-08 23:30 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 10702 bytes --]
On Fri, 9 Nov 2018 at 00:27, Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
> On Thu, 8 Nov 2018, Andrew Zaborowski wrote:
> > ---
> > unit/test-key.c | 89 ++++++++++++++++++++-----------------------------
> > unit/test-pem.c | 57 ++++++++++++++++++-------------
> > unit/test-tls.c | 24 ++++++-------
> > 3 files changed, 83 insertions(+), 87 deletions(-)
> >
> > diff --git a/unit/test-key.c b/unit/test-key.c
> > index afe4103..5f65793 100644
> > --- a/unit/test-key.c
> > +++ b/unit/test-key.c
> > @@ -394,23 +394,20 @@ static void test_trusted_keyring(const void *data)
> > {
> > struct l_keyring *ring;
> > struct l_keyring *trust;
> > - uint8_t *cacert;
> > - size_t cacertlen;
> > - uint8_t *cert;
> > - size_t certlen;
> > + struct l_cert *cacert;
> > + struct l_cert *cert;
> > struct l_key *cakey;
> > struct l_key *key;
> > bool success;
> >
> > - cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem", &cacertlen);
> > + cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
> > assert(cacert);
> > - cert = l_pem_load_certificate(CERTDIR "cert-server.pem",
> > - &certlen);
> > + cert = l_pem_load_certificate(CERTDIR "cert-server.pem");
> > assert(cert);
> >
> > - cakey = l_key_new(L_KEY_RSA, cacert, cacertlen);
> > + cakey = l_cert_get_pubkey(cacert);
> > assert(cakey);
> > - key = l_key_new(L_KEY_RSA, cert, certlen);
> > + key = l_cert_get_pubkey(cert);
> > assert(key);
> >
> > trust = l_keyring_new();
> > @@ -431,39 +428,34 @@ static void test_trusted_keyring(const void *data)
> > l_keyring_free(ring);
> > l_key_free(cakey);
> > l_key_free(key);
> > - l_free(cacert);
> > - l_free(cert);
> > + l_cert_free(cacert);
> > + l_cert_free(cert);
> > }
> >
> > static void test_trust_chain(const void *data)
> > {
> > struct l_keyring *ring;
> > struct l_keyring *trust;
> > - uint8_t *cacert;
> > - size_t cacertlen;
> > - uint8_t *intcert;
> > - size_t intcertlen;
> > - uint8_t *cert;
> > - size_t certlen;
> > + struct l_cert *cacert;
> > + struct l_cert *intcert;
> > + struct l_cert *cert;
> > struct l_key *cakey;
> > struct l_key *intkey;
> > struct l_key *key;
> > bool success;
> >
> > - cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem", &cacertlen);
> > + cacert = l_pem_load_certificate(CERTDIR "cert-ca.pem");
> > assert(cacert);
> > - intcert = l_pem_load_certificate(CERTDIR "cert-intca.pem",
> > - &intcertlen);
> > + intcert = l_pem_load_certificate(CERTDIR "cert-intca.pem");
> > assert(intcert);
> > - cert = l_pem_load_certificate(CERTDIR "cert-entity-int.pem",
> > - &certlen);
> > + cert = l_pem_load_certificate(CERTDIR "cert-entity-int.pem");
> > assert(cert);
> >
> > - cakey = l_key_new(L_KEY_RSA, cacert, cacertlen);
> > + cakey = l_cert_get_pubkey(cacert);
> > assert(cakey);
> > - intkey = l_key_new(L_KEY_RSA, intcert, intcertlen);
> > + intkey = l_cert_get_pubkey(intcert);
> > assert(intkey);
> > - key = l_key_new(L_KEY_RSA, cert, certlen);
> > + key = l_cert_get_pubkey(cert);
> > assert(key);
> >
> > trust = l_keyring_new();
> > @@ -492,18 +484,15 @@ static void test_trust_chain(const void *data)
> > l_key_free(cakey);
> > l_key_free(intkey);
> > l_key_free(key);
> > - l_free(cacert);
> > - l_free(intcert);
> > - l_free(cert);
> > + l_cert_free(cacert);
> > + l_cert_free(intcert);
> > + l_cert_free(cert);
> > }
> >
> > static void test_key_crypto(const void *data)
> > {
> > - uint8_t *cert;
> > - size_t certlen;
> > - uint8_t *pubcert;
> > - size_t pubcertlen;
> > - struct l_key *key;
> > + struct l_key *privkey;
> > + struct l_cert *cert;
> > struct l_key *pubkey;
> > bool is_public;
> > size_t keybits;
> > @@ -514,24 +503,21 @@ static void test_key_crypto(const void *data)
> > int hash = L_CHECKSUM_NONE;
> > int rsa = L_KEY_RSA_PKCS1_V1_5;
> >
> > - cert = l_pem_load_private_key(CERTDIR "cert-client-key-pkcs8.pem",
> > - NULL, NULL, &certlen);
> > + privkey = l_pem_load_private_key(CERTDIR "cert-client-key-pkcs8.pem",
> > + NULL, NULL);
> > + assert(privkey);
> > + cert = l_pem_load_certificate(CERTDIR "cert-client.pem");
> > assert(cert);
> > - pubcert = l_pem_load_certificate(CERTDIR "cert-client.pem",
> > - &pubcertlen);
> > - assert(pubcert);
> >
> > - key = l_key_new(L_KEY_RSA, cert, certlen);
> > - assert(key);
> > - pubkey = l_key_new(L_KEY_RSA, pubcert, pubcertlen);
> > + pubkey = l_cert_get_pubkey(cert);
> > assert(pubkey);
> >
> > - success = l_key_get_info(key, rsa, hash, &keybits, &is_public);
> > + success = l_key_get_info(privkey, rsa, hash, &keybits, &is_public);
> > assert(success);
> > assert(keybits == 2048);
> > assert(!is_public);
> >
> > - success = l_key_get_info(key, rsa, L_CHECKSUM_NONE, &keybits,
> > + success = l_key_get_info(privkey, rsa, L_CHECKSUM_NONE, &keybits,
> > &is_public);
> > assert(success);
> > assert(keybits == 2048);
> > @@ -554,14 +540,14 @@ static void test_key_crypto(const void *data)
> > sizeof(ciphertext), sizeof(decrypted));
> > assert(len < 0);
> >
> > - len = l_key_decrypt(key, rsa, hash, ciphertext, decrypted,
> > + len = l_key_decrypt(privkey, rsa, hash, ciphertext, decrypted,
> > sizeof(ciphertext), sizeof(decrypted));
> > assert(len == (ssize_t)strlen(plaintext));
> > assert(strcmp(plaintext, (char *)decrypted) == 0);
> >
> > /* Decrypt reference ciphertext */
> > memset(decrypted, 0, sizeof(decrypted));
> > - len = l_key_decrypt(key, rsa, hash, reference_ciphertext, decrypted,
> > + len = l_key_decrypt(privkey, rsa, hash, reference_ciphertext, decrypted,
> > sizeof(reference_ciphertext),
> > sizeof(decrypted));
> > assert(len == (ssize_t)strlen(plaintext));
> > @@ -571,7 +557,7 @@ static void test_key_crypto(const void *data)
> > memset(decrypted, 0, sizeof(decrypted));
> > memcpy(ciphertext, reference_ciphertext, sizeof(ciphertext));
> > ciphertext[0] = ciphertext[0] ^ (uint8_t)0xFF;
> > - len = l_key_decrypt(key, rsa, hash, ciphertext, decrypted,
> > + len = l_key_decrypt(privkey, rsa, hash, ciphertext, decrypted,
> > sizeof(ciphertext),
> > sizeof(decrypted));
> > assert(len < 0);
> > @@ -581,7 +567,7 @@ static void test_key_crypto(const void *data)
> > strlen(plaintext), sizeof(ciphertext));
> > assert(len < 0);
> >
> > - len = l_key_sign(key, rsa, hash, plaintext, ciphertext,
> > + len = l_key_sign(privkey, rsa, hash, plaintext, ciphertext,
> > strlen(plaintext), sizeof(ciphertext));
> > assert(len == sizeof(ciphertext));
> >
> > @@ -589,7 +575,7 @@ static void test_key_crypto(const void *data)
> > strlen(plaintext), sizeof(ciphertext));
> > assert(success);
> >
> > - success = l_key_verify(key, rsa, hash, plaintext, ciphertext,
> > + success = l_key_verify(privkey, rsa, hash, plaintext, ciphertext,
> > strlen(plaintext), sizeof(ciphertext));
> > assert(success);
> >
> > @@ -600,14 +586,13 @@ static void test_key_crypto(const void *data)
> >
> > /* Corrupt signature */
> > ciphertext[42] = ciphertext[52] ^ (uint8_t)0xFF;
> > - success = l_key_verify(key, rsa, hash, plaintext, ciphertext,
> > + success = l_key_verify(privkey, rsa, hash, plaintext, ciphertext,
> > strlen(plaintext), sizeof(ciphertext));
> > assert(!success);
> >
> > - l_key_free(key);
> > + l_key_free(privkey);
> > l_key_free(pubkey);
> > - l_free(cert);
> > - l_free(pubcert);
> > + l_cert_free(cert);
> > }
> >
> > int main(int argc, char *argv[])
> > diff --git a/unit/test-pem.c b/unit/test-pem.c
> > index 3325e54..b37a29e 100644
> > --- a/unit/test-pem.c
> > +++ b/unit/test-pem.c
> > @@ -91,36 +91,47 @@ static void test_encrypted_pkey(const void *data)
> > {
> > const char *encrypted_pem = data;
> > const char *plaintext_pem = CERTDIR "cert-client-key-pkcs8.pem";
> > - bool encrypted;
> > - size_t size1, size2;
> > - uint8_t *pkey1, *pkey2;
> > + bool is_encrypted;
> > + size_t size;
> > + uint8_t encrypted1[256], encrypted2[256], plaintext[256];
> > + struct l_key *pkey1, *pkey2;
> > + bool is_public;
> >
> > - encrypted = false;
> > - assert(!l_pem_load_private_key(encrypted_pem, NULL,
> > - &encrypted, &size1));
> > - assert(encrypted);
> > + is_encrypted = false;
> > + assert(!l_pem_load_private_key(encrypted_pem, NULL, &is_encrypted));
> > + assert(is_encrypted);
> >
> > - encrypted = false;
> > + is_encrypted = false;
> > assert(!l_pem_load_private_key(encrypted_pem, "wrong-passwd",
> > - &encrypted, &size1));
> > - assert(encrypted);
> > + &is_encrypted));
> > + assert(is_encrypted);
> >
> > - encrypted = false;
> > - pkey1 = l_pem_load_private_key(encrypted_pem, "abc",
> > - &encrypted, &size1);
> > + is_encrypted = false;
> > + pkey1 = l_pem_load_private_key(encrypted_pem, "abc", &is_encrypted);
> > assert(pkey1);
>
> This assert is failing for me with a kernel that doesn't support RSA keys
> (v4.18.17). Could you add an l_key_is_supported() check like test-key.c to
> skip tests that depend on RSA key support for kernels that don't have the
> necessary feature?
Oh, right, I didn't realize this added a new requirement for this
test. Thanks for testing.
Best regards
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] cert: Add cert.c for certificate types and routines
2018-11-08 23:22 ` Andrew Zaborowski
@ 2018-11-09 1:05 ` Denis Kenzior
2018-11-09 8:47 ` Andrew Zaborowski
0 siblings, 1 reply; 14+ messages in thread
From: Denis Kenzior @ 2018-11-09 1:05 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]
Hi Andrew,
> Yep, that's the downside of this approach, the up side is less code
> for both the implementation and the user. I'd understood you actually
> preferred this approach but I'll happily switch to having separate
> types.
When have I ever preferred less code in favor of less readability? :)
Anyhow, I rather have something with clear semantics even if it costs a
bit more code size.
>>> +
>>> + if (i == L_ARRAY_SIZE(pkcs1_encryption_oids))
>>> + cert->pubkey_type = L_CERT_KEY_UNKNOWN;
>>> + else
>>> + cert->pubkey_type = pkcs1_encryption_oids[i].key_type;
>>
>> It is unclear to me how you're making sure pubkey_type enumeration
>> actually matches up to key_type. There's no initializer on the enum, so
>> the compiler can be free to choose anything?
>
> Sorry, I don't understand what you mean here. The left side of the
> '=' is of the same type as the right side. The right side will always
> be L_CERT_KEY_RSA for now.
>
Yes, ignore me. I misread what this was doing.
>> and consider dropping link_issuer completely. Just build a certchain
>> from the raw data directly, perhaps as a private constructor for now.
>
> Could also just do:
>
> struct l_certchain {
> struct l_queue *certs;
> };
>
> for simplicity. We only have one use case for iterating from root to
> leaf so we can reverse the queue at that time. We maybe simply want
> two public methods, l_certchain_foreach_from_root and
> l_certchain_foreach_from_leaf.
Yes but a quite common one. And you still need to memdup the l_cert
when you add it to the certchain, so maybe just do it right.
>
> As for the constructor we already need to parse two different "raw"
> formats both of which are defined in pretty distant specs so I don't
> think cert.c should know them, I'd rather have a l_certchain_link_cert
> or similar.
You can also just implement the two constructors directly here, since
they're the most common case anyway.
>>
>> So we're still doing the recursive verify. It might makes things
>> clearer if you added l_cert* stub classes first. Then a separate commit
>> moving the recursive implementation from tls.c to cert.c.
>
> Ugh, that would be a commit for each method being moved? There isn't
> really much to review if we're just moving the code, however not
> breaking TLS between the multiple commits could be really complicated.
Still much clearer / preferable than adding a brand new version and then
removing the old one. And don't worry about breaking compilation too
much. We can handwave that away as long as the entire series makes
things happy.
Regards,
-Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] cert: Add cert.c for certificate types and routines
2018-11-09 1:05 ` Denis Kenzior
@ 2018-11-09 8:47 ` Andrew Zaborowski
2018-11-09 15:34 ` Denis Kenzior
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-09 8:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]
On Fri, 9 Nov 2018 at 02:05, Denis Kenzior <denkenz@gmail.com> wrote:
> > Yep, that's the downside of this approach, the up side is less code
> > for both the implementation and the user. I'd understood you actually
> > preferred this approach but I'll happily switch to having separate
> > types.
>
> When have I ever preferred less code in favor of less readability? :)
>
> Anyhow, I rather have something with clear semantics even if it costs a
> bit more code size.
We must have been talking about different things then.
> >> and consider dropping link_issuer completely. Just build a certchain
> >> from the raw data directly, perhaps as a private constructor for now.
> >
> > Could also just do:
> >
> > struct l_certchain {
> > struct l_queue *certs;
> > };
> >
> > for simplicity. We only have one use case for iterating from root to
> > leaf so we can reverse the queue at that time. We maybe simply want
> > two public methods, l_certchain_foreach_from_root and
> > l_certchain_foreach_from_leaf.
>
> Yes but a quite common one. And you still need to memdup the l_cert
> when you add it to the certchain, so maybe just do it right.
By that you mean adding a new l_queue-like structure in cert.c, right?
Can we think of an API that avoids the memdup of the whole
certificate?
>
> >
> > As for the constructor we already need to parse two different "raw"
> > formats both of which are defined in pretty distant specs so I don't
> > think cert.c should know them, I'd rather have a l_certchain_link_cert
> > or similar.
>
> You can also just implement the two constructors directly here, since
> they're the most common case anyway.
One case is a completely specific to TLS where parse a binary
structure so there's no reason it shouldn't be in tls.c. The other
case is loading from a PEM file (remember we tried to avoid adding the
l_pem_load_certificate_list function returning multiple buffers? We
can't build the certchain in cert.c without that function, or calling
l_pem_load_file multiple times)
>
> >>
> >> So we're still doing the recursive verify. It might makes things
> >> clearer if you added l_cert* stub classes first. Then a separate commit
> >> moving the recursive implementation from tls.c to cert.c.
> >
> > Ugh, that would be a commit for each method being moved? There isn't
> > really much to review if we're just moving the code, however not
> > breaking TLS between the multiple commits could be really complicated.
>
> Still much clearer / preferable than adding a brand new version and then
> removing the old one.
We're not adding a brand new version here though, I tried to be
careful not to suggest this in the commit. The structs are new
though, let me send them as one patch, then move the methods in
another and then switch tls from struct tls_cert to struct l_cert.
Best regards
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] cert: Add cert.c for certificate types and routines
2018-11-09 8:47 ` Andrew Zaborowski
@ 2018-11-09 15:34 ` Denis Kenzior
2018-11-12 10:47 ` Andrew Zaborowski
0 siblings, 1 reply; 14+ messages in thread
From: Denis Kenzior @ 2018-11-09 15:34 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]
Hi Andrew,
On 11/09/2018 02:47 AM, Andrew Zaborowski wrote:
> On Fri, 9 Nov 2018 at 02:05, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> and consider dropping link_issuer completely. Just build a certchain
>>>> from the raw data directly, perhaps as a private constructor for now.
>>>
>>> Could also just do:
>>>
>>> struct l_certchain {
>>> struct l_queue *certs;
>>> };
>>>
>>> for simplicity. We only have one use case for iterating from root to
>>> leaf so we can reverse the queue at that time. We maybe simply want
>>> two public methods, l_certchain_foreach_from_root and
>>> l_certchain_foreach_from_leaf.
>>
>> Yes but a quite common one. And you still need to memdup the l_cert
>> when you add it to the certchain, so maybe just do it right.
>
> By that you mean adding a new l_queue-like structure in cert.c, right?
> Can we think of an API that avoids the memdup of the whole
> certificate?
I can, but it also makes things way too complicated. Honestly I think
we should keep l_certchain fully opaque for now and only expose the
absolute bare minimum.
>
>>
>>>
>>> As for the constructor we already need to parse two different "raw"
>>> formats both of which are defined in pretty distant specs so I don't
>>> think cert.c should know them, I'd rather have a l_certchain_link_cert
>>> or similar.
>>
>> You can also just implement the two constructors directly here, since
>> they're the most common case anyway.
>
> One case is a completely specific to TLS where parse a binary
> structure so there's no reason it shouldn't be in tls.c. The other
We do use the same constructor in tools/certchain-verify. So I don't
buy this argument :)
> case is loading from a PEM file (remember we tried to avoid adding the
> l_pem_load_certificate_list function returning multiple buffers? We
> can't build the certchain in cert.c without that function, or calling
> l_pem_load_file multiple times)
Sure you can. Just have pem.c load all of them and then pass a queue or
an array to a private constructor in cert.c. Or have some fun magic one
that just builds certchain in-order.
But neither of these need to be public at all. I seriously don't see
any usecase for us right now where a client could need to build
certificate chains outside of TLS. So until we have such a use case I
don't see _any_ reason to make these public.
Regards,
-Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] cert: Add cert.c for certificate types and routines
2018-11-09 15:34 ` Denis Kenzior
@ 2018-11-12 10:47 ` Andrew Zaborowski
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Zaborowski @ 2018-11-12 10:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]
Hi Denis,
On Fri, 9 Nov 2018 at 16:34, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/09/2018 02:47 AM, Andrew Zaborowski wrote:
> > On Fri, 9 Nov 2018 at 02:05, Denis Kenzior <denkenz@gmail.com> wrote:
> >>>> and consider dropping link_issuer completely. Just build a certchain
> >>>> from the raw data directly, perhaps as a private constructor for now.
> >>>
> >>> Could also just do:
> >>>
> >>> struct l_certchain {
> >>> struct l_queue *certs;
> >>> };
> >>>
> >>> for simplicity. We only have one use case for iterating from root to
> >>> leaf so we can reverse the queue at that time. We maybe simply want
> >>> two public methods, l_certchain_foreach_from_root and
> >>> l_certchain_foreach_from_leaf.
> >>
> >> Yes but a quite common one. And you still need to memdup the l_cert
> >> when you add it to the certchain, so maybe just do it right.
> >
> > By that you mean adding a new l_queue-like structure in cert.c, right?
> > Can we think of an API that avoids the memdup of the whole
> > certificate?
>
> I can, but it also makes things way too complicated. Honestly I think
> we should keep l_certchain fully opaque for now and only expose the
> absolute bare minimum.
Well, then if we're building the certchain using a private API then we
can probably avoid the memdup more easily. So I'm thinking of having
the following two public functions:
l_pem_load_certificate
l_pem_load_certificate_list
The first one loads a single certificate, the second one loads all the
certificates in a PEM file and returns an l_queue (not a certchain
object), and then TLS uses a private chertchain_from_queue() or
simliar to convert the l_queue into a certchain object.
>
> >
> >>
> >>>
> >>> As for the constructor we already need to parse two different "raw"
> >>> formats both of which are defined in pretty distant specs so I don't
> >>> think cert.c should know them, I'd rather have a l_certchain_link_cert
> >>> or similar.
> >>
> >> You can also just implement the two constructors directly here, since
> >> they're the most common case anyway.
> >
> > One case is a completely specific to TLS where parse a binary
> > structure so there's no reason it shouldn't be in tls.c. The other
>
> We do use the same constructor in tools/certchain-verify. So I don't
> buy this argument :)
This doesn't make it any less TLS-specific, tools/certchain-verify
uses the TLS Certificate.certificate_list format and even uses
tls_cert_from_certificate_list().
Best regards
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-11-12 10:47 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-08 12:06 [PATCH 1/5] cert: Add cert.c for certificate types and routines Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 2/5] tls: Update tls, pem to use the l_cert type, drop local code Andrew Zaborowski
2018-11-08 23:20 ` Mat Martineau
2018-11-08 12:06 ` [PATCH 3/5] pem: Return an l_key from l_pem_load_private_key Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 4/5] tools: Update certchain-verify to new certificate API Andrew Zaborowski
2018-11-08 12:06 ` [PATCH 5/5] unit: Update tests " Andrew Zaborowski
2018-11-08 23:27 ` Mat Martineau
2018-11-08 23:30 ` Andrew Zaborowski
2018-11-08 17:39 ` [PATCH 1/5] cert: Add cert.c for certificate types and routines Denis Kenzior
2018-11-08 23:22 ` Andrew Zaborowski
2018-11-09 1:05 ` Denis Kenzior
2018-11-09 8:47 ` Andrew Zaborowski
2018-11-09 15:34 ` Denis Kenzior
2018-11-12 10:47 ` Andrew Zaborowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.