Document ASN1_TYPE and related functions.

The representation here is a bit more messy than necessary. In doing so,
clean up the variable names and smooth away two rough edges:

- X509_ALGOR_get0 would leave *out_param_value uninitialized if
  *out_param_type is V_ASN1_UNDEF. Instead, set it to NULL, so callers
  do not accidentally use an uninitialized pointer.

- X509_PUBKEY_set0_param, if key is NULL, would leave the key alone. No
  one calls this function externally and none of the (since removed)
  callers in OpenSSL rely on this behavior. A NULL check here adds a
  discontinuity at the empty string that seems unnecessary here:
  changing the algorithm without changing the key isn't useful.
  (Note the API doesn't support changing the key without the algorithm.)

Note for reviewing: the representation of ASN1_TYPE is specified
somewhat indirectly. ASN1_TYPE uses the ASN1_ANY ASN1_ITEM, which has
utype V_ASN1_ANY. Then you look at asn1_d2i_ex_primitive and asn1_ex_c2i
which peel off the ASN1_TYPE layer and parse directly into the value
field, with a fixup for NULL. Hopefully we can rework this someday...

Change-Id: I628c4e20f8ea2fd036132242337f4dcac5ba5015
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46165
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/asn1_lib.c

@@ -370,8 +370,7 @@ int ASN1_STRING_set(ASN1_STRING *str, const void *_data, int len)
 
 void ASN1_STRING_set0(ASN1_STRING *str, void *data, int len)
 {
-    if (str->data)
-        OPENSSL_free(str->data);
+    OPENSSL_free(str->data);
     str->data = data;
     str->length = len;
 }

crypto/x509/x_algor.c

@@ -103,19 +103,23 @@ int X509_ALGOR_set0(X509_ALGOR *alg, ASN1_OBJECT *aobj, int ptype, void *pval)
     return 1;
 }
 
-void X509_ALGOR_get0(const ASN1_OBJECT **paobj, int *pptype, const void **ppval,
-                     const X509_ALGOR *algor)
+void X509_ALGOR_get0(const ASN1_OBJECT **out_obj, int *out_param_type,
+                     const void **out_param_value, const X509_ALGOR *alg)
 {
-    if (paobj)
-        *paobj = algor->algorithm;
-    if (pptype) {
-        if (algor->parameter == NULL) {
-            *pptype = V_ASN1_UNDEF;
-            return;
-        } else
-            *pptype = algor->parameter->type;
-        if (ppval)
-            *ppval = algor->parameter->value.ptr;
+    if (out_obj != NULL) {
+        *out_obj = alg->algorithm;
+    }
+    if (out_param_type != NULL) {
+        int type = V_ASN1_UNDEF;
+        void *value = NULL;
+        if (alg->parameter != NULL) {
+            type = alg->parameter->type;
+            value = alg->parameter->value.ptr;
+        }
+        *out_param_type = type;
+        if (out_param_value != NULL) {
+            *out_param_value = value;
+        }
     }
 }
 

crypto/x509/x_pubkey.c

@@ -180,34 +180,33 @@ EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key)
     return NULL;
 }
 
-int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *aobj, int ptype,
-                           void *pval, unsigned char *penc, int penclen)
+int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *obj, int param_type,
+                           void *param_value, uint8_t *key, int key_len)
 {
-    if (!X509_ALGOR_set0(pub->algor, aobj, ptype, pval))
+    if (!X509_ALGOR_set0(pub->algor, obj, param_type, param_value)) {
         return 0;
-    if (penc) {
-        if (pub->public_key->data)
-            OPENSSL_free(pub->public_key->data);
-        pub->public_key->data = penc;
-        pub->public_key->length = penclen;
-        /* Set number of unused bits to zero */
-        pub->public_key->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07);
-        pub->public_key->flags |= ASN1_STRING_FLAG_BITS_LEFT;
     }
+
+    ASN1_STRING_set0(pub->public_key, key, key_len);
+    /* Set the number of unused bits to zero. */
+    pub->public_key->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07);
+    pub->public_key->flags |= ASN1_STRING_FLAG_BITS_LEFT;
     return 1;
 }
 
