Skip to content

PKCS12_create and PKCS12_parse use inconsistent ordering #6698

@davidben

Description

@davidben

See the output of this test file (error-handling and memory leaks ignored out of laziness):

#include <stdio.h>

#include <openssl/ec.h>
#include <openssl/evp.h>
#include <openssl/crypto.h>
#include <openssl/pkcs12.h>
#include <openssl/rand.h>
#include <openssl/x509.h>


static EVP_PKEY *MakeKey(void) {
  EC_KEY *ec;
  EVP_PKEY *key;

  ec = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
  EC_KEY_generate_key(ec);

  key = EVP_PKEY_new();
  EVP_PKEY_assign_EC_KEY(key, ec);
  return key;
}

static X509_NAME *MakeName(const char *name) {
  X509_NAME *name_obj = X509_NAME_new();
  X509_NAME_add_entry_by_txt(name_obj, "CN", MBSTRING_ASC,
                             (const unsigned char *)name, -1, -1, 0);
  return name_obj;
}

static X509 *MakeCert(const char *subject, EVP_PKEY *subject_key,
                      const char *issuer, EVP_PKEY *issuer_key) {
  uint32_t serial;
  X509 *x509;

  x509 = X509_new();
  RAND_bytes((unsigned char *)&serial, sizeof(serial));
  ASN1_INTEGER_set_uint64(X509_get_serialNumber(x509), serial);
  X509_gmtime_adj(X509_get_notBefore(x509), 0);
  X509_gmtime_adj(X509_get_notAfter(x509), 60 * 60 * 24);
  X509_set_subject_name(x509, MakeName(subject));
  X509_set_issuer_name(x509, MakeName(issuer));
  X509_set_pubkey(x509, subject_key);
  X509_sign(x509, issuer_key, EVP_sha256());
  return x509;
}

static void PrintCert(X509 *x509) {
  if (x509 == NULL) {
    printf("  (null)\n");
  } else {
    X509_NAME_print_ex_fp(stdout, X509_get_subject_name(x509), 2,
                          XN_FLAG_ONELINE);
    printf("\n");
  }
}

int main(int argc, char **argv) {
  EVP_PKEY *leaf_key, *ca1_key, *ca2_key, *root_key, *parsed_key = NULL;
  X509 *leaf, *ca1, *ca2, *root, *parsed_leaf = NULL;
  STACK_OF(X509) *chain, *parsed_chain = NULL;
  int i;
  PKCS12 *p12;
  FILE *f;

  OPENSSL_init_crypto(0, NULL);

  leaf_key = MakeKey();
  ca1_key = MakeKey();
  ca2_key = MakeKey();
  root_key = MakeKey();

  leaf = MakeCert("Leaf", leaf_key, "CA #1", ca1_key);
  ca1 = MakeCert("CA #1", ca1_key, "CA #2", ca2_key);
  ca2 = MakeCert("CA #2", ca2_key, "Root", root_key);
  root = MakeCert("Root", root_key, "Root", root_key);

  chain = sk_X509_new_null();
  sk_X509_push(chain, ca1);
  sk_X509_push(chain, ca2);
  sk_X509_push(chain, root);

  printf("Calling PKCS12_create with leaf:\n");
  PrintCert(leaf);
  printf("And CAs:\n");
  for (i = 0; i < sk_X509_num(chain); i++) {
    PrintCert(sk_X509_value(chain, i));
  }

  p12 = PKCS12_create(NULL, NULL, leaf_key, leaf, chain, 0, 0, 0, 0, 0);
  if (argc == 2) {
    printf("Writing to %s\n", argv[1]);
    f = fopen(argv[1], "w");
    i2d_PKCS12_fp(f, p12);
    fclose(f);
  }

  PKCS12_parse(p12, NULL, &parsed_key, &parsed_leaf, &parsed_chain);
  printf("PKCS12_parse on the result gives leaf:\n");
  PrintCert(parsed_leaf);
  printf("And CAs:\n");
  for (i = 0; i < sk_X509_num(parsed_chain); i++) {
    PrintCert(sk_X509_value(parsed_chain, i));
  }

  return 0;
}

In OpenSSL, this gives:

Calling PKCS12_create with leaf:
  CN = Leaf
And CAs:
  CN = CA #1
  CN = CA #2
  CN = Root
Writing to foo.pfx
PKCS12_parse on the result gives leaf:
  CN = Leaf
And CAs:
  CN = Root
  CN = CA #2
  CN = CA #1

Examining foo.pfx (which does not use PKCS12_parse) shows that the ordering in the file is the PKCS12_create one. PKCS12_create writes out the leaf, followed by the CAs in the supplied order:

if (cert) {
bag = PKCS12_add_cert(&bags, cert);
if (name && !PKCS12_add_friendlyname(bag, name, -1))
goto err;
if (keyidlen && !PKCS12_add_localkeyid(bag, keyid, keyidlen))
goto err;
}
/* Add all other certificates */
for (i = 0; i < sk_X509_num(ca); i++) {
if (!PKCS12_add_cert(&bags, sk_X509_value(ca, i)))
goto err;
}

Rather, the quirk comes from this logic, which ends up reversing the order of all non-leaf certificates:

while ((x = sk_X509_pop(ocerts))) {
if (pkey && *pkey && cert && !*cert) {
ERR_set_mark();
if (X509_check_private_key(x, *pkey)) {
*cert = x;
x = NULL;
}
ERR_pop_to_mark();
}
if (ca && x) {
if (!*ca)
*ca = sk_X509_new_null();
if (!*ca)
goto err;
if (!sk_X509_push(*ca, x))
goto err;
x = NULL;
}
X509_free(x);
}

It's no doubt to late to change this for 1.1.x (I learned about this from code which was sensitive to this), but it should at least be documented.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions