Add ASN1 decoding of encrypted data using PBES2 algorithm #889

fmarco76 · 2022-08-05T15:57:34Z

The validation of pk12 file with certificate in :pkcs7-encryptedData fails because the ASN1 decode work only for PBES1.

See https://bugzilla.redhat.com/show_bug.cgi?id=2115765.

With this PR the decode does not work but there are problem with the key generation but can be managed in a separate PR/task.

edewata · 2022-08-08T15:37:12Z

I will need to defer to @jmagne on this one.

edewata · 2022-08-08T15:41:45Z

SonarCloud complains about duplicated code blocks. Is there a way to make it less sensitive? If not we probably should just disable it because the complaints don't really make sense to me. @ckelleyRH What do you think?

ckelleyRH · 2022-08-08T15:50:56Z

SonarCloud complains about duplicated code blocks. Is there a way to make it less sensitive? If not we probably should just disable it because the complaints don't really make sense to me. @ckelleyRH What do you think?

It is complaining that there are multiple identical methods, e.g.:

private static final Tag TAG = SEQUENCE.TAG;
@Override
public Tag getTag() {
    return TAG;
 }
  
@Override
public void encode(OutputStream ostream) throws IOException {
    sequence.encode(ostream);
}

...is duplicated across many classes - implying that a common base class to implement this method may be useful. Or a default method in the interface. Anyway, I think this is actually useful as it is telling us that we can reduce boilerplate code by tweaking the classes/interfaces.

edewata · 2022-08-08T17:12:12Z

Sometimes the classes are not inheriting from the same base class, and sometimes the code depends on class attributes (i.e. not static), so it could be difficult to consolidate the duplicate code without a major redesign. This could be a major barrier if we require that to be fixed before merging the PR.

ckelleyRH · 2022-08-08T17:19:06Z

Difficult, and potentially dangerous, as something could go wrong easily in the refactor.

Currently the duplication metric doesn't affect the gate, so it is an advisory. I guess it has had the desired effect, we have discussed it, but there is no need to fix it for a PR as it won't make the CI fail.

edewata · 2022-08-08T17:26:15Z

So code smells are just advisory? I thought I saw SonarCloud did not pass due to code smells (maybe there were too many).

ckelleyRH · 2022-08-09T05:48:12Z

So code smells are just advisory? I thought I saw SonarCloud did not pass due to code smells (maybe there were too many).

Supposedly, they could make the "maintainability" gate fail. I conclude this metric must be a simple weight of numbers, as there are supposed "blocker" and "critical" severity issues listed there and we have an A rating. This contrasts to the "reliability" metric, which does seem to be tied to the severity of the issues more.

sonarqubecloud · 2022-08-23T17:05:51Z

SonarCloud Quality Gate failed.

2 Bugs
0 Vulnerabilities
2 Security Hotspots
26 Code Smells

0.0% Coverage
14.4% Duplication

fmarco76 · 2022-10-17T09:32:58Z

This PR incluide the #888. They fix two different problem so we keep them separated but both need to be merged in order to manage p12 files with SHA256 with jss

jmagne

This is just to show I got started looking at this. Will continue.. thx.

base/src/main/java/org/mozilla/jss/crypto/PBEKeyGenParams.java

base/src/main/java/org/mozilla/jss/pkcs12/MacData.java

jmagne

Just added a few more comments / questions. I guess we need to decide if for this round 256 is good enough or we try to get 384 and 512 working. Next round I should approve once we've decided.

base/src/main/java/org/mozilla/jss/pkcs7/EncryptedContentInfo.java

base/src/main/java/org/mozilla/jss/pkix/primitive/PBES2Params.java

jmagne · 2022-11-04T23:56:11Z

native/src/main/native/org/mozilla/jss/pkcs11/PK11KeyGenerator.c

