Opaquify PKCS8_PRIV_KEY_INFO.

This is partially imported from upstream's
54dbf42398e23349b59f258a3dd60387bbc5ba13 which does something similar.

In doing so, remove the pkcs8->broken field, which is a remnant of some
parsing hacks we long since removed (PKCS8_set_broken). The immediate
motivation is, if this sticks, this would make it easier to detach
i2d_PKCS8_PRIV_KEY_INFO and d2i_PKCS8_PRIV_KEY_INFO from the old ASN.1
code.

Update-Note: Direct accesses of PKCS8_PRIV_KEY_INFO now need to use the
accessors. Code search suggests no one uses the fields. Even the
accessors are virtually unused (the one thing which uses it doesn't need
it).

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

crypto/pkcs8/internal.h

@@ -63,6 +63,13 @@ extern "C" {
 #endif
 
 
+struct pkcs8_priv_key_info_st {
+  ASN1_INTEGER *version;
+  X509_ALGOR *pkeyalg;
+  ASN1_OCTET_STRING *pkey;
+  STACK_OF(X509_ATTRIBUTE) *attributes;
+};
+
 // pkcs8_pbe_decrypt decrypts |in| using the PBE scheme described by
 // |algorithm|, which should be a serialized AlgorithmIdentifier structure. On
 // success, it sets |*out| to a newly-allocated buffer containing the decrypted

crypto/pkcs8/pkcs8_x509.c

@@ -96,10 +96,8 @@ static int pkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
   // Since the structure must still be valid use ASN1_OP_FREE_PRE
   if (operation == ASN1_OP_FREE_PRE) {
     PKCS8_PRIV_KEY_INFO *key = (PKCS8_PRIV_KEY_INFO *)*pval;
-    if (key->pkey && key->pkey->type == V_ASN1_OCTET_STRING &&
+    if (key->pkey) {
-        key->pkey->value.octet_string) {
+      OPENSSL_cleanse(key->pkey->data, key->pkey->length);
-      OPENSSL_cleanse(key->pkey->value.octet_string->data,
-                      key->pkey->value.octet_string->length);
     }
   }
   return 1;
@@ -108,12 +106,45 @@ static int pkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
 ASN1_SEQUENCE_cb(PKCS8_PRIV_KEY_INFO, pkey_cb) = {
   ASN1_SIMPLE(PKCS8_PRIV_KEY_INFO, version, ASN1_INTEGER),
   ASN1_SIMPLE(PKCS8_PRIV_KEY_INFO, pkeyalg, X509_ALGOR),
-  ASN1_SIMPLE(PKCS8_PRIV_KEY_INFO, pkey, ASN1_ANY),
+  ASN1_SIMPLE(PKCS8_PRIV_KEY_INFO, pkey, ASN1_OCTET_STRING),
   ASN1_IMP_SET_OF_OPT(PKCS8_PRIV_KEY_INFO, attributes, X509_ATTRIBUTE, 0)
 } ASN1_SEQUENCE_END_cb(PKCS8_PRIV_KEY_INFO, PKCS8_PRIV_KEY_INFO)
 
 IMPLEMENT_ASN1_FUNCTIONS(PKCS8_PRIV_KEY_INFO)
 
+int PKCS8_pkey_set0(PKCS8_PRIV_KEY_INFO *priv, ASN1_OBJECT *aobj, int version,
+                    int ptype, void *pval, uint8_t *penc, int penclen) {
+  if (version >= 0 &&
+      !ASN1_INTEGER_set(priv->version, version)) {
+    return 0;
+  }
+
+  if (!X509_ALGOR_set0(priv->pkeyalg, aobj, ptype, pval)) {
+    return 0;
+  }
+
+  if (penc != NULL) {
+    ASN1_STRING_set0(priv->pkey, penc, penclen);
+  }
+
+  return 1;
+}
+
+int PKCS8_pkey_get0(ASN1_OBJECT **ppkalg, const uint8_t **pk, int *ppklen,
+                    X509_ALGOR **pa, PKCS8_PRIV_KEY_INFO *p8) {
+  if (ppkalg) {
+    *ppkalg = p8->pkeyalg->algorithm;
+  }
+  if (pk) {
+    *pk = ASN1_STRING_data(p8->pkey);
+    *ppklen = ASN1_STRING_length(p8->pkey);
+  }
+  if (pa) {
+    *pa = p8->pkeyalg;
+  }
+  return 1;
+}
+
 EVP_PKEY *EVP_PKCS82PKEY(PKCS8_PRIV_KEY_INFO *p8) {
   uint8_t *der = NULL;
   int der_len = i2d_PKCS8_PRIV_KEY_INFO(p8, &der);

crypto/x509/x509.c

@@ -65,73 +65,6 @@
  * it to avoid downstream churn. */
 OPENSSL_DECLARE_ERROR_REASON(X509, UNSUPPORTED_ALGORITHM)
 
-int PKCS8_pkey_set0(PKCS8_PRIV_KEY_INFO *priv, ASN1_OBJECT *aobj, int version,
-                    int ptype, void *pval, uint8_t *penc, int penclen) {
-  uint8_t **ppenc = NULL;
-  if (version >= 0) {
-    if (!ASN1_INTEGER_set(priv->version, version)) {
-      return 0;
-    }
-  }
-
-  if (penc) {
-    int pmtype;
-    ASN1_OCTET_STRING *oct;
-
-    oct = ASN1_OCTET_STRING_new();
-    if (!oct) {
-      return 0;
-    }
-    oct->data = penc;
-    ppenc = &oct->data;
-    oct->length = penclen;
-    if (priv->broken == PKCS8_NO_OCTET) {
-      pmtype = V_ASN1_SEQUENCE;
-    } else {
-      pmtype = V_ASN1_OCTET_STRING;
-    }
-    ASN1_TYPE_set(priv->pkey, pmtype, oct);
-  }
-
-  if (!X509_ALGOR_set0(priv->pkeyalg, aobj, ptype, pval)) {
-    /* If call fails do not swallow 'enc' */
-    if (ppenc) {
-      *ppenc = NULL;
-    }
-    return 0;
-  }
-
-  return 1;
-}
-
-int PKCS8_pkey_get0(ASN1_OBJECT **ppkalg, const uint8_t **pk, int *ppklen,
-                    X509_ALGOR **pa, PKCS8_PRIV_KEY_INFO *p8) {
-  if (ppkalg) {
-    *ppkalg = p8->pkeyalg->algorithm;
-  }
-
-  if (p8->pkey->type == V_ASN1_OCTET_STRING) {
-    p8->broken = PKCS8_OK;
-    if (pk) {
-      *pk = p8->pkey->value.octet_string->data;
-      *ppklen = p8->pkey->value.octet_string->length;
-    }
-  } else if (p8->pkey->type == V_ASN1_SEQUENCE) {
-    p8->broken = PKCS8_NO_OCTET;
-    if (pk) {
-      *pk = p8->pkey->value.sequence->data;
-      *ppklen = p8->pkey->value.sequence->length;
-    }
-  } else {
-    return 0;
-  }
-
-  if (pa) {
-    *pa = p8->pkeyalg;
-  }
-  return 1;
-}
-
 int X509_signature_dump(BIO *bp, const ASN1_STRING *sig, int indent) {
   const uint8_t *s;
   int i, n;

include/openssl/x509.h

@@ -460,21 +460,6 @@ struct Netscape_spki_st {
   ASN1_BIT_STRING *signature;
 } /* NETSCAPE_SPKI */;
 
-// PKCS#8 private key info structure
-
-struct pkcs8_priv_key_info_st {
-  int broken;  // Flag for various broken formats
-#define PKCS8_OK 0
-#define PKCS8_NO_OCTET 1
-#define PKCS8_EMBEDDED_PARAM 2
-#define PKCS8_NS_DB 3
-#define PKCS8_NEG_PRIVKEY 4
-  ASN1_INTEGER *version;
-  X509_ALGOR *pkeyalg;
-  ASN1_TYPE *pkey;  // Should be OCTET STRING but some are broken
-  STACK_OF(X509_ATTRIBUTE) * attributes;
-};
-
 #ifdef __cplusplus
 }
 #endif