Document the X509V3_get_d2i family of functions.
These are a bit of a mess. Callers almost never handle the error
correctly.
Change-Id: I85ea6d4c03cca685f0be579459efb66fea996c9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43804
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/x509/x509_ext.c b/crypto/x509/x509_ext.c
index a8f0ab6..f6da54a 100644
--- a/crypto/x509/x509_ext.c
+++ b/crypto/x509/x509_ext.c
@@ -93,9 +93,10 @@
return (X509v3_delete_ext(x->crl->extensions, loc));
}
-void *X509_CRL_get_ext_d2i(const X509_CRL *x, int nid, int *crit, int *idx)
+void *X509_CRL_get_ext_d2i(const X509_CRL *crl, int nid, int *out_critical,
+ int *out_idx)
{
- return X509V3_get_d2i(x->crl->extensions, nid, crit, idx);
+ return X509V3_get_d2i(crl->crl->extensions, nid, out_critical, out_idx);
}
int X509_CRL_add1_ext_i2d(X509_CRL *x, int nid, void *value, int crit,
@@ -145,9 +146,11 @@
return (X509v3_add_ext(&(x->cert_info->extensions), ex, loc) != NULL);
}
-void *X509_get_ext_d2i(const X509 *x, int nid, int *crit, int *idx)
+void *X509_get_ext_d2i(const X509 *x509, int nid, int *out_critical,
+ int *out_idx)
{
- return X509V3_get_d2i(x->cert_info->extensions, nid, crit, idx);
+ return X509V3_get_d2i(x509->cert_info->extensions, nid, out_critical,
+ out_idx);
}
int X509_add1_ext_i2d(X509 *x, int nid, void *value, int crit,
@@ -194,10 +197,10 @@
return (X509v3_add_ext(&(x->extensions), ex, loc) != NULL);
}
-void *X509_REVOKED_get_ext_d2i(const X509_REVOKED *x, int nid, int *crit,
- int *idx)
+void *X509_REVOKED_get_ext_d2i(const X509_REVOKED *revoked, int nid,
+ int *out_critical, int *out_idx)
{
- return X509V3_get_d2i(x->extensions, nid, crit, idx);
+ return X509V3_get_d2i(revoked->extensions, nid, out_critical, out_idx);
}
int X509_REVOKED_add1_ext_i2d(X509_REVOKED *x, int nid, void *value, int crit,
diff --git a/crypto/x509v3/v3_lib.c b/crypto/x509v3/v3_lib.c
index d5eda3d..d89733f 100644
--- a/crypto/x509v3/v3_lib.c
+++ b/crypto/x509v3/v3_lib.c
@@ -122,7 +122,7 @@
return sk_X509V3_EXT_METHOD_value(ext_list, idx);
}
-const X509V3_EXT_METHOD *X509V3_EXT_get(X509_EXTENSION *ext)
+const X509V3_EXT_METHOD *X509V3_EXT_get(const X509_EXTENSION *ext)
{
int nid;
if ((nid = OBJ_obj2nid(ext->object)) == NID_undef)
@@ -203,7 +203,7 @@
/* Return an extension internal structure */
-void *X509V3_EXT_d2i(X509_EXTENSION *ext)
+void *X509V3_EXT_d2i(const X509_EXTENSION *ext)
{
const X509V3_EXT_METHOD *method;
const unsigned char *p;
@@ -217,49 +217,38 @@
return method->d2i(NULL, &p, ext->value->length);
}
-/*
- * Get critical flag and decoded version of extension from a NID. The "idx"
- * variable returns the last found extension and can be used to retrieve
- * multiple extensions of the same NID. However multiple extensions with the
- * same NID is usually due to a badly encoded certificate so if idx is NULL
- * we choke if multiple extensions exist. The "crit" variable is set to the
- * critical value. The return value is the decoded extension or NULL on
- * error. The actual error can have several different causes, the value of
- * *crit reflects the cause: >= 0, extension found but not decoded (reflects
- * critical value). -1 extension not found. -2 extension occurs more than
- * once.
- */
-
-void *X509V3_get_d2i(STACK_OF(X509_EXTENSION) *x, int nid, int *crit,
- int *idx)
+void *X509V3_get_d2i(const STACK_OF(X509_EXTENSION) *extensions, int nid,
+ int *out_critical, int *out_idx)
{
int lastpos;
size_t i;
X509_EXTENSION *ex, *found_ex = NULL;
- if (!x) {
- if (idx)
- *idx = -1;
- if (crit)
- *crit = -1;
+ if (!extensions) {
+ if (out_idx)
+ *out_idx = -1;
+ if (out_critical)
+ *out_critical = -1;
return NULL;
}
- if (idx)
- lastpos = *idx + 1;
+ if (out_idx)
+ lastpos = *out_idx + 1;
else
lastpos = 0;
if (lastpos < 0)
lastpos = 0;
- for (i = lastpos; i < sk_X509_EXTENSION_num(x); i++) {
- ex = sk_X509_EXTENSION_value(x, i);
+ for (i = lastpos; i < sk_X509_EXTENSION_num(extensions); i++) {
+ ex = sk_X509_EXTENSION_value(extensions, i);
if (OBJ_obj2nid(ex->object) == nid) {
- if (idx) {
- *idx = i;
+ if (out_idx) {
+ /* TODO(https://crbug.com/boringssl/379): Consistently reject
+ * duplicate extensions. */
+ *out_idx = i;
found_ex = ex;
break;
} else if (found_ex) {
/* Found more than one */
- if (crit)
- *crit = -2;
+ if (out_critical)
+ *out_critical = -2;
return NULL;
}
found_ex = ex;
@@ -267,16 +256,16 @@
}
if (found_ex) {
/* Found it */
- if (crit)
- *crit = X509_EXTENSION_get_critical(found_ex);
+ if (out_critical)
+ *out_critical = X509_EXTENSION_get_critical(found_ex);
return X509V3_EXT_d2i(found_ex);
}
/* Extension not found */
- if (idx)
- *idx = -1;
- if (crit)
- *crit = -1;
+ if (out_idx)
+ *out_idx = -1;
+ if (out_critical)
+ *out_critical = -1;
return NULL;
}
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 580c591..aa32f0f 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -1238,7 +1238,15 @@
OPENSSL_EXPORT X509_EXTENSION *X509_get_ext(const X509 *x, int loc);
OPENSSL_EXPORT X509_EXTENSION *X509_delete_ext(X509 *x, int loc);
OPENSSL_EXPORT int X509_add_ext(X509 *x, X509_EXTENSION *ex, int loc);
-OPENSSL_EXPORT void *X509_get_ext_d2i(const X509 *x, int nid, int *crit, int *idx);
+
+// X509_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the extension in
+// |x509|'s extension list.
+//
+// WARNING: This function is difficult to use correctly. See the documentation
+// for |X509V3_get_d2i| for details.
+OPENSSL_EXPORT void *X509_get_ext_d2i(const X509 *x509, int nid,
+ int *out_critical, int *out_idx);
+
OPENSSL_EXPORT int X509_add1_ext_i2d(X509 *x, int nid, void *value, int crit,
unsigned long flags);
@@ -1251,8 +1259,15 @@
OPENSSL_EXPORT X509_EXTENSION *X509_CRL_get_ext(const X509_CRL *x, int loc);
OPENSSL_EXPORT X509_EXTENSION *X509_CRL_delete_ext(X509_CRL *x, int loc);
OPENSSL_EXPORT int X509_CRL_add_ext(X509_CRL *x, X509_EXTENSION *ex, int loc);
-OPENSSL_EXPORT void *X509_CRL_get_ext_d2i(const X509_CRL *x, int nid, int *crit,
- int *idx);
+
+// X509_CRL_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the
+// extension in |crl|'s extension list.
+//
+// WARNING: This function is difficult to use correctly. See the documentation
+// for |X509V3_get_d2i| for details.
+OPENSSL_EXPORT void *X509_CRL_get_ext_d2i(const X509_CRL *crl, int nid,
+ int *out_critical, int *out_idx);
+
OPENSSL_EXPORT int X509_CRL_add1_ext_i2d(X509_CRL *x, int nid, void *value,
int crit, unsigned long flags);
@@ -1270,8 +1285,16 @@
int loc);
OPENSSL_EXPORT int X509_REVOKED_add_ext(X509_REVOKED *x, X509_EXTENSION *ex,
int loc);
-OPENSSL_EXPORT void *X509_REVOKED_get_ext_d2i(const X509_REVOKED *x, int nid,
- int *crit, int *idx);
+
+// X509_REVOKED_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the
+// extension in |revoked|'s extension list.
+//
+// WARNING: This function is difficult to use correctly. See the documentation
+// for |X509V3_get_d2i| for details.
+OPENSSL_EXPORT void *X509_REVOKED_get_ext_d2i(const X509_REVOKED *revoked,
+ int nid, int *out_critical,
+ int *out_idx);
+
OPENSSL_EXPORT int X509_REVOKED_add1_ext_i2d(X509_REVOKED *x, int nid,
void *value, int crit,
unsigned long flags);
diff --git a/include/openssl/x509v3.h b/include/openssl/x509v3.h
index 0a4e776..14d5e95 100644
--- a/include/openssl/x509v3.h
+++ b/include/openssl/x509v3.h
@@ -633,13 +633,53 @@
OPENSSL_EXPORT int X509V3_EXT_add_alias(int nid_to, int nid_from);
OPENSSL_EXPORT void X509V3_EXT_cleanup(void);
-OPENSSL_EXPORT const X509V3_EXT_METHOD *X509V3_EXT_get(X509_EXTENSION *ext);
+OPENSSL_EXPORT const X509V3_EXT_METHOD *X509V3_EXT_get(
+ const X509_EXTENSION *ext);
OPENSSL_EXPORT const X509V3_EXT_METHOD *X509V3_EXT_get_nid(int nid);
OPENSSL_EXPORT int X509V3_add_standard_extensions(void);
OPENSSL_EXPORT STACK_OF(CONF_VALUE) *X509V3_parse_list(const char *line);
-OPENSSL_EXPORT void *X509V3_EXT_d2i(X509_EXTENSION *ext);
-OPENSSL_EXPORT void *X509V3_get_d2i(STACK_OF(X509_EXTENSION) *x, int nid,
- int *crit, int *idx);
+
+// X509V3_EXT_d2i decodes |ext| and returns a pointer to a newly-allocated
+// structure, with type dependent on the type of the extension. It returns NULL
+// if |ext| is an unsupported extension or if there was a syntax error in the
+// extension. The caller should cast the return value to the expected type and
+// free the structure when done.
+//
+// WARNING: Casting the return value to the wrong type is a potentially
+// exploitable memory error, so callers must not use this function before
+// checking |ext| is of a known type.
+OPENSSL_EXPORT void *X509V3_EXT_d2i(const X509_EXTENSION *ext);
+
+// X509V3_get_d2i finds and decodes the extension in |extensions| of type |nid|.
+// If found, it decodes it and returns a newly-allocated structure, with type
+// dependent on |nid|. If the extension is not found or on error, it returns
+// NULL. The caller may distinguish these cases using the |out_critical| value.
+//
+// If |out_critical| is not NULL, this function sets |*out_critical| to one if
+// the extension is found and critical, zero if it is found and not critical, -1
+// if it is not found, and -2 if there is an invalid duplicate extension. Note
+// this function may set |*out_critical| to one or zero and still return NULL if
+// the extension is found but has a syntax error.
+//
+// If |out_idx| is not NULL, this function looks for the first occurrence of the
+// extension after |*out_idx|. It then sets |*out_idx| to the index of the
+// extension, or -1 if not found. If |out_idx| is non-NULL, duplicate extensions
+// are not treated as an error. Callers, however, should not rely on this
+// behavior as it may be removed in the future. Duplicate extensions are
+// forbidden in RFC5280.
+//
+// WARNING: This function is difficult to use correctly. Callers should pass a
+// non-NULL |out_critical| and check both the return value and |*out_critical|
+// to handle errors. If the return value is NULL and |*out_critical| is not -1,
+// there was an error. Otherwise, the function succeeded and but may return NULL
+// for a missing extension. Callers should pass NULL to |out_idx| so that
+// duplicate extensions are handled correctly.
+//
+// Additionally, casting the return value to the wrong type is a potentially
+// exploitable memory error, so callers must ensure the cast and |nid| match.
+OPENSSL_EXPORT void *X509V3_get_d2i(const STACK_OF(X509_EXTENSION) *extensions,
+ int nid, int *out_critical, int *out_idx);
+
OPENSSL_EXPORT int X509V3_EXT_free(int nid, void *ext_data);
