further x509 cleanup in rpki-client

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

further x509 cleanup in rpki-client

Claudio Jeker
Instead of iterating over all x509 extension and look for SKI and AKI use
X509_get_ext_d2i(). This reduces the complexity a fair bit. Also add
additional checks (e.g. make sure the extensions are non-critical).
More cleanup in cert.c should follow but one step at a time.

--
:wq Claudio

Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.26
diff -u -p -r1.26 cert.c
--- cert.c 16 Feb 2021 07:58:30 -0000 1.26
+++ cert.c 16 Feb 2021 20:04:54 -0000
@@ -1080,19 +1080,10 @@ cert_parse_inner(X509 **xp, const char *
  /* ignored here, handled later */
  break;
  case NID_info_access:
- free(p.res->aia);
- p.res->aia = x509_get_aia(x, p.fn);
- c = (p.res->aia != NULL);
  break;
  case NID_authority_key_identifier:
- free(p.res->aki);
- p.res->aki = x509_get_aki_ext(ext, p.fn);
- c = (p.res->aki != NULL);
  break;
  case NID_subject_key_identifier:
- free(p.res->ski);
- p.res->ski = x509_get_ski_ext(ext, p.fn);
- c = (p.res->ski != NULL);
  break;
  default:
  /* {
@@ -1107,8 +1098,12 @@ cert_parse_inner(X509 **xp, const char *
  goto out;
  }
 
- if (!ta)
+ p.res->aki = x509_get_aki(x, ta, p.fn);
+ p.res->ski = x509_get_ski(x, p.fn);
+ if (!ta) {
+ p.res->aia = x509_get_aia(x, p.fn);
  p.res->crl = x509_get_crl(x, p.fn);
+ }
 
  /* Validation on required fields. */
 
@@ -1131,6 +1126,16 @@ cert_parse_inner(X509 **xp, const char *
  } else if (!ta && strcmp(p.res->aki, p.res->ski) == 0) {
  warnx("%s: RFC 6487 section 8.4.2: "
     "non-trust anchor AKI may not match SKI", p.fn);
+ goto out;
+ }
+
+ if (!ta && p.res->aia == NULL) {
+ warnx("%s: RFC 6487 section 8.4.7: "
+    "non-trust anchor missing AIA", p.fn);
+ goto out;
+ } else if (ta && p.res->aia != NULL) {
+ warnx("%s: RFC 6487 section 8.4.7: "
+    "trust anchor must not have AIA", p.fn);
  goto out;
  }
 
Index: extern.h
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.44
diff -u -p -r1.44 extern.h
--- extern.h 16 Feb 2021 08:52:00 -0000 1.44
+++ extern.h 16 Feb 2021 20:03:40 -0000
@@ -421,12 +421,12 @@ void io_str_read(int, char **);
 /* X509 helpers. */
 
 char *x509_get_aia(X509 *, const char *);
-char *x509_get_aki_ext(X509_EXTENSION *, const char *);
-char *x509_get_ski_ext(X509_EXTENSION *, const char *);
+char *x509_get_aki(X509 *, int, const char *);
+char *x509_get_ski(X509 *, const char *);
 int x509_get_extensions(X509 *, const char *, char **, char **,
  char **);
 char *x509_get_crl(X509 *, const char *);
-char *x509_crl_get_aki(X509_CRL *);
+char *x509_crl_get_aki(X509_CRL *, const char *);
 
 /* Output! */
 
Index: parser.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.4
diff -u -p -r1.4 parser.c
--- parser.c 4 Feb 2021 14:32:01 -0000 1.4
+++ parser.c 16 Feb 2021 17:46:25 -0000
@@ -361,7 +361,8 @@ proc_parser_crl(struct entity *entp, X50
  if ((x509_crl = crl_parse(entp->file)) != NULL) {
  if ((crl = malloc(sizeof(*crl))) == NULL)
  err(1, NULL);
- if ((crl->aki = x509_crl_get_aki(x509_crl)) == NULL)
+ if ((crl->aki = x509_crl_get_aki(x509_crl, entp->file)) ==
+    NULL)
  errx(1, "x509_crl_get_aki failed");
  crl->x509_crl = x509_crl;
 
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.15
diff -u -p -r1.15 x509.c
--- x509.c 16 Feb 2021 07:58:30 -0000 1.15
+++ x509.c 18 Feb 2021 13:28:41 -0000
@@ -30,88 +30,67 @@
 #include "extern.h"
 
 /*
- * Wrapper around ASN1_get_object() that preserves the current start
- * state and returns a more meaningful value.
- * Return zero on failure, non-zero on success.
- */
-static int
-ASN1_frame(const char *fn, size_t sz,
-    const unsigned char **cnt, long *cntsz, int *tag)
-{
- int ret, pcls;
-
- assert(cnt != NULL && *cnt != NULL);
- assert(sz > 0);
- ret = ASN1_get_object(cnt, cntsz, tag, &pcls, sz);
- if ((ret & 0x80)) {
- cryptowarnx("%s: ASN1_get_object", fn);
- return 0;
- }
- return ASN1_object_size((ret & 0x01) ? 2 : 0, *cntsz, *tag);
-}
-
-/*
  * Parse X509v3 authority key identifier (AKI), RFC 6487 sec. 4.8.3.
  * Returns the AKI or NULL if it could not be parsed.
  * The AKI is formatted as aa:bb:cc:dd, with each being a hex value.
  */
 char *
-x509_get_aki_ext(X509_EXTENSION *ext, const char *fn)
+x509_get_aki(X509 *x, int ta, const char *fn)
 {
  const unsigned char *d;
- const ASN1_TYPE *t;
- const ASN1_OCTET_STRING *os = NULL;
- ASN1_SEQUENCE_ANY *seq = NULL;
- int dsz, ptag;
- long i, plen;
+ AUTHORITY_KEYID *akid;
+ ASN1_OCTET_STRING *os;
+ int i, dsz, crit;
  char buf[4];
  char *res = NULL;
 
- assert(NID_authority_key_identifier ==
-    OBJ_obj2nid(X509_EXTENSION_get_object(ext)));
- os = X509_EXTENSION_get_data(ext);
- assert(os != NULL);
-
- d = os->data;
- dsz = os->length;
-
- if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) {
- cryptowarnx("%s: RFC 6487 section 4.8.3: AKI: "
-    "failed ASN.1 sub-sequence parse", fn);
+ akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &crit, NULL);
+ if (akid == NULL) {
+ if (!ta)
+ warnx("%s: RFC 6487 section 4.8.3: AKI: "
+    "extension missing", fn);
+ return NULL;
+ }
+ if (crit != 0) {
+ warnx("%s: RFC 6487 section 4.8.3: "
+    "AKI: extension not non-critical", fn);
  goto out;
  }
- if (sk_ASN1_TYPE_num(seq) != 1) {
+ if (akid->issuer != NULL || akid->serial != NULL) {
  warnx("%s: RFC 6487 section 4.8.3: AKI: "
-    "want 1 element, have %d", fn, sk_ASN1_TYPE_num(seq));
+    "authorityCertIssuer or authorityCertSerialNumber present",
+    fn);
  goto out;
  }
 
- t = sk_ASN1_TYPE_value(seq, 0);
- if (t->type != V_ASN1_OTHER) {
+ os = akid->keyid;
+ if (os == NULL) {
  warnx("%s: RFC 6487 section 4.8.3: AKI: "
-    "want ASN.1 external, have %s (NID %d)",
-    fn, ASN1_tag2str(t->type), t->type);
+    "Key Identifier missing", fn);
  goto out;
  }
 
- d = t->value.asn1_string->data;
- dsz = t->value.asn1_string->length;
+ d = os->data;
+ dsz = os->length;
 
- if (!ASN1_frame(fn, dsz, &d, &plen, &ptag))
+ if (dsz != 20) {
+ warnx("%s: RFC 6487 section 4.8.2: AKI: "
+    "want 20 bytes SHA1 hash, have %d bytes", fn, dsz);
  goto out;
+ }
 
  /* Make room for [hex1, hex2, ":"]*, NUL. */
 
- if ((res = calloc(plen * 3 + 1, 1)) == NULL)
+ if ((res = calloc(dsz * 3 + 1, 1)) == NULL)
  err(1, NULL);
 
- for (i = 0; i < plen; i++) {
+ for (i = 0; i < dsz; i++) {
  snprintf(buf, sizeof(buf), "%02X:", d[i]);
- strlcat(res, buf, plen * 3 + 1);
+ strlcat(res, buf, dsz * 3 + 1);
  }
- res[plen * 3 - 1] = '\0';
+ res[dsz * 3 - 1] = '\0';
 out:
- sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free);
+ AUTHORITY_KEYID_free(akid);
  return res;
 }
 
@@ -121,35 +100,31 @@ out:
  * The SKI is formatted as aa:bb:cc:dd, with each being a hex value.
  */
 char *
-x509_get_ski_ext(X509_EXTENSION *ext, const char *fn)
+x509_get_ski(X509 *x, const char *fn)
 {
  const unsigned char *d;
- const ASN1_OCTET_STRING *os;
- ASN1_OCTET_STRING *oss = NULL;
- int i, dsz;
+ ASN1_OCTET_STRING *os;
+ int i, dsz, crit;
  char buf[4];
  char *res = NULL;
 
- assert(NID_subject_key_identifier ==
-    OBJ_obj2nid(X509_EXTENSION_get_object(ext)));
-
- os = X509_EXTENSION_get_data(ext);
- assert(os != NULL);
- d = os->data;
- dsz = os->length;
-
- if ((oss = d2i_ASN1_OCTET_STRING(NULL, &d, dsz)) == NULL) {
- cryptowarnx("%s: RFC 6487 section 4.8.2: SKI: "
-    "failed ASN.1 octet string parse", fn);
+ os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
+ if (os == NULL) {
+ warnx("%s: RFC 6487 section 4.8.2: SKI: extension missing", fn);
+ return NULL;
+ }
+ if (crit != 0) {
+ warnx("%s: RFC 6487 section 4.8.2: "
+    "SKI: extension not non-critical", fn);
  goto out;
  }
 
- d = oss->data;
- dsz = oss->length;
+ d = os->data;
+ dsz = os->length;
 
  if (dsz != 20) {
  warnx("%s: RFC 6487 section 4.8.2: SKI: "
-    "want 20 B SHA1 hash, have %d B", fn, dsz);
+    "want 20 bytes SHA1 hash, have %d bytes", fn, dsz);
  goto out;
  }
 
@@ -164,7 +139,7 @@ x509_get_ski_ext(X509_EXTENSION *ext, co
  }
  res[dsz * 3 - 1] = '\0';
 out:
- ASN1_OCTET_STRING_free(oss);
+ ASN1_OCTET_STRING_free(os);
  return res;
 }
 
@@ -180,12 +155,18 @@ x509_get_aia(X509 *x, const char *fn)
  ACCESS_DESCRIPTION *ad;
  AUTHORITY_INFO_ACCESS *info;
  char *aia = NULL;
+ int crit;
 
- info = X509_get_ext_d2i(x, NID_info_access, NULL, NULL);
+ info = X509_get_ext_d2i(x, NID_info_access, &crit, NULL);
  if (info == NULL) {
  warnx("%s: RFC 6487 section 4.8.7: AIA: extension missing", fn);
  return NULL;
  }
+ if (crit != 0) {
+ warnx("%s: RFC 6487 section 4.8.7: "
+    "AIA: extension not non-critical", fn);
+ goto out;
+ }
  if (sk_ACCESS_DESCRIPTION_num(info) != 1) {
  warnx("%s: RFC 6487 section 4.8.7: AIA: "
     "want 1 element, have %d", fn,
@@ -217,45 +198,21 @@ out:
 }
 
 /*
- * Wraps around x509_get_ski_ext, x509_get_aki_ext, and x509_get_aia.
+ * Wraps around x509_get_aia, x509_get_aki, and x509_get_ski.
  * Returns zero on failure (out pointers are NULL) or non-zero on
  * success (out pointers must be freed).
  */
 int
 x509_get_extensions(X509 *x, const char *fn, char **aia, char **aki, char **ski)
 {
- X509_EXTENSION *ext = NULL;
- const ASN1_OBJECT *obj;
- int extsz, i;
-
  *aia = *aki = *ski = NULL;
 
- if ((extsz = X509_get_ext_count(x)) < 0)
- cryptoerrx("X509_get_ext_count");
-
- for (i = 0; i < extsz; i++) {
- ext = X509_get_ext(x, i);
- assert(ext != NULL);
- obj = X509_EXTENSION_get_object(ext);
- assert(obj != NULL);
- switch (OBJ_obj2nid(obj)) {
- case NID_subject_key_identifier:
- free(*ski);
- *ski = x509_get_ski_ext(ext, fn);
- break;
- case NID_authority_key_identifier:
- free(*aki);
- *aki = x509_get_aki_ext(ext, fn);
- break;
- case NID_info_access:
- free(*aia);
- *aia = x509_get_aia(x, fn);
- break;
- }
- }
+ *aia = x509_get_aia(x, fn);
+ *aki = x509_get_aki(x, 0, fn);
+ *ski = x509_get_ski(x, fn);
 
  if (*aia == NULL || *aki == NULL || *ski == NULL) {
- cryptowarnx("%s: RFC 6487 section 4.8: "
+ warnx("%s: RFC 6487 section 4.8: "
     "missing AIA, AKI or SKI X509 extension", fn);
  free(*aia);
  free(*aki);
@@ -277,49 +234,55 @@ x509_get_extensions(X509 *x, const char
 char *
 x509_get_crl(X509 *x, const char *fn)
 {
- STACK_OF(DIST_POINT) *crldp;
+ CRL_DIST_POINTS *crldp;
  DIST_POINT *dp;
  GENERAL_NAME *name;
- char *crl;
+ char *crl = NULL;
+ int crit;
 
- crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, NULL, NULL);
+ crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, &crit, NULL);
  if (crldp == NULL) {
  warnx("%s: RFC 6487 section 4.8.6: CRL: "
     "no CRL distribution point extension", fn);
  return NULL;
  }
+ if (crit != 0) {
+ warnx("%s: RFC 6487 section 4.8.6: "
+    "CRL distribution point: extension not non-critical", fn);
+ goto out;
+ }
 
  if (sk_DIST_POINT_num(crldp) != 1) {
  warnx("%s: RFC 6487 section 4.8.6: CRL: "
     "want 1 element, have %d", fn,
     sk_DIST_POINT_num(crldp));
- return NULL;
+ goto out;
  }
 
  dp = sk_DIST_POINT_value(crldp, 0);
  if (dp->distpoint == NULL) {
  warnx("%s: RFC 6487 section 4.8.6: CRL: "
     "no distribution point name", fn);
- return NULL;
+ goto out;
  }
  if (dp->distpoint->type != 0) {
  warnx("%s: RFC 6487 section 4.8.6: CRL: "
     "expected GEN_OTHERNAME, have %d", fn, dp->distpoint->type);
- return NULL;
+ goto out;
  }
 
  if (sk_GENERAL_NAME_num(dp->distpoint->name.fullname) != 1) {
  warnx("%s: RFC 6487 section 4.8.6: CRL: "
     "want 1 full name, have %d", fn,
     sk_GENERAL_NAME_num(dp->distpoint->name.fullname));
- return NULL;
+ goto out;
  }
 
  name = sk_GENERAL_NAME_value(dp->distpoint->name.fullname, 0);
  if (name->type != GEN_URI) {
  warnx("%s: RFC 6487 section 4.8.6: CRL: "
     "want URI type, have %d", fn, name->type);
- return NULL;
+ goto out;
  }
 
  crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier),
@@ -327,21 +290,66 @@ x509_get_crl(X509 *x, const char *fn)
  if (crl == NULL)
  err(1, NULL);
 
+out:
+ CRL_DIST_POINTS_free(crldp);
  return crl;
 }
 
 char *
-x509_crl_get_aki(X509_CRL *crl)
+x509_crl_get_aki(X509_CRL *crl, const char *fn)
 {
- X509_EXTENSION *ext;
- int loc;
+ const unsigned char *d;
+ AUTHORITY_KEYID *akid;
+ ASN1_OCTET_STRING *os;
+ int i, dsz, crit;
+ char buf[4];
+ char *res = NULL;
 
- loc = X509_CRL_get_ext_by_NID(crl, NID_authority_key_identifier, -1);
- if (loc == -1) {
- warnx("%s: CRL without AKI extension", __func__);
+ akid = X509_CRL_get_ext_d2i(crl, NID_authority_key_identifier, &crit,
+    NULL);
+ if (akid == NULL) {
+ warnx("%s: RFC 6487 section 4.8.3: AKI: extension missing", fn);
  return NULL;
  }
- ext = X509_CRL_get_ext(crl, loc);
+ if (crit != 0) {
+ warnx("%s: RFC 6487 section 4.8.3: "
+    "AKI: extension not non-critical", fn);
+ goto out;
+ }
+ if (akid->issuer != NULL || akid->serial != NULL) {
+ warnx("%s: RFC 6487 section 4.8.3: AKI: "
+    "authorityCertIssuer or authorityCertSerialNumber present",
+    fn);
+ goto out;
+ }
+
+ os = akid->keyid;
+ if (os == NULL) {
+ warnx("%s: RFC 6487 section 4.8.3: AKI: "
+    "Key Identifier missing", fn);
+ goto out;
+ }
 
- return x509_get_aki_ext(ext, "x509_crl_get_aki");
+ d = os->data;
+ dsz = os->length;
+
+ if (dsz != 20) {
+ warnx("%s: RFC 6487 section 4.8.2: AKI: "
+    "want 20 bytes SHA1 hash, have %d bytes", fn, dsz);
+ goto out;
+ }
+
+ /* Make room for [hex1, hex2, ":"]*, NUL. */
+
+ if ((res = calloc(dsz * 3 + 1, 1)) == NULL)
+ err(1, NULL);
+
+ for (i = 0; i < dsz; i++) {
+ snprintf(buf, sizeof(buf), "%02X:", d[i]);
+ strlcat(res, buf, dsz * 3 + 1);
+ }
+ res[dsz * 3 - 1] = '\0';
+out:
+ AUTHORITY_KEYID_free(akid);
+ return res;
 }

Reply | Threaded
Open this post in threaded view
|

Re: further x509 cleanup in rpki-client

Theo Buehler-3
On Thu, Feb 18, 2021 at 02:41:39PM +0100, Claudio Jeker wrote:
> Instead of iterating over all x509 extension and look for SKI and AKI use
> X509_get_ext_d2i(). This reduces the complexity a fair bit. Also add
> additional checks (e.g. make sure the extensions are non-critical).
> More cleanup in cert.c should follow but one step at a time.

ok tb