Invalid PKCS#12. macData field present but MAC was not validated.

[dmitrc]

As of version 1.3.2, trying to get PKCS12 now throws this for valid untampered certificates that used to work before:

Error: Invalid PKCS#12. macData field present but MAC was not validated.
at p12.pkcs12FromAsn1 (/home/dmitrii/src/forge-repro/node_modules/node-forge/lib/pkcs12.js:479:11)
at getP12 (file:///home/dmitrii/src/forge-repro/index.js:30:28)
at parseCertificate (file:///home/dmitrii/src/forge-repro/index.js:9:15)
at file:///home/dmitrii/src/forge-repro/index.js:6:13

Find attached example.pfx (.debug added to satisfy GitHub filetype filter), generated in my personal Azure Key Vault (self-signed), with no password set.

I am using the following code to attempt to get PKCS12 object out of it:

import forge from "node-forge";
import { readFile } from "fs/promises";

//  normally we'd use @azure/keyvault-secrets to get PFX as Base64 directly from AKV
const pfx = await readFile("./example.pfx");
const base64Pfx = pfx.toString("base64");

const p12Der = forge.util.decode64(base64Pfx);
const p12Asn1 = forge.asn1.fromDer(p12Der);
const p12 = forge.pkcs12.pkcs12FromAsn1(p12Asn1);

In v1.3.1 this works as expected.
In v1.3.2 this throws the error above.

The certificate itself is not tampered with in any way, and works fine with OpenSSL directly, as well as similar libraries in C# and Python.

Is the check added in pkcs12.js to address the recent CVE issue too overzealous, trying to cover for cases it shouldn't be touching?

example.pfx.debug

[wodzen]

I'm investigating this now.

I was able to reproduce the issue with the provided example.pfx.

TLDR; The existing PFX.macData ASN.1 schema treats digestAlgorithm.parameters as required, but in modern specs it’s optional. The example PFX omits the parameters for the MAC digest algorithm, so PFX.macData fails validation and is treated as absent. Before the CVE fix, this likely meant the MAC was never verified.

This error likely occurs because the patched lib/pkcs12 check sees a 3-element PFX (version, authSafe, macData) with no captured MAC and throws, even though the ASN.1 is perfectly valid and the MAC is verifiable.

+  } else if (Array.isArray(obj.value) && obj.value.length > 2) {
+    /* This is pfx data that should ahve mac and verify macDigest */
+    throw new Error('Invalid PKCS#12: macData field present but MAC was not validated');

The following fix on lib/pkcs12.js:171 should correct the error you're seeing, though the issue may be a bit more complex. This makes digestAlgorithm.parameters optional per today's specs. Additional tests should be added to cover these cases. I can do a bit more digging and work on this working PR tomorrow.
#1128

...
        }, {
          name: 'PFX.macData.mac.digestAlgorithm.parameters',
+        optional: true,
          tagClass: asn1.Class.UNIVERSAL,
          captureAsn1: 'macAlgorithmParameters'
        }]
...

In the days of SHA1 when this repository was under active development, parameters was a required field. Today, it is optional. Looking to the future, there's new specs that require it again. Ultimately this depends on the target implementation.

I used the following to troubleshoot the PFX ASN.1

openssl asn1parse -in example.pfx.debug -inform der -i  

We can see that the provided PFX is using an encoding without parameters.

0:d=0  cons: SEQUENCE                <- PFX
 4:d=1   prim: INTEGER :03           <- version = 3
 7:d=1   cons: SEQUENCE              <- authSafe (ContentInfo)
 ...
2599:d=1 hl=2 l=59 cons: SEQUENCE    <- macData
2601:d=2 hl=2 l=31 cons:  SEQUENCE   <- mac (DigestInfo)
2603:d=3 hl=2 l= 7 cons:   SEQUENCE  <- digestAlgorithm (AlgorithmIdentifier)
2605:d=4 hl=2 l= 5 prim:    OBJECT   :sha1
2612:d=3 hl=2 l=20 prim:   OCTET STRING  <- digest
2634:d=2 hl=2 l=20 prim:  OCTET STRING   <- macSalt
2656:d=2 hl=2 l= 2 prim:  INTEGER :07D0  <- iterations