-        if( skey == NULL ) {
-            JSS_throwMsg(env, TOKEN_EXCEPTION, "Failed to generate PBE key");
-            goto finish;
+        break;


OK, after looking at the original file, I think I see what this is doing. Previously, the code was just an (if) for SHA1 amd (else) for everything else, which results in using a PBEV2 algid. You have just made this a switch where you've added a case for SEC_OID_SHA256, which takes it's own action.

My question here, just for my own understanding, is what case are we satisfying here? Is this something you ran into when experimenting with various p12 files and found out you needed this? Also should there be 384 and 512 variants. Trying to refresh my memory, I think the case we were trying to debug was ending up in the (default) clause. I'm just trying to get an understanding where the added case comes into play and if you saw this code being hit? Thanks.

The case we have tested are for the two cases:

CKM_PBA_SHA1_WITH_SHA1_HMAC for sha-1

SEC_OID_SHA256 for SHA-256

In the original file the default case should also work with SHA-256 but I have seen thatit was not managed in NSS like that so I leave the two options and add the special case for the SHA-256. I do not know which cases were tested with the default case.
It is a switch because in my idea is easier to add other cases if needed for other algorithms. In particular for SHA-384 and SHA-512 I am not sure if they require new cases or can be grouped with SHA-256, I need some tests.

Finally, I have reverted the code to use the if. However, I have add a new if for SHA-[256, 384, 512]. In case the symmetric key is not generated a last attempt is done with the default case. To note that the skey is null only if the other if are not used because if they are and the key is NULL the execution go the finish label at the end of the method.

Additionally, I have add a new method for the new case to avoid code repetition.

jmagne · 2022-11-04T23:57:39Z

native/src/main/native/org/mozilla/jss/crypto/Algorithm.c

@@ -124,7 +124,7 @@ JSS_AlgInfo JSS_AlgTable[NUM_ALGS] = {
 /* 35 */    {CKM_AES_CBC_PAD, PK11_MECH},
 /* 36 */    {CKM_RC2_CBC_PAD, PK11_MECH},
 /* 37 */    {CKM_RC2_KEY_GEN, PK11_MECH},
-/* 38 */    {SEC_OID_SHA256, SEC_OID_TAG},
+/* 38 */    {SEC_OID_SHA256, PK11_MECH},
 /* 39 */    {SEC_OID_SHA384, SEC_OID_TAG},
 /* 40 */    {SEC_OID_SHA512, SEC_OID_TAG},
 /* 41 */    {SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION, SEC_OID_TAG},


I take it we will need entries here for 384 and 512?

After thinking to this change I have decided to revert this change and apply the conversion just before the key generation. I have thought this table can be used by other methods and the changes here should be investigated. However, since I get the related mech algorithm from NSS and convert to the one I really use just during the generation we should not have problem with other code accessing the table.

fmarco76 · 2022-11-07T09:49:30Z

I guess we need to decide if for this round 256 is good enough or we try to get 384 and 512 working

The original ticket generating this work is #796 where only the case for 256 is mentioned. I started working for that with the idea to verify the other cases (>256) after this. We can try to add SHA-384 and SHA-512 in this PR if we agree since it should be straight forward (to be verified). I will do some tests.

fmarco76 · 2022-11-07T18:29:16Z

I have also integrated SHA-384 and SHA-512 and tested against a .p12 generated by OpenSSL using the macalg option. This is the output:

$ openssl pkcs12 -in test-open-sha384.p12 -noout -info
Enter Import Password:
MAC: sha384, Iteration 2048
MAC length: 48, salt length: 8
PKCS7 Encrypted data: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256
Certificate bag
PKCS7 Data
Shrouded Keybag: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256

$ /usr/lib/jvm/java-17-openjdk-17.0.4.1.1-1.fc36.x86_64/bin/java -cp "/usr/share/pki/lib/*" -Dcom.redhat.fips=false -Djava.util.logging.config.file=/usr/share/pki/etc/logging.properties com.netscape.cmstools.cli.MainCLI pkcs12-cert-find --pkcs12-file test-open-sha384.p12 --pkcs12-password Secret.123
WARNING: The SHA-1 algorithm used in org.mozilla.jss.pkcs12.SafeBag::getLocalKeyIDFromCert:264 is deprecated. Use a more secure algorithm.
---------------
1 entries found
---------------
  Certificate ID: 0xf8361681d6c99ac7b3cc01345bc7b6eb6acd224a
  Serial Number: 0xa0c0761e2dd098b4e030b3f7ecd15c10
  Friendly Name: Certificate Authority
  Subject DN: CN=Certificate Authority
  Issuer DN: CN=Certificate Authority
  Has Key: true
  Key ID: 0xf8361681d6c99ac7b3cc01345bc7b6eb6acd224a

jmagne

OK, I've gotten back to this one. Resolved a few conversations. I still need to get back to this next week and look over some of the final more complex changes such as the pbe key gneration changes. Thx.

jmagne · 2022-11-23T00:52:01Z

base/src/main/java/org/mozilla/jss/crypto/HMACAlgorithm.java

@@ -58,6 +58,14 @@ public static HMACAlgorithm fromOID(OBJECT_IDENTIFIER oid)
        (CKM_SHA_1_HMAC, "SHA-1-HMAC",
             OBJECT_IDENTIFIER.ALGORITHM.subBranch(26), 20);

+    public static final HMACAlgorithm hSHA1 = new HMACAlgorithm


My apologies if this has been discussed or asked about before.... I see that we've created some new types with names such as hSHA1 and sSHA256 to kind of add to the ones that were there before. Question is, what is the purpose of these, since I've searched through the PR and not found any references. Perhaps they are referenced in a companion PR.

These are used in this PR. I need to register these algorithm because they are used to decrypt the PKCS Authenticated Safe. As an example is I remove them I get:

java.security.NoSuchAlgorithmException at org.mozilla.jss.crypto.HMACAlgorithm.fromOID(HMACAlgorithm.java:45) at org.mozilla.jss.pkcs7.EncryptedContentInfo.decrypt(EncryptedContentInfo.java:286) at org.mozilla.jss.pkcs12.AuthenticatedSafes.getSafeContentsAt(AuthenticatedSafes.java:169) ...

This is for a SHA1 based pkcs12 like:

$ openssl pkcs12 -in test-nss-sha1.p12 -info -noout Enter Import Password: MAC: sha1, Iteration 600000 MAC length: 20, salt length: 16 PKCS7 Data Shrouded Keybag: PBES2, PBKDF2, AES-256-CBC, Iteration 600000, PRF hmacWithSHA1 PKCS7 Encrypted data: PBES2, PBKDF2, AES-128-CBC, Iteration 600000, PRF hmacWithSHA1 Certificate bag

If I tried with a pkcs12 generated by openssl with SHA-384 like:

``
$ openssl pkcs12 -in test-open-sha384.p12 -info -noout
Enter Import Password:
MAC: sha384, Iteration 2048
MAC length: 48, salt length: 8
PKCS7 Encrypted data: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256
Certificate bag
PKCS7 Data
Shrouded Keybag: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256

The error is:

org.mozilla.jss.crypto.TokenException: Cipher context finalization failed: (-8190) security library: received bad data.
at org.mozilla.jss.pkcs11.PK11Cipher.finalizeContext(Native Method)
at org.mozilla.jss.pkcs11.PK11Cipher.doFinal(PK11Cipher.java:186)
at org.mozilla.jss.pkcs7.EncryptedContentInfo.decrypt(EncryptedContentInfo.java:325)
at org.mozilla.jss.pkcs12.AuthenticatedSafes.getSafeContentsAt(AuthenticatedSafes.java:169)

In this second case the HMAC is found but it is not correct.

jmagne · 2022-11-23T01:00:58Z

base/src/main/java/org/mozilla/jss/crypto/HMACAlgorithm.java

@@ -58,6 +58,14 @@ public static HMACAlgorithm fromOID(OBJECT_IDENTIFIER oid)
        (CKM_SHA_1_HMAC, "SHA-1-HMAC",
             OBJECT_IDENTIFIER.ALGORITHM.subBranch(26), 20);

+    public static final HMACAlgorithm hSHA1 = new HMACAlgorithm
+            (SEC_OID_HMAC_SHA1, "SHA-1-HMAC",


Apologies again since I didn't notice this the first trip through. Since the PR has been adding some new algorithms such as these PBE HMAC algorithms, I failed to mention that we usually put versions of these JSS algorithms in the cyrpto provider class we support to allow the various algorithms to be accessed through the cyrpto provider interface. The main file that establishes these interfaces is base/src/main/java/org/mozilla/jss/JSSProvider.java. The following is an example I did for the PSS SHA256 signature algorithm within JSSProvider.java:

put("Signature.RSASSA-PSS",
"org.mozilla.jss.provider.java.security.JSSSignatureSpi$RSAPSSSignature");

put("Alg.Alias.Signature.1.2.840.113549.1.1.10", "RSASSA-PSS"); put("Alg.Alias.Signature.OID.1.2.840.113549.1.1.10", "RSASSA-PSS"); put("Signature.SHA-256/RSA/PSS", "org.mozilla.jss.provider.java.security.JSSSignatureSpi$SHA256RSAPSS"); put("Alg.Alias.Signature.SHA256withRSA/PSS","SHA-256/RSA/PSS");

Note that the alias items refer to different names that can be used to call up the various alg with the cyrpto provider interface.

Examples of tests for both styles can be found in: tests/SigTest.java and tests/JSASigTest.java. I mention the test files because this shows examples on how the JCA interface is used and also to state that we usually put some simple tests when we add new algs such as this, and the tests will run during the build time tests that usually take place. If you need further info on this, just ask, and that will be my opportunity to refresh my own memory of this since it's been awhile since I did the PSS feature.

native/src/main/native/org/mozilla/jss/pkcs11/PK11KeyGenerator.c

jmagne

Left a few more comments but it continues to look better and better, thx.

jmagne · 2022-12-02T00:49:48Z

base/src/main/java/org/mozilla/jss/crypto/HMACAlgorithm.java

@@ -56,18 +56,18 @@ public static HMACAlgorithm fromOID(OBJECT_IDENTIFIER oid)
    @Deprecated(since="5.0.1", forRemoval=true)
    public static final HMACAlgorithm SHA1 = new HMACAlgorithm
        (CKM_SHA_1_HMAC, "SHA-1-HMAC",
-             OBJECT_IDENTIFIER.ALGORITHM.subBranch(26), 20);


OK, this looks interesting. In the chat you asked about the SEC_OIDS vs CKMs. My thing about converting to CKMs was only a question at the time, not a concrete suggestion :) After looking at the nss code, it seems like these algs are are declared in seoid.c something like this.:

OD(hmac_sha1, SEC_OID_HMAC_SHA1, "HMAC SHA-1",
CKM_SHA_1_HMAC, INVALID_CERT_EXTENSION)

and

OD(hmac_sha256, SEC_OID_HMAC_SHA256, "HMAC SHA-256",
CKM_SHA256_HMAC, INVALID_CERT_EXTENSION)

This leads me to the conclusion that nss must have a way to convert between the two when someone is trying to use say a SEC_OID. The CKMs are mechanisms that the pkcs#11 modules need to do the work. Therefore I think your idea to go all SEC_OIDs seems quite the way to go. Interesting that the SHA1 version we are still keeping as a CKM. Since the 3.67 nss code I was looking at shows that there is a corresponding SECOID for that one as well, SEC_OID_HMAC_SHA1, I wonder if it would make sense to just try that out to see if all is kosher. Maybe there has been some historical reason why this was done.

I just wanted to get this straight since the hsm's use the pkcs#11 interface. If nss is correctly converting these for us it should be all good there, that is if the hsm actually supports said algs.

SEC_OID_HMAC_SHA1 is not defined I was adding it but reverting the code after the integration with JSSProvider I have removed it. I can try to reintroduce so we will use only SEC_OID and let nss to convert to the correct pk11 mechanism.

his leads me to the conclusion that nss must have a way to convert between the two when someone is trying to use say a SEC_OID

I think this should invoke the conversion: https://github.com/dogtagpki/jss/blob/master/native/src/main/native/org/mozilla/jss/crypto/Algorithm.c#L227-L240

jmagne

OK, I think we are at the point where we have to approve, which I'm doing. If any corner cases have been introduced hopefully and their test suite will uncover. Thanks for the assistance on this one.

Stop id the algorithm is not manged Add custom hash algorithm Add SHA1 HMAC for PBES2 Add sha1 hmac for pbes2 Fix total num of algs Fis SonarCloud code smell about deprecated comment Fix missing checks Add SHA-384 and SHA-512 to MacData and KeyGen Add SHA-384 and SHA-512 support into native key generation Move the meck selection logic to the KeyGenerator Move the key generation to a separate method Add PBE to the list of provided algorithms Revert hash algorithm definition and fix SHA1 Convert SHA_[] HMAC algorithms from CKM_* to SEC_OID_* Convert SHA1 HMAC from CKM_SHA_1_HMAC to SEC_OID_HMAC_SHA1

Key generated but decrypt paramter problem Decrypt the content without generating the digest The method doFinal will generate error for the decrypt operation so only the update is used to parse the encrypted data. Fix password conversion The password for PBES2 EncryptedContentInfo needs the default password converter in the majority of cases. The cusomt password converter as adding extra bytes which were not managed by nss/openssl p12 code. Fix optional keyLength parameter in PBKDF2 Fix formatting

sonarqubecloud · 2022-12-06T09:53:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
14.1% Duplication

fmarco76 · 2022-12-06T10:07:06Z

Last commit was just to squash all commits in just two and run the CI workflow which was broken during the previous commits.
No code changes

fmarco76 · 2022-12-06T10:07:36Z

@jmagne Thanks for the support and review!

fmarco76 requested review from edewata and jmagne August 5, 2022 15:58

fmarco76 force-pushed the pbeParserError branch 2 times, most recently from 5387c49 to e1e3681 Compare August 8, 2022 15:17

fmarco76 force-pushed the pbeParserError branch from e1e3681 to 1af4cc2 Compare August 8, 2022 16:10

fmarco76 force-pushed the pbeParserError branch 2 times, most recently from 9b32cb7 to 3fce2db Compare August 10, 2022 17:45

fmarco76 force-pushed the pbeParserError branch from 3fce2db to 95b7376 Compare August 19, 2022 08:53

fmarco76 marked this pull request as draft August 19, 2022 08:57

fmarco76 force-pushed the pbeParserError branch from 95b7376 to c5f7ad3 Compare August 23, 2022 17:02

fmarco76 changed the title ~~Add ASN1 decoding of encryopted data using PBES2 algorithm~~ Add ASN1 decoding of encrypted data using PBES2 algorithm Aug 23, 2022

fmarco76 force-pushed the pbeParserError branch 3 times, most recently from a24aecd to dde7b0f Compare October 17, 2022 09:09

fmarco76 mentioned this pull request Oct 17, 2022

Fix import .p12 files with sha256 hash #888

Closed

fmarco76 marked this pull request as ready for review October 17, 2022 09:30

fmarco76 requested review from ladycfu and removed request for edewata October 17, 2022 09:30

jmagne reviewed Nov 1, 2022

View reviewed changes

base/src/main/java/org/mozilla/jss/crypto/PBEKeyGenParams.java Outdated Show resolved Hide resolved

base/src/main/java/org/mozilla/jss/pkcs12/MacData.java Show resolved Hide resolved

base/src/main/java/org/mozilla/jss/pkcs12/MacData.java Outdated Show resolved Hide resolved

fmarco76 force-pushed the pbeParserError branch from dde7b0f to d13c36e Compare November 2, 2022 10:43

jmagne reviewed Nov 4, 2022

View reviewed changes

fmarco76 force-pushed the pbeParserError branch from 5149c6d to d406b34 Compare November 7, 2022 18:09

fmarco76 requested review from jmagne and cipherboy November 7, 2022 18:42

jmagne reviewed Nov 19, 2022

View reviewed changes

jmagne reviewed Nov 23, 2022

View reviewed changes

native/src/main/native/org/mozilla/jss/pkcs11/PK11KeyGenerator.c Show resolved Hide resolved

jmagne reviewed Nov 23, 2022

View reviewed changes

native/src/main/native/org/mozilla/jss/pkcs11/PK11KeyGenerator.c Show resolved Hide resolved

jmagne reviewed Nov 23, 2022

View reviewed changes

fmarco76 force-pushed the pbeParserError branch 2 times, most recently from ae86e86 to ad3c171 Compare November 30, 2022 19:27

jmagne reviewed Dec 2, 2022

View reviewed changes

jmagne approved these changes Dec 5, 2022

View reviewed changes

fmarco76 added 2 commits December 6, 2022 10:45

fmarco76 force-pushed the pbeParserError branch from f495ae9 to 71bb663 Compare December 6, 2022 09:50

fmarco76 merged commit 842f4dd into dogtagpki:master Dec 6, 2022

fmarco76 mentioned this pull request Dec 6, 2022

Unable to validate PKCS #12 files generated by pk12util #796

Closed

fmarco76 deleted the pbeParserError branch December 6, 2022 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ASN1 decoding of encrypted data using PBES2 algorithm #889

Add ASN1 decoding of encrypted data using PBES2 algorithm #889

fmarco76 commented Aug 5, 2022

edewata commented Aug 8, 2022

edewata commented Aug 8, 2022

ckelleyRH commented Aug 8, 2022

edewata commented Aug 8, 2022

ckelleyRH commented Aug 8, 2022

edewata commented Aug 8, 2022

ckelleyRH commented Aug 9, 2022

sonarqubecloud bot commented Aug 23, 2022

fmarco76 commented Oct 17, 2022

jmagne left a comment

jmagne left a comment

jmagne Nov 4, 2022

fmarco76 Nov 7, 2022

fmarco76 Nov 7, 2022

jmagne Nov 4, 2022

fmarco76 Nov 7, 2022

fmarco76 Nov 7, 2022

fmarco76 commented Nov 7, 2022

fmarco76 commented Nov 7, 2022

jmagne left a comment

jmagne Nov 23, 2022

fmarco76 Nov 24, 2022

jmagne Nov 23, 2022

jmagne left a comment

jmagne Dec 2, 2022

jmagne Dec 2, 2022

fmarco76 Dec 2, 2022

fmarco76 Dec 2, 2022

jmagne left a comment

sonarqubecloud bot commented Dec 6, 2022

fmarco76 commented Dec 6, 2022

fmarco76 commented Dec 6, 2022

Add ASN1 decoding of encrypted data using PBES2 algorithm #889

Add ASN1 decoding of encrypted data using PBES2 algorithm #889

Conversation

fmarco76 commented Aug 5, 2022

edewata commented Aug 8, 2022

edewata commented Aug 8, 2022

ckelleyRH commented Aug 8, 2022

edewata commented Aug 8, 2022

ckelleyRH commented Aug 8, 2022

edewata commented Aug 8, 2022

ckelleyRH commented Aug 9, 2022

sonarqubecloud bot commented Aug 23, 2022

fmarco76 commented Oct 17, 2022

jmagne left a comment

Choose a reason for hiding this comment

jmagne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarco76 commented Nov 7, 2022

fmarco76 commented Nov 7, 2022

jmagne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagne left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 6, 2022

fmarco76 commented Dec 6, 2022

fmarco76 commented Dec 6, 2022