-int X509_PUBKEY_get0_param(ASN1_OBJECT **ppkalg,
-                           const unsigned char **pk, int *ppklen,
-                           X509_ALGOR **pa, X509_PUBKEY *pub)
+int X509_PUBKEY_get0_param(ASN1_OBJECT **out_obj, const uint8_t **out_key,
+                           int *out_key_len, X509_ALGOR **out_alg,
+                           X509_PUBKEY *pub)
 {
-    if (ppkalg)
-        *ppkalg = pub->algor->algorithm;
-    if (pk) {
-        *pk = pub->public_key->data;
-        *ppklen = pub->public_key->length;
+    if (out_obj != NULL) {
+        *out_obj = pub->algor->algorithm;
+    }
+    if (out_key != NULL) {
+        *out_key = pub->public_key->data;
+        *out_key_len = pub->public_key->length;
+    }
+    if (out_alg != NULL) {
+        *out_alg = pub->algor;
     }
-    if (pa)
-        *pa = pub->algor;
     return 1;
 }

include/openssl/asn1.h

@@ -197,9 +197,8 @@ extern "C" {
 // the DER encoding of the value. For example, the UNIX epoch would be
 // "19700101000000Z" for a GeneralizedTime and "700101000000Z" for a UTCTime.
 //
-// TODO(davidben): |ASN1_TYPE| additionally uses |ASN1_STRING| to represent
-// various other odd cases. It also likes to assume unknown universal tags are
-// string types. Make a note here when documenting |ASN1_TYPE|.
+// |ASN1_STRING|, when stored in an |ASN1_TYPE|, may also represent an element
+// with tag not directly supported by this library. See |ASN1_TYPE| for details.
 //
 // |ASN1_STRING| additionally has the following typedefs: |ASN1_BIT_STRING|,
 // |ASN1_BMPSTRING|, |ASN1_ENUMERATED|, |ASN1_GENERALIZEDTIME|,
@@ -313,6 +312,114 @@ OPENSSL_EXPORT void ASN1_STRING_set0(ASN1_STRING *str, void *data, int len);
 // types.
 
 
+// Arbitrary elements.
+
+// ASN1_VALUE_st (aka |ASN1_VALUE|) is an opaque type used internally in the
+// library.
+typedef struct ASN1_VALUE_st ASN1_VALUE;
+
+// An asn1_type_st (aka |ASN1_TYPE|) represents an arbitrary ASN.1 element,
+// typically used used for ANY types. It contains a |type| field and a |value|
+// union dependent on |type|.
+//
+// WARNING: This struct has a complex representation. Callers performing
+// non-trivial operations on this type are encouraged to use |CBS| and |CBB|
+// from <openssl/bytestring.h>, and convert to or from |ASN1_TYPE| with
+// |d2i_ASN1_TYPE| or |i2d_ASN1_TYPE|.
+//
+// The |type| field corresponds to the tag of the ASN.1 element being
+// represented:
+//
+// If |type| is a |V_ASN1_*| constant for an ASN.1 string-like type, as defined
+// by |ASN1_STRING|, the tag matches the constant. |value| contains an
+// |ASN1_STRING| pointer (equivalently, one of the more specific typedefs). See
+// |ASN1_STRING| for details on the representation. Unlike |ASN1_STRING|,
+// |ASN1_TYPE| does not use the |V_ASN1_NEG| flag for negative INTEGER and
+// ENUMERATE values. For a negative value, the |ASN1_TYPE|'s |type| will be
+// |V_ASN1_INTEGER| or |V_ASN1_ENUMERATED|, but |value| will an |ASN1_STRING|
+// whose |type| is |V_ASN1_NEG_INTEGER| or |V_ASN1_NEG_ENUMERATED|.
+//
+// If |type| is |V_ASN1_OBJECT|, the tag is OBJECT IDENTIFIER and |value|
+// contains an |ASN1_OBJECT| pointer.
+//
+// If |type| is |V_ASN1_NULL|, the tag is NULL. |value| contains a NULL pointer.
+//
+// If |type| is |V_ASN1_BOOLEAN|, the tag is BOOLEAN. |value| contains an
+// |ASN1_BOOLEAN|. While |ASN1_BOOLEAN| is an |int|, any unused bits of |value|
+// must also be zero, so callers must not construct these values manually.
+//
+// TODO(davidben): Fix code which relies on this. It's anything which looks at
+// |value.ptr| or |value.asn1_value| unconditionally.
+//
+// If |type| is |V_ASN1_SEQUENCE|, |V_ASN1_SET|, or |V_ASN1_OTHER|, the tag is
+// SEQUENCE, SET, or some non-universal tag, respectively. |value| is an
+// |ASN1_STRING| containing the entire element, including the tag and length.
+// The |ASN1_STRING|'s |type| field matches the containing |ASN1_TYPE|'s |type|.
+//
+// Other positive values of |type|, up to |V_ASN1_MAX_UNIVERSAL|, correspond to
+// universal primitive tags not directly supported by this library. |value| is
+// an |ASN1_STRING| containing the body of the element, excluding the tag
+// and length. The |ASN1_STRING|'s |type| field matches the containing
+// |ASN1_TYPE|'s |type|.
+struct asn1_type_st {
+  int type;
+  union {
+    char *ptr;
+    ASN1_BOOLEAN boolean;
+    ASN1_STRING *asn1_string;
+    ASN1_OBJECT *object;
+    ASN1_INTEGER *integer;
+    ASN1_ENUMERATED *enumerated;
+    ASN1_BIT_STRING *bit_string;
+    ASN1_OCTET_STRING *octet_string;
+    ASN1_PRINTABLESTRING *printablestring;
+    ASN1_T61STRING *t61string;
+    ASN1_IA5STRING *ia5string;
+    ASN1_GENERALSTRING *generalstring;
+    ASN1_BMPSTRING *bmpstring;
+    ASN1_UNIVERSALSTRING *universalstring;
+    ASN1_UTCTIME *utctime;
+    ASN1_GENERALIZEDTIME *generalizedtime;
+    ASN1_VISIBLESTRING *visiblestring;
+    ASN1_UTF8STRING *utf8string;
+    // set and sequence are left complete and still contain the entire element.
+    ASN1_STRING *set;
+    ASN1_STRING *sequence;
+    ASN1_VALUE *asn1_value;
+  } value;
+};
+
+// ASN1_TYPE_get returns the type of |a|, which will be one of the |V_ASN1_*|
+// constants, or zero if |a| is not fully initialized.
+OPENSSL_EXPORT int ASN1_TYPE_get(const ASN1_TYPE *a);
+
+// ASN1_TYPE_set sets |a| to an |ASN1_TYPE| of type |type| and value |value|,
+// releasing the previous contents of |a|.
+//
+// If |type| is |V_ASN1_BOOLEAN|, |a| is set to FALSE if |value| is NULL and
+// TRUE otherwise. If setting |a| to TRUE, |value| may be an invalid pointer,
+// such as (void*)1.
+//
+// If |type| is |V_ASN1_NULL|, |value| must be NULL.
+//
+// For other values of |type|, this function takes ownership of |value|, which
+// must point to an object of the corresponding type. See |ASN1_TYPE| for
+// details.
+OPENSSL_EXPORT void ASN1_TYPE_set(ASN1_TYPE *a, int type, void *value);
+
+// ASN1_TYPE_set1 behaves like |ASN1_TYPE_set| except it does not take ownership
+// of |value|. It returns one on success and zero on error.
+OPENSSL_EXPORT int ASN1_TYPE_set1(ASN1_TYPE *a, int type, const void *value);
+
+// ASN1_TYPE_cmp returns zero if |a| and |b| are equal and some non-zero value
+// otherwise. Note this function can only be used for equality checks, not an
+// ordering.
+OPENSSL_EXPORT int ASN1_TYPE_cmp(const ASN1_TYPE *a, const ASN1_TYPE *b);
+
+// TODO(davidben): Most of |ASN1_TYPE|'s APIs are hidden behind macros. Expand
+// the macros, document them, and move them to this section.
+
+
 // Underdocumented functions.
 //
 // The following functions are not yet documented and organized.
@@ -424,8 +531,6 @@ typedef struct asn1_string_table_st {
 // see asn1t.h
 typedef struct ASN1_TEMPLATE_st ASN1_TEMPLATE;
 typedef struct ASN1_TLC_st ASN1_TLC;
-// This is just an opaque pointer
-typedef struct ASN1_VALUE_st ASN1_VALUE;
 
 // Declare ASN1 functions: the implement macro in in asn1t.h
 
@@ -591,35 +696,6 @@ typedef const ASN1_ITEM ASN1_ITEM_EXP;
 DEFINE_STACK_OF(ASN1_INTEGER)
 DECLARE_ASN1_SET_OF(ASN1_INTEGER)
 
-struct asn1_type_st {
-  int type;
-  union {
-    char *ptr;
-    ASN1_BOOLEAN boolean;
-    ASN1_STRING *asn1_string;
-    ASN1_OBJECT *object;
-    ASN1_INTEGER *integer;
-    ASN1_ENUMERATED *enumerated;
-    ASN1_BIT_STRING *bit_string;
-    ASN1_OCTET_STRING *octet_string;
-    ASN1_PRINTABLESTRING *printablestring;
-    ASN1_T61STRING *t61string;
-    ASN1_IA5STRING *ia5string;
-    ASN1_GENERALSTRING *generalstring;
-    ASN1_BMPSTRING *bmpstring;
-    ASN1_UNIVERSALSTRING *universalstring;
-    ASN1_UTCTIME *utctime;
-    ASN1_GENERALIZEDTIME *generalizedtime;
-    ASN1_VISIBLESTRING *visiblestring;
-    ASN1_UTF8STRING *utf8string;
-    // set and sequence are left complete and still
-    // contain the set or sequence bytes
-    ASN1_STRING *set;
-    ASN1_STRING *sequence;
-    ASN1_VALUE *asn1_value;
-  } value;
-};
-
 DEFINE_STACK_OF(ASN1_TYPE)
 DECLARE_ASN1_SET_OF(ASN1_TYPE)
 
@@ -706,11 +782,6 @@ typedef struct BIT_STRING_BITNAME_st {
 
 DECLARE_ASN1_FUNCTIONS_fname(ASN1_TYPE, ASN1_ANY, ASN1_TYPE)
 
-OPENSSL_EXPORT int ASN1_TYPE_get(const ASN1_TYPE *a);
-OPENSSL_EXPORT void ASN1_TYPE_set(ASN1_TYPE *a, int type, void *value);
-OPENSSL_EXPORT int ASN1_TYPE_set1(ASN1_TYPE *a, int type, const void *value);
-OPENSSL_EXPORT int ASN1_TYPE_cmp(const ASN1_TYPE *a, const ASN1_TYPE *b);
-
 OPENSSL_EXPORT ASN1_OBJECT *ASN1_OBJECT_new(void);
 OPENSSL_EXPORT void ASN1_OBJECT_free(ASN1_OBJECT *a);
 OPENSSL_EXPORT int i2d_ASN1_OBJECT(const ASN1_OBJECT *a, unsigned char **pp);

include/openssl/x509.h

@@ -905,12 +905,54 @@ OPENSSL_EXPORT X509_CRL *X509_CRL_dup(X509_CRL *crl);
 OPENSSL_EXPORT X509_REVOKED *X509_REVOKED_dup(X509_REVOKED *rev);
 OPENSSL_EXPORT X509_REQ *X509_REQ_dup(X509_REQ *req);
 OPENSSL_EXPORT X509_ALGOR *X509_ALGOR_dup(X509_ALGOR *xn);
-OPENSSL_EXPORT int X509_ALGOR_set0(X509_ALGOR *alg, ASN1_OBJECT *aobj,
-                                   int ptype, void *pval);
-OPENSSL_EXPORT void X509_ALGOR_get0(const ASN1_OBJECT **paobj, int *pptype,
-                                    const void **ppval,
-                                    const X509_ALGOR *algor);
+
+// X509_ALGOR_set0 sets |alg| to an AlgorithmIdentifier with algorithm |obj| and
+// parameter determined by |param_type| and |param_value|. It returns one on
+// success and zero on error. This function takes ownership of |obj| and
+// |param_value| on success.
+//
+// If |param_type| is |V_ASN1_UNDEF|, the parameter is omitted. If |param_type|
+// is zero, the parameter is left unchanged. Otherwise, |param_type| and
+// |param_value| are interpreted as in |ASN1_TYPE_set|.
+//
+// Note omitting the parameter (|V_ASN1_UNDEF|) and encoding an explicit NULL
+// value (|V_ASN1_NULL|) are different. Some algorithms require one and some the
+// other. Consult the relevant specification before calling this function. The
+// correct parameter for an RSASSA-PKCS1-v1_5 signature is |V_ASN1_NULL|. The
+// correct one for an ECDSA or Ed25519 signature is |V_ASN1_UNDEF|.
+OPENSSL_EXPORT int X509_ALGOR_set0(X509_ALGOR *alg, ASN1_OBJECT *obj,
+                                   int param_type, void *param_value);
+
+// X509_ALGOR_get0 sets |*out_obj| to the |alg|'s algorithm. If |alg|'s
+// parameter is omitted, it sets |*out_param_type| and |*out_param_value| to
+// |V_ASN1_UNDEF| and NULL. Otherwise, it sets |*out_param_type| and
+// |*out_param_value| to the parameter, using the same representation as
+// |ASN1_TYPE_set0|. See |ASN1_TYPE_set0| and |ASN1_TYPE| for details.
+//
+// Callers that require the parameter in serialized form should, after checking
+// for |V_ASN1_UNDEF|, use |ASN1_TYPE_set1| and |d2i_ASN1_TYPE|, rather than
+// inspecting |*out_param_value|.
+//
+// Each of |out_obj|, |out_param_type|, and |out_param_value| may be NULL to
+// ignore the output. If |out_param_type| is NULL, |out_param_value| is ignored.
+//
+// WARNING: If |*out_param_type| is set to |V_ASN1_UNDEF|, OpenSSL and older
+// revisions of BoringSSL leave |*out_param_value| unset rather than setting it
+// to NULL. Callers that support both OpenSSL and BoringSSL should not assume
+// |*out_param_value| is uniformly initialized.
+OPENSSL_EXPORT void X509_ALGOR_get0(const ASN1_OBJECT **out_obj,
+                                    int *out_param_type,
+                                    const void **out_param_value,
+                                    const X509_ALGOR *alg);
+
+// X509_ALGOR_set_md sets |alg| to the hash function |md|. Note this
+// AlgorithmIdentifier represents the hash function itself, not a signature
+// algorithm that uses |md|.
 OPENSSL_EXPORT void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md);
+
+// X509_ALGOR_cmp returns zero if |a| and |b| are equal, and some non-zero value
+// otherwise. Note this function can only be used for equality checks, not an
+// ordering.
 OPENSSL_EXPORT int X509_ALGOR_cmp(const X509_ALGOR *a, const X509_ALGOR *b);
 
 OPENSSL_EXPORT X509_NAME *X509_NAME_dup(X509_NAME *xn);
@@ -1538,12 +1580,27 @@ OPENSSL_EXPORT int PKCS8_pkey_get0(ASN1_OBJECT **ppkalg,
                                    const unsigned char **pk, int *ppklen,
                                    X509_ALGOR **pa, PKCS8_PRIV_KEY_INFO *p8);
 
-OPENSSL_EXPORT int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *aobj,
-                                          int ptype, void *pval,
-                                          unsigned char *penc, int penclen);
-OPENSSL_EXPORT int X509_PUBKEY_get0_param(ASN1_OBJECT **ppkalg,
-                                          const unsigned char **pk, int *ppklen,
-                                          X509_ALGOR **pa, X509_PUBKEY *pub);
+// X509_PUBKEY_set0_param sets |pub| to a key with AlgorithmIdentifier
+// determined by |obj|, |param_type|, and |param_value|, and an encoded
+// public key of |key|. On success, it takes ownership of all its parameters and
+// returns one. Otherwise, it returns zero. |key| must have been allocated by
+// |OPENSSL_malloc|.
+//
+// |obj|, |param_type|, and |param_value| are interpreted as in
+// |X509_ALGOR_set0|. See |X509_ALGOR_set0| for details.
+OPENSSL_EXPORT int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *obj,
+                                          int param_type, void *param_value,
+                                          uint8_t *key, int key_len);
+
+// X509_PUBKEY_get0_param outputs fields of |pub| and returns one. If |out_obj|
+// is not NULL, it sets |*out_obj| to AlgorithmIdentifier's OID. If |out_key|
+// is not NULL, it sets |*out_key| and |*out_key_len| to the encoded public key.
+// If |out_alg| is not NULL, it sets |*out_alg| to the AlgorithmIdentifier.
+OPENSSL_EXPORT int X509_PUBKEY_get0_param(ASN1_OBJECT **out_obj,
+                                          const uint8_t **out_key,
+                                          int *out_key_len,
+                                          X509_ALGOR **out_alg,
+                                          X509_PUBKEY *pub);
 
 OPENSSL_EXPORT int X509_check_trust(X509 *x, int id, int flags);
 OPENSSL_EXPORT int X509_TRUST_get_count(void);