PFX macData is optional at the top level, however the digest algorithm parameters is limited and is not marked optional.

        name: 'PFX.macData.mac.digestAlgorithm',
        tagClass: asn1.Class.UNIVERSAL,
        type: asn1.Type.SEQUENCE,
        constructed: true,
        value: [{
          name: 'PFX.macData.mac.digestAlgorithm.algorithm',
          tagClass: asn1.Class.UNIVERSAL,
          type: asn1.Type.OID,
          constructed: false,
          capture: 'macAlgorithm'
        }, {
          name: 'PFX.macData.mac.digestAlgorithm.parameters',
          tagClass: asn1.Class.UNIVERSAL,
          captureAsn1: 'macAlgorithmParameters'
        }]

With the old loop, because macData is marked optional at the PFX level, the whole macData subtree could fail and still allow the outer PFX to be treated as valid.

if(v.value && forge.util.isArray(v.value)) {
  var j = 0;
  for(var i = 0; rval && i < v.value.length; ++i) {
    rval = v.value[i].optional || false;
    if(obj.value[j]) {
      rval = asn1.validate(obj.value[j], v.value[i], capture, errors);
      if(rval) {
        ++j;
      } else if(v.value[i].optional) {
        rval = true;
      }
    }
    if(!rval && errors) {
      errors.push('[' + v.name + '] ' +
        'Tag class "' + v.tagClass + '", type "' +
        v.type + '" expected value length "' +
        v.value.length + '", got "' +
        obj.value.length + '"');
    }
  }
}

The new loop, if a child schema is marked optional and doesn’t match, the validator skips it and does not consume the current objChild. But PFX.macData itself is optional, so if anything inside macData fails, asn1.validate can return false for that schema node, while the parent will accept that as optional field absent.

...
if (v.value && forge.util.isArray(v.value)) {
  var j = 0;
  for (var i = 0; rval && i < v.value.length; ++i) {
    var schemaItem = v.value[i];
    rval = schemaItem.optional || false;

    var objChild = obj.value[j];

    if (!objChild) {
      if (!schemaItem.optional) {
        rval = false;
        ...
      }
      continue;
    }

    var schemaHasTag = (typeof schemaItem.tagClass !== 'undefined' &&
                        typeof schemaItem.type !== 'undefined');

    if (schemaHasTag &&
        (objChild.tagClass !== schemaItem.tagClass ||
         objChild.type !== schemaItem.type)) {
      if (schemaItem.optional) {
        rval = true;
        continue;        // skip optional, don't ++j
      } else {
        rval = false;
        ...
        break;
      }
    }

    var childRval = asn1.validate(objChild, schemaItem, capture, errors);
    if (childRval) {
      ++j;              // consume child only on success
      rval = true;
    } else if (schemaItem.optional) {
      rval = true;      // treat failure as "optional absent"
      // note: no ++j here either
    } else {
      rval = false;
      break;
    }
  }
}
...

[sei-vsarvepalli]

The example certificate from @dmitrc is empty password or no password, is that right? May be a test case for such a thing is also useful to have.

[davidlehn]

Thanks for the test case. Is it ok to include the pfx in the test suite? That would at least help avoid regressions on that particular case. A handful of other variations in the test suite would be good but I'm not sure how to construct them.

[davidlehn]

@dmitrc Thanks for the report.
@wodzen Thanks for the analysis and fix! Released in 1.3.3. If you're motivated to add related test cases, I opened an issue for that. Not sure when I'll get to it. Other than if it's ok to use the one here, that's not too difficult to encode and embed in the test suite.

[dmitrc]

Thanks for the test case. Is it ok to include the pfx in the test suite? That would at least help avoid regressions on that particular case. A handful of other variations in the test suite would be good but I'm not sure how to construct them.

Yeah, of course. I've generated this PFX just for the purpose of this repro, so you are welcome to use it in any way you see fit.

Appreciate the quick turnaround, folks! ❤️