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>

crypto/x509/x509_ext.c

@@ -93,9 +93,10 @@ X509_EXTENSION *X509_CRL_delete_ext(X509_CRL *x, int loc)
     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 @@ int X509_add_ext(X509 *x, X509_EXTENSION *ex, int loc)
     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 @@ int X509_REVOKED_add_ext(X509_REVOKED *x, X509_EXTENSION *ex, int loc)
     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,

crypto/x509v3/v3_lib.c

@@ -122,7 +122,7 @@ const X509V3_EXT_METHOD *X509V3_EXT_get_nid(int nid)
     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 @@ int X509V3_add_standard_extensions(void)
 
 /* 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 @@ void *X509V3_EXT_d2i(X509_EXTENSION *ext)
     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 @@ void *X509V3_get_d2i(STACK_OF(X509_EXTENSION) *x, int nid, int *crit,
     }
     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;
 }
 

include/openssl/x509.h

@@ -1238,7 +1238,15 @@ OPENSSL_EXPORT int X509_get_ext_by_critical(const X509 *x, int crit,
 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 int X509_CRL_get_ext_by_critical(const X509_CRL *x, int crit,
 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 @@ OPENSSL_EXPORT X509_EXTENSION *X509_REVOKED_delete_ext(X509_REVOKED *x,
                                                        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);

include/openssl/x509v3.h

@@ -633,13 +633,53 @@ OPENSSL_EXPORT int X509V3_EXT_add_list(X509V3_EXT_METHOD *extlist);
 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);