Can't decrypt some valid EnvelopedData values that use ECDH keys

[gnarea]

Whilst testing the interoperability of #333, I discovered that PKI.js fails to decrypt EnvelopedData values generated by other tools (e.g., BouncyCastle) when using ECDH keys. The other tools, however, can decrypt values produced by PKI.js.

For example, if I try to decrypt this EnvelopedData with this private key and this certificate, all produced with Bouncy Castle, I'd get the following error:

Error: Trying to add data in unsupported state

    at Decipheriv.update (internal/crypto/cipher.js:159:29)
    at Function.decryptAesKW (/home/gus/repos/relaynet-core-js/node_modules/@peculiar/webcrypto/build/webcrypto.js:269:28)
    at Function.decrypt (/home/gus/repos/relaynet-core-js/node_modules/@peculiar/webcrypto/build/webcrypto.js:203:29)
    at AesKwProvider.onDecrypt (/home/gus/repos/relaynet-core-js/node_modules/@peculiar/webcrypto/build/webcrypto.js:511:26)
    at AesKwProvider.decrypt (/home/gus/repos/relaynet-core-js/node_modules/webcrypto-core/build/webcrypto-core.js:176:31)
    at SubtleCrypto.unwrapKey (/home/gus/repos/relaynet-core-js/node_modules/webcrypto-core/build/webcrypto-core.js:892:38)
    at CryptoEngine.unwrapKey (/home/gus/repos/relaynet-core-js/node_modules/pkijs/src/CryptoEngine.js:889:22)
    at /home/gus/repos/relaynet-core-js/node_modules/pkijs/src/EnvelopedData.js:1463:19
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at Object.<anonymous> (/home/gus/repos/relaynet-core-js/src/integration_tests/pkijs_bug.test.ts:58:21)

The only reference to this error I could find was from @microshine in an unrelated issue, which I think could mean that there's either an integrity issue caused by the BouncyCastle code or there's indeed a bug in PKI.js -- assuming the errors have the same root cause. (I'm using Node.js 12.18 and @peculiar/webcrypto 1.1.7.)

However, OpenSSL has no issue decrypting the EnvelopedData value above (the expected plaintext is hello):

#!/bin/bash
set -o nounset
set -o errexit
set -o pipefail

cat >/tmp/enveloped-data.pem <<EOF
-----BEGIN CMS-----
MIAGCSqGSIb3DQEHA6CAMIACAQIxggFIoYIBRAIBA6BboVkwEwYHKoZIzj0CAQYIKoZIzj0DAQcD
QgAEqtFmeyRh41GzvKyOQZX6sk4BRQJrMM1gFM9u8tCNJNs/Uj9vTlUq3/CiT8ry5DSN9KoqHUqe
I+L0YKhO/veEuKEKBAgxpb1aH9rIwTAVBgYrgQQBCwEwCwYJYIZIAWUDBAEFMIG+MIG7MIGeMIGQ
MYGNMIGKBgNVBAMegYIAMAAzADcAYgA5ADUAOQA0ADIAZQA4ADUAZgBkADIAZgA2AGMAZAAyAGEA
NQBlADAAYQAyADgAYgA0ADcAZAA2ADEAOQA0ADAAMQAxADAAMABhAGEAZABhADkAZQAwAGYAMQA4
ADUAMAAxADgAMwBkAGUAOQAyAGYAMABkADMANwBkAgkAwyKTy3J1TCEEGAja6pyt/S3+ZNW5kLKK
kMUow8oBmWmMGjCABgkqhkiG9w0BBwEwHQYJYIZIAWUDBAECBBAgGb8TEZEicTYqXkaN4SyIoIAE
EDrHeNwTi033+5ORMLynTMoAAAAAAAAAAAAA
-----END CMS-----
EOF

cat >/tmp/recipient-private-key.pem <<EOF
-----BEGIN EC PRIVATE KEY-----
MIGTAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBHkwdwIBAQQgg9QQJRaSNFXvXjGcYotGLyt6n2ba
UTRT663+nVPbtRWgCgYIKoZIzj0DAQehRANCAATCHKPIeew90IreJ2eCCogQgb0QK4FPv2DFsAta
EujG2c3MgRClDKj5R/GUGt3UqyKNK94sJC+gVaxtCsXcw2aB
-----END EC PRIVATE KEY-----
EOF

echo "Got the following plaintext:"
openssl cms -decrypt \
  -in /tmp/enveloped-data.pem \
  -inform PEM \
  -inkey /tmp/recipient-private-key.pem \
  -out -
echo

Repro script

You can reproduce the issue above with this Jest test:

import { fromBER, LocalBaseBlock } from 'asn1js';
import { Certificate, ContentInfo, EnvelopedData } from 'pkijs';

const envelopedDataPem = `-----BEGIN CMS-----
MIAGCSqGSIb3DQEHA6CAMIACAQIxggFIoYIBRAIBA6BboVkwEwYHKoZIzj0CAQYIKoZIzj0DAQcD
QgAEqtFmeyRh41GzvKyOQZX6sk4BRQJrMM1gFM9u8tCNJNs/Uj9vTlUq3/CiT8ry5DSN9KoqHUqe
I+L0YKhO/veEuKEKBAgxpb1aH9rIwTAVBgYrgQQBCwEwCwYJYIZIAWUDBAEFMIG+MIG7MIGeMIGQ
MYGNMIGKBgNVBAMegYIAMAAzADcAYgA5ADUAOQA0ADIAZQA4ADUAZgBkADIAZgA2AGMAZAAyAGEA
NQBlADAAYQAyADgAYgA0ADcAZAA2ADEAOQA0ADAAMQAxADAAMABhAGEAZABhADkAZQAwAGYAMQA4
ADUAMAAxADgAMwBkAGUAOQAyAGYAMABkADMANwBkAgkAwyKTy3J1TCEEGAja6pyt/S3+ZNW5kLKK
kMUow8oBmWmMGjCABgkqhkiG9w0BBwEwHQYJYIZIAWUDBAECBBAgGb8TEZEicTYqXkaN4SyIoIAE
EDrHeNwTi033+5ORMLynTMoAAAAAAAAAAAAA
-----END CMS-----
`;

const recipientPrivateKeyPem = `-----BEGIN EC PRIVATE KEY-----
MIGTAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBHkwdwIBAQQgg9QQJRaSNFXvXjGcYotGLyt6n2ba
UTRT663+nVPbtRWgCgYIKoZIzj0DAQehRANCAATCHKPIeew90IreJ2eCCogQgb0QK4FPv2DFsAta
EujG2c3MgRClDKj5R/GUGt3UqyKNK94sJC+gVaxtCsXcw2aB
-----END EC PRIVATE KEY-----
`;
const recipientCertificatePem = `-----BEGIN CERTIFICATE-----
MIIDsDCCAmSgAwIBAgIJAMMik8tydUwhMEEGCSqGSIb3DQEBCjA0oA8wDQYJYIZI
AWUDBAIBBQChHDAaBgkqhkiG9w0BAQgwDQYJYIZIAWUDBAIBBQCiAwIBIDCBkDGB
jTCBigYDVQQDHoGCADAAMwA3AGIAOQA1ADkANAAyAGUAOAA1AGYAZAAyAGYANgBj
AGQAMgBhADUAZQAwAGEAMgA4AGIANAA3AGQANgAxADkANAAwADEAMQAwADAAYQBh
AGQAYQA5AGUAMABmADEAOAA1ADAAMQA4ADMAZABlADkAMgBmADAAZAAzADcAZDAe
Fw0yMTEwMDcxNjI3MjhaFw0yMTEwMDgxNjI3MjhaMIGQMYGNMIGKBgNVBAMegYIA
MAAzADcAYgA5ADUAOQA0ADIAZQA4ADUAZgBkADIAZgA2AGMAZAAyAGEANQBlADAA
YQAyADgAYgA0ADcAZAA2ADEAOQA0ADAAMQAxADAAMABhAGEAZABhADkAZQAwAGYA
MQA4ADUAMAAxADgAMwBkAGUAOQAyAGYAMABkADMANwBkMFkwEwYHKoZIzj0CAQYI
KoZIzj0DAQcDQgAEwhyjyHnsPdCK3idnggqIEIG9ECuBT79gxbALWhLoxtnNzIEQ
pQyo+UfxlBrd1KsijSveLCQvoFWsbQrF3MNmgaNuMGwwEgYDVR0TAQH/BAgwBgEB
AAIBADApBgNVHQ4EIgQg3QlYJ1gsut/lRVNkWzeNGdlU3Q09Dy0SIvFxvz5KGh8w
KwYDVR0jBCQwIoAgN7lZQuhf0vbNKl4KKLR9YZQBEAqtqeDxhQGD3pLw030wQQYJ
KoZIhvcNAQEKMDSgDzANBglghkgBZQMEAgEFAKEcMBoGCSqGSIb3DQEBCDANBglg
hkgBZQMEAgEFAKIDAgEgA4IBAQCqY/WLk5C1O80ItllTfIHc1k1N++SkbuOpf6Rq
EJrUqHmtgCWYNkxa98qu7fYISBSyEMkDYyzfYnNFeBFBSrjMxGXVuNOU4x/XrUAK
56wkhb5pEUTV2NX0bCDewmBaPH91M7QgnNX2s/Gndt8VzBXqqF9O0lu6MQHPkvH/
lFrVNWaL8wFEaIR/7AgTcv28JsaYOcxgoAt7IWCcUzZALcKkAa53ZoSc+ImXxImv
YazX+1PAxPVoPQGnqEdYSuHHMeo6JEzaEmPB7HNjtYs0lR2iOiUuGShSfQ7xDHNd
RGFgMr8Hw/hQ3czGRimFfdm0W8yIfRtH7tfSZBlkTkVgR9P7
-----END CERTIFICATE-----
`;

const expectedPlaintext = 'hello';

test('PKIjs issue', async () => {
  const contentInfoAsn1 = parsePEM(envelopedDataPem);
  const contentInfo = new ContentInfo({ schema: contentInfoAsn1 });
  const envelopedData = new EnvelopedData({ schema: contentInfo.content });

  const recipientCertificateAsn1 = parsePEM(recipientCertificatePem);
  const recipientCertificate = new Certificate({ schema: recipientCertificateAsn1 });

  const recipientPrivateKey = pemStringToDerArrayBuffer(recipientPrivateKeyPem);

  const plaintext = await envelopedData.decrypt(0, {
    recipientCertificate,
    recipientPrivateKey,
  });
  expect(Buffer.from(plaintext).toString()).toEqual(expectedPlaintext);
});

function pemStringToDerArrayBuffer(pem: string): ArrayBuffer {
  const derBase64 = pem.replace(/-----(BEGIN|END) [\w ]+-----/g, '').replace(/\r?\n|\r/g, '');
  const der = Buffer.from(derBase64, 'base64');
  return der.buffer.slice(der.byteOffset, der.byteOffset + der.byteLength);
}

function parsePEM(pem: string): LocalBaseBlock {
  const derArrayBuffer = pemStringToDerArrayBuffer(pem);

  const asn1Value = fromBER(derArrayBuffer);
  if (asn1Value.offset === -1) {
    throw new Error('Value is invalid');
  }
  return asn1Value.result;
}

[gnarea]

I've just managed to replicate this issue with this EnvelopedData value generated with the certificate above, using the latest openssl as follows:

echo -n "hello" | openssl cms -encrypt -aes-128-cbc -in - -outform PEM /tmp/recipient-cert.pem

Since I get the error above regardless of whether the EnvelopedData is produced with openssl or Bouncy Castle, I think that confirms it's a bug in PKI.js.

(BTW, I had to patch PKI.js locally to work around #335 so that I could test this)

[gnarea]

I've done some further debugging on this and I've managed to narrow down the issue to the KDF, or the input that EnvelopedData.decrypt() passes to it:

https://github.com/PeculiarVentures/PKI.js/blob/8fd65c4c787f4c8153428e745edde888e637c582/src/EnvelopedData.js#L866-L919

As suggested by the error above, the problem occurs when decrypting/unwrapping the AES key in an EnvelopedData generated with Bouncy Castle or OpenSSL:

• With @peculiar/webcrypto: We get the error above.
• With node-webcrypto-ossl: The unwrapKey() method "works" but it returns an array buffer where each byte is a 00. This causes the payload decryption to fail with Error: 140102613321280:error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt:../deps/openssl/openssl/crypto/evp/evp_enc.c:610:.

I've also triple-checked that BouncyCastle and OpenSSL can decrypt each other's EnvelopedData values, and they can. So I don't think this is a problem with Bouncy Castle or OpenSSL.

I'm going to try to spend more time on this later this week but if anyone can give me a hand with this I'd really appreciate it.

[gnarea]

I finally found the problem! 🎉

https://github.com/PeculiarVentures/PKI.js/blob/699ac6e942c279c12e38d4829a44f3fe20040b0a/src/EnvelopedData.js#L1418-L1423

Bouncy Castle and OpenSSL are compliant with RFC 5753, so they leave algorithmParams absent when encrypting with AES.

The reason why Bouncy Castle and OpenSSL can decrypt EnvelopedData values produced by PKI.js is because they have fallbacks for non-compliant implementations using NULL. Here's what BC does, for example:

• https://github.com/bcgit/bc-java/blob/8cd94e90e8536cd58ee0480edca1f86739859982/pkix/src/main/java/org/bouncycastle/cms/jcajce/JceKeyAgreeRecipient.java#L256-L265
• https://github.com/bcgit/bc-java/blob/8cd94e90e8536cd58ee0480edca1f86739859982/pkix/src/main/java/org/bouncycastle/cms/jcajce/JceKeyAgreeRecipient.java#L316-L333

Would you be open to a PR that implemented an RFC5753-compliant fallback?

[rmhrisk]

Yes, we could match bouncy/OpenSSL in the PR