Closed Bug 1884444 Opened 1 year ago Closed 1 year ago

SMIME/CMS and PKCS #12 do not integrate with modern NSS policy

Categories

(NSS :: Libraries, enhancement, P2)

Product:
NSS
Component:
Libraries
Type:
enhancement

Tracking

(firefox128 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox128 --- fixed

People

(Reporter: rrelyea, Assigned: rrelyea)

References

Details

Attachments

(2 files)

Bug 1884444 SMIME/CMS and PKCS #12 do not integrate with modern NSS policy r=kaie
48 bytes, text/x-phabricator-request
Details | Review
Bug 1884444 SMIME/CMS and PKCS #12 do not integrate with modern NSS policy r=jschanck
48 bytes, text/x-phabricator-request
Details | Review

We've upgraded the NSS policy code to be configurable, and tied to oid tags. SSL, certificates, and Signing have all been integrated into the policy code already. PKCS #12 has been partially integrated. S/MIME still uses the only export policy code that and been mostly set to a static value these days.

We need to integrate S/MIME into the existing NSS system policy. This will allow us to control S/MIME configuration with tools like Red Hat's system policy (allowing us to turn off RSA-PKCS in the future, for instance).

Unlike SSL, S/MIME and PKCS #12 really has two policy choices: 1) Encrypt/Sign/Creation and 2) Decrypt/Verify/Legacy. The first would be policies against new content, while the latter would be policies against parsing existing content. Often we want to turn off weak algorithms (like SHA1) in creating new content, but not turn it off when we are trying to process old, existing stuff. At the same time some algorithms are so broken we don't want to trust using them even for existing stuff (verify with MD2, for instance). This means we should separate this operations in S/MIME and pkcs12, where we wouldn't do so in SSL.

Adding kai to the cc list as it affects S/MIME on thunderbird. The patch should also help point out where new S/MIME features like OAEP, PSS, DH and ECDH Key exchange would go.

This patch addes the following policy flags:
NSS_USE_ALG_IN_SMIME_ENCRYPT
NSS_USE_ALG_IN_SMIME_LEGACY
NSS_USE_ALG_IN_SMIME_KX_ENCRYPT
NSS_USE_ALG_IN_SMIME_KX_LEGACY
NSS_USE_ALG_IN_PKCS12_ENCRYPT
NSS_USE_ALG_IN_PKCS12_DECRYPT

The NSS_USE_ALG_IN_PKCS12_DECRYPT reuses the old NSS_USE_ALG_IN_PKCS12 flag. The latter is now a combination of NSS_USE_ALG_IN_PKCS12_DECRYPT and NSS_USE_ALG_IN_PKCS12_ENCRYPT. The _LEGACY and _DECRYPT flags handle cases where we are dealing with preexisting files we want to process. In these cases we want policy to be more lenient so we can still import keys and read old email. The corresponding _ENCRYPT flags are only meaningfull if the _LEGACY flags are set. Combo defines of NSS_USE_ALG_IN_SMIME, NSS_USE_ALG_IN_SMIME_KX, and NSS_USE_FLAGS_IN_PKCS12 represent this combination, and all the flags should be set.

In adding new S/MIME policy for encryption algorithms, we now clear those bits from all policy oids. This allows us to use the policy oids to figure out what algorithms are even possible. If no policies for S/MIME are turned on, then we will use the default S/MIME values, and those will be turned on implicitly. This also prevents us from turning on RC4 or other ciphers that would 'work' with S/MIME but should be enabled generally (someone could turn on RC4 manually, or explicity, but just adding enable=rc4 will only turn on SSL and not S/MIME (you'd have to say enable=rc4/smime to get smime explicity).

Like SSL, we can control the default enabled ciphers for S/MIME. This primarily controls what ciphers are included in our capabilities list we send to others. Even though a policy may be enabled for legacy, we only include it in the list if it is allowed for both legacy and encrypt. The theory here is we want to decrypt email sent by others in the past, or using an old S/MIME capabilities, but we want them to use the stronger ciphers in the future. If you dont' enable any S/MIME ciphers explicitly, then S/MIME will enable the existing default ciphers (which are allowed by policy). The whole enable intrastructure is replaced from a static array with bools, to a dynamic list that includes that enabled oid in preference order. If the oids are enabled through the policy system, then the system sorts the oids by key length and preferring older known ciphers to new ones.

Key length checks for key exchange have been added as well. Key length checks for signature already happens in the signature code.

While implementing and testing a few bugs were identified and fixed.

Detailed explanation of the patch:

cmd/pk12util/pk12util.c

  • remove the explicit enable calls. All ciphers are already enabled by default, and we want pk12util to accept system policy changes.
    cmd/smimetools/cmsutil.c
  • when we add certificates, also add the S/MIME profile to the database.
  • fix reference and memory leak in the signerinfo if we fail before the signerinfo is added to the NSSCMSMessage.
  • remove hard coded Hash identifiers and fetch the hash from the OID table and verify it with the HASH_ functions.
    lib/certhigh/certvfy.c
  • check the KEY_SIZE_FLAGS for KEY_SIZE_VERIFY before applying the min key lengths. This allows us to test the S/MIME
    KEA KEY_SIZE_FLAGS by just setting the KEY_SIZE_SMIME flag explicitly in the test. I've updated the default flags to
    have all the flags on by default (not just SSL).
  • (NOTE: I did not try to update the ECC side with ECC min lengths at this point).
    lib/cryptohi/keyhi.h
    lib/cryptohi/keyi.h
    lib/cryptohi/seckey.c
    lib/cryptohi/secsign.c
    lib/cryptohi/secvfy.c
  • export SECKEY_EnforceKeySize for use in S/MIME
    lib/nss/nss.def
    automation/abi-check/expected-report-libnss3.so.tx
  • export a bunch of new functions:
    • HASH_GetHashOidTagByHMACOidTag,SECKEY_EnforceKeySize, and SECKEY_PrivateKeyStrengthInBits are existing functions that S/MIME or the tools need to call
    • SEC_PKCS5GetCryptoFromAlgTag, SEC_PKCS5GetHashAlgorithm, and SEC_PKCS5GetHashFromAlgTag are new functions to extract algtags from PKCS5pbe oidtags and algorithm IDs so they can be checked against policy.
      lib/nss/nss.h
  • add NSS_KEY_SIZE_POLICY_SMIME
    lib/nss/nssoptions.c
  • update the key size default to have all the flags, not just ssl.
    lib/pk11wrap/pk11nobj.c
  • fix bug in pk11nobj.c where we weren't finding S/MIME profile objects in the database if the database was sql
    lib/pk11wrap/pk11pars.c
  • Add SMIME and PKCS12 identifiers and flags.
  • fix bug in secmod_getPolicyOptValue so it succeeds when parsing key-size paramters.
  • Enable/disable setting has been moved to a helper function, secmod_setDefault to handle the case that we now have separate ssl and s/mime enablement. (NOTE: remember enable implies allow)
    lib/pk11wrap/pk11pbe.c
    lib/pk11wrap/secpkcs5.h
  • implement new functions to extract algtags from PKCS5pbe oidtags and algorithm IDs so they can be checked against policy.
    lib/pk11wrap/pk11priv.h
    lib/pk11wrap/pk11pub.h
  • export PK11_GetMaxKeyLength
    lib/pkcs12/p12d.c
    lib/pkcs12/p12e.c
    lib/pkcs12/p12plcy.c
    lib/pkcs12/p12plcy.h
  • add pkcs12 policy checks.
  • p12plcy decomposes the PBE into encryption and hash algorithms and checks policy on both. It deals with the differences between pkcs5v1 pkcs12pbe and pkcs5v2
    lib/smime/cmsdigest.c
    lib/smime/cmsencdata.c
    lib/smime/cmsrecinfo.c
    lib/smime/cmssiginfo.c
  • add smime policy checks
  • NSS_CMSRecipientInfo_UnwrapBulkKey was reorded to collect the algid and encryption key then handling the actual operation in a new encalgtag switch.
    lib/smime/smime.def
    automation/abi-check/expected-report-libsmime3.so.txt
  • export the new PKCS12 helpers to return policy (shouldn't we export the smime ones as well?)
    lib/smime/smime.h
  • export the new S/MIME utils to checking various s/MIME policies.
    lib/smime/smimemessage.c
  • Fix same signerinfo leak as in cmd/simetools/cmsutil.c
    lib/smime/smimeutil.c
  • This file has been pretty heavily rewritten. There used to be a static array which mapped the old SMIME algorithm value to and OID, a parameter, enabled/disabled, and allowed/disallowed (policy). The table has been repurposed to make and the old SMIME algorithm to an OID tag which reflects the policy and is now a CONST table. It also functions as the list of known old algorithms to default to if no algorithms are set by policy. NOTE that SEC_OID_RC2_XXX_CBC are 'fake' oids to allow mapping the policy with keysize explicitly. This matches all the other algorithms which encode the key length in the oid.
  • smime_legacy_to_policy and smime_legacy_to_oid maps the old SMIME algorithm values to the modern oid value. This allows the old functions to still use the old values, but also work using an actual oid.
  • smime_allowed_by_policy is a helper to check an algtag for a particular policy. All requested policy bits must match.
  • smime_key_size_by_cipher is a helper to get the keysize from an algtag. The general case should be handle in pk11wrap, but we dont' have a function there to do that yet.
  • SMIMEList and smime_algorithm_list is a simple array list, ordered by algorithm preference. This replaces the enable field of the old static algorithm table. smime_list_ are helper functions. NOTE: smime_list_index_find returns table_size if the element is not found. This means you need to know smime_list_length when this function is called, and you need to lock around references to prevent the table from changing while you work with it.
  • smime_init_once handles initializing our policy and enable arrays based on existing policy and handling the default cases when no policy has been enabled.
  • nss_SMIME_FindCipherForSMIMECap, smime_choose_cipher,NSS_SMIMEUtil_FindBulkAlgForRecipients,smime_create_capability have been rewritten to use the new policy system and enable list. RC2 was the only cipher that needed parameters, so the parameters are generated on the fly if necessary.
  • NSS_SMIMEUtil_CreateSMIMECapabilities was well as being recoded to use the new lists, It's also been updated to include hash and key encipherment algorithms.
    lib/ssl/sslinit.c
  • add a shutdown function so that we reset the default algorithms and policy if NSS is initialized again after shutdown. (probably haven't ran into any issues because few applications of any complexity can actually successfully shut NSS down.
    lib/util/nssutil.def
    lib/util/secoid.c
    lib/util/secoid.h
  • add new helper functions to 1) return the number of recognized oid tags in the system (including dynamic oids), 2) Find all the oids which are approved for a particular policy (used by SMIME to determine what oids are valid and if we need to initialize policy from the default list, 3) set or clear a policy on all the oids (used to clear the S/MIME policy so getting all the S/MIME policy oids are meaningful).
  • also add the RC2_XXX fake oids to the oid table (RC2_40 already exists).
  • clear the SMIME policy from oids by default.
  • allow legacy decrypt for PKCS #12 files decrypting old integrity or pbe hash values. Only allow reading, not generation.
    lib/util/secoidt.h
  • add the RC2_XXX fake oids to the oid table (RC2_40 already exists).
  • add the new NSS_USE_ALG defines. make CMS_SIGNATURE an alias for SMIME_SIGNATURE and keep the same bit.
  • NSS_USE_ALG_IN_PKCS12 becomes NSS_USE_ALG_IN_PKCS12_DECRYPT, and the new NSS_USE_ALG_IN_PKCS12 becomes NSS_USE_ALG_IN_PKCS12_DECRYPT|NSS_USE_ALG_IN_PKCS12_ENCRYPT
  • Add more combo defines.
    -------------------------------------- tests -------------------------------------------------
    tests/common/init.sh
    tests/policy/policy.sh
    tests/ssl/ssl.sh
  • move the generic policy setup code from ssl.sh to init.sh add generic save and restore functions.
  • move the ignore_blank_lines macro from ssl.sh and policy.sh to init.sh
    tests/policy/crypto-policy.txt
  • add s/mime policy values to verify parsing validity
    tests/smime/smime.sh
  • add loop to test smime policy. Each tests uses two entities, a sender and a receiver. Each gets it's own policy.
    • The reciever first creates a signed s/mime message with capabilities.
    • The sender then receives the message, and if successful stores the cert and the capabilities.
    • The sender then encrypts a message. The chosen cipher is verified to be the expected cipher.
    • The receiver decrypts the message and verifies it.
    • The expected return values are checked for each operation, which may fail by policy. If the expected operation does not fail by policy when it should, it generates a test failure.
      tests/smime/smimepolicy.txt
  • new file with the list of policy tests used by the loop above. new tests can be added by simply adding them to this file.
    tests/tools/tools.sh
  • existing helpers were updated with new parameters which can vary in the policy case.
  • add loop to test pkcs12 policy. Each test sets up two entities, an exporter and an import. Each gets it's own policy
    • The export exports a private key.
    • The importer imports the private key.
    • The expected return values are checked for each operation, which may fail by policy. If the expected operation does not fail by policy when it should, it generates a test failure.
      tests/tools/pkcs12policy.txt
  • new file with the list of policy tests used by the loop above. new tests can be added by simply adding them to this file.
Blocks: 1894459
Blocks: 1895370

The patch appears to contain a functional change.
The PSM test security/manager/ssl/tests/unit/test_certDB_import_pkcs12.js fails with the patch applied.
In particular, importing this file fails:
security/manager/ssl/tests/unit/test_certDB_import/encrypted_with_aes.p12

I can reproduce with pk12util:

$ pk12util -d /tmp/db-new -i /tmp/encrypted_with_aes.p12 
Enter password for PKCS12 file: 
pk12util: PKCS12 decode not verified: SEC_ERROR_BAD_DER: security library: improperly formatted DER-encoded message.
pk12util: PKCS12 decode validate bags failed: SEC_ERROR_INVALID_ARGS: security library: invalid arguments.
Flags: needinfo?(rrelyea)

Here is a Thunderbird try run (not yet using Bob's patch from yesterday, but from before that).
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9844ac550051387946efae547b06e94065147177

Thanks for the reproducer kai. What is the password on the .p12 file?
-- never mind. found the .js file and it was there.

Interesting. the .p12 file doesn't have any pkcs5v2 parameters even though it has a pkcs5v2 oid...

[rrelyea@localhost tmp]$ pp -t pkcs12 -i encrypted_with_aes.p12
PKCS #12 File:
Version: 3 (0x3)
AuthSafe:
Safe 1:
PKCS #7 Encrypted Data:
Version: 0 (0x0)
Encrypted Content Information:
Content Type: PKCS #7 Data
Content Encryption Algorithm: PKCS #5 Password Based Encryption v2
Encryption:
KDF: PKCS #5 Password Based Key Dervive Function v2
Parameters:
Cipher: AES-128-CBC
Args:
04:10:d2:95:d5:24:11:17:c8:51:99:d8:be:88:
b8:c4:a7:d1

Expecting something like:[rrelyea@localhost tools]$
pp -t pkcs12 -i Alice.p12
PKCS #12 File:
Version: 3 (0x3)
AuthSafe:
Safe 1:
Bag 1 ID: PKCS #12 V1 PKCS8 Shrouded Key Bag
Encrypted Private Key:
Encryption Algorithm: PKCS #5 Password Based Encryption v2
Encryption:
KDF: PKCS #5 Password Based Key Dervive Function v2
Parameters:
Salt:
50:ab:19:b2:8d:08:92:88:e6:6c:eb:f0:f4:1b:
0a:96
Iteration Count: 10000 (0x2710)
Key Length: 24 (0x18)
KDF algorithm: HMAC SHA-384
Cipher: CAMELLIA-192-CBC
Args:
04:10:74:79:fe:b6:60:27:6c:91:3f:29:fa:c9:6c:
67:87:6e

It looks like we previously defaulted to something here... checking with the old code.
Encrypted Data:

OK I've found the problem. The pkcs#5PBE parameters are there, but they kdf (which is optional) is not. In that case we are supposed to default to HMAC_SHA1. So I found 2 issues here: 1) the pp tool was not handling the case where the kdf wasn't present. I've fixed that in the pp tool, and 2) the new code to extract the underlying hash alg for the policy check code didn't default to SHA1 if the prf value didn't exist.

I'll attach a new patch shortly which fixes 1, 2, fixes the spelling of 'Derive' in the PKCS #5 v2 KDF description (see output above), and adds the .p12 file Kai supplied to the NSS ci test so we catch any of these issues earlier.

Flags: needinfo?(rrelyea)

Sigh, it should be updated now... The update was waiting for me to 'confirm'.

Thanks Bob. I'm looking into it shortly and will rerun the tests. We are close.

I see at least two more problems.

This test fails with the patch:
security/manager/ssl/tests/unit/test_ocsp_stapling.js | - Actual and expected connection result should match - 2153394070 == 2153389936

These tests run into crashes with the patch:
security/manager/ssl/tests/unit/test_content_signing.js
toolkit/mozapps/extensions/test/xpcshell/test_ProductAddonChecker_signatures.js

I haven't investigated the cause of the crash, and I don't know if it's the same or different crashes.
(I'm off today, public holiday.)

Flags: needinfo?(rrelyea)

The first test is a policy issue. It's rejecting the signature algorithm for some reason (probably too small?):
[task 2024-05-08T22:05:29.924Z] 22:05:29 INFO - PID 29213 | found pre-defined host 'keysize-ocsp-delegated.example.com'
[task 2024-05-08T22:05:29.924Z] 22:05:29 INFO - PID 29213 | found pre-defined host 'keysize-ocsp-delegated.example.com'
[task 2024-05-08T22:05:29.924Z] 22:05:29 INFO - PID 29213 | CreateEncodedOCSPResponse failed: SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED
[task 2024-05-08T22:05:29.924Z] 22:05:29 INFO - PID 29213 | PR_Recv failed: SSL_ERROR_UNRECOGNIZED_NAME_ALERT

It's only failing under Linux. I wonder if you have policy controls set up on Linux or are we just defaulting. Is the test_ocsp_stapling.js running on OSX and Windows as well?

Not clearing my need info until I've figured out what the signature is that's failing.

Flags: needinfo?(rrelyea) → needinfo?(kaie)

So it looks like this is a test that is supposed to fail, and we just now fail at a different spot with a different error code. I'm guessing it's an issue with the size of the key on the certificate. CreateEncodedOCSPResponse isn't in NSS, so I don't know exactly what function it's calling that returns the SIGNATURE_ALGORITHM_DISABLED error. My guess is this is OK, but we should verify what exactly changes. If you can point me to CreateEncodedOCSPResponse I can verify where that failure is happening. My guess it's before the actual OCSP Verify. The actual error code numbers are printed as an unsigned value, so my nsserr program doesn't know how to process it correctly.

I'm not seeing the crash cases in the tests, which tests have the two crash cases?

keep my own needinfo on until we get these answered

Flags: needinfo?(rrelyea)
Attachment #9390371 - Attachment description: Bug 1884444 SMIME/CMS and PKCS #12 do not integrate with modern NSS policy → Bug 1884444 SMIME/CMS and PKCS #12 do not integrate with modern NSS policy r=kaie

OK, it looks like the issue is the tests is generating the invalid signature which we are testing on the fly, but because of the policy code, that signature isn't being created (thus the different error code). If we want the test to succeed, we'll have to turn off policy in the server test code just before the signature. I know firefox does turn off policy length checks, at least for RSA, and then uses PSM do the checks in a way that can be overridden. I wonder if we need to adjust that code for ECC as well.

Flags: needinfo?(rrelyea)

Hmm I'm confused now. I didn't add ECC lengths to this patch, so we shouldn't be failing on an ECC straight signature simply as a result of this patch. Also, kai, do you have any information on the crashes? I'm sort of stuck now because I don't have the ability to run the test cases myself and only have the logs to go on.

Let's start by clearing up the situation with the failed OCSP test.

I'm proactive here. It's actually an issue that isn't related to Thunderbird. The consequence of Bob's suggested change to NSS is that a test in PSM is failing. So let's CC keeler.

Thanks Bob for pointing out that this is a policy issue. It indeed seems to be. And I'm running on a Fedora Linux system, and when I execute the tests locally, my system policy probably gets honored.

We see error code SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED.
It's happening in a test that has this comment:
// Check that OCSP responder certificates with key sizes below 1024 bits are
// rejected, even when the main certificate chain keys are at least 1024 bits.

However, with the patch applied, we don't even get to that point any longer (at least on Fedora with system policy).
We don't get to the point where a responder certificate is rejected.

Rather, we fail one step earlier, when our test code attempts to produce that OCSP response,
in CreateEncodedOCSPResponse.

When uplifting this patch into mozilla-central, we might need an adjustment to this automated test.

I think this could be simply seen as a follow-up TODO, and doesn't need to block this NSS level bug.

keeler, FYI:

I have attempted to fix the test by changing the test to:
const SSL_ERROR_UNRECOGNIZED_NAME_ALERT = SSL_ERROR_BASE + 106;
add_ocsp_test(
"keysize-ocsp-delegated.example.com",
SSL_ERROR_UNRECOGNIZED_NAME_ALERT,
true,
true
);

But that change isn't sufficient, because we get an unexpected number of failed/good tests.

Flags: needinfo?(kaie)

(In reply to Robert Relyea from comment #20)

Also, kai, do you have any information on the crashes? I'm sort of stuck now because I don't have the ability to run the test cases myself and only have the logs to go on.

It's a shutdown crash, so probably new leaking?

0:01.03 pid:165096 [165096] Hit MOZ_CRASH(NSS_Shutdown failed) at /home/user/moz/commcent/mozilla/xpcom/build/XPCOMInit.cpp:765

What's your preference, should we go ahead and approve/land the NSS patch, and fix the shutdown crash in a follow-up bug?

keeler is out this week and next. File bugs for any PSM changes that need to land with the NSS uplift and I'll take care of them.

(In reply to John Schanck [:jschanck] from comment #24)

keeler is out this week and next. File bugs for any PSM changes that need to land with the NSS uplift and I'll take care of them.

Thanks John.

I've filed bug 1898166 for the OCSP test.

I think the leaks should probably be fixed as part of this part here, but it might be easier to manage as an incremental patch.

Bob, can you please have a look at the leaks that John identified in Phabricator and possibly file a follow-up patch to fix them?

Flags: needinfo?(rrelyea)

I've updated the patch to fixed the leaks John pointed out. They were all leaks, most were preexisting and couldn't cause a shutdown failure in NSS, but one was potentially new and did cause a shutdown failure. Anyway the updated patch should fix all the ones John pointed out.

bob

Flags: needinfo?(rrelyea)

I still get the shutdown crash with Bob's updated patch.

I did some experiments, trying to identify the test that triggers the crash.

In file
toolkit/mozapps/extensions/test/xpcshell/test_ProductAddonChecker_signatures.js

If I disable the test test_valid_content_signature() - then it passes without crash.

So the shutdown issue must be related to the specific scenario that is triggered in this test - which seems about a valid signature - while the others tests that handle invalid signatures don't trigger the shutdown crash apparently.

Maybe this helps you.

I was able to track it down further.

The following block is responsible:
https://searchfox.org/mozilla-central/source/security/manager/ssl/ContentSignatureVerifier.cpp#369-402

If I remove that block of code, both tests don't crash.

It cannot be as simple as not destroying the result of VFY_CreateContext, I think we can assume that mozilla::UniqueVFYContext automatically calls VFY_DestroyContext.

As long as the destructor is called before shutdown. This is a c++ function, not a Java or js function, so we aren't waiting on a garbage collector, and cx goes out of scope before the function returns, so it's unlikely shutdown happens before (NSS appears to be initialized before we call this function, so we aren't running into some arbitrary destructor race situation. I've verified that mozilla::UniqueVfyContext has VFY_DestroyContext as the destroy function.

Kai, if you run this test against the tip of NSS without my patch explicitly, do you have issues. There is a new patch for signatures that landed (another one of mine) a few days ago.

Hmmm I see that we aren't destroying the PK11Context in DestroyContext (or in VFY_End(), though that one is fine as long as we destroy it in Destroy). That could very well cause this issue. That wasn't part of this patch, but the previous one I talked about in the above paragrah. Easy to add the fix to this patch, probably cleaner to supply a separate patch.

Bob, sorry for not noticing earlier. You're right.

The shutdown crash is introduced by the tip of NSS (post version 3.100),
so it's unrelated to your patch.

Thanks... well it's unrelated to this patch. It's related to the other patch I checked in... I've now determined why the NSS tests didn't pick this up. We don't have any tests for the VFY_Begin... VFY_Update... VFY_End interfaces directly. Most of your code uses the VFY_Dataxxxx or VFY_Digestxxxx, neither of which has this leak... only if you call VFY_Begin.... I've verified that the Signing interfaces don't have the leak.

The bug in question is bug 1827444.

oK, I'll push this patch and attach the leak fix to bug 1827444 once the try builds are complete.

https://treeherder.mozilla.org/jobs?repo=nss&revision=86e2a841f0cd7640ac3ba9002dc85066d9bb0362

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

Looks like this broke the Tools task.

Sample error:

pk12util -o "Alice.p12" -n "Alice" -d "../alicedir" \
         -k ../tests.pw -w ../tests.pw \
         -c "PKCS #5 Password Based Encryption with SHA-1 and DES-CBC" \
         -C "PKCS #5 Password Based Encryption with MD2 and DES-CBC" \
         -M "SHA-1"
pk12util: key or cert safe creation failed: SEC_ERROR_BAD_EXPORT_ALGORITHM: Required algorithm is not allowed.
Error attempting to export certificates.tools.sh: #13: Exporting with [PKCS #5 Password Based Encryption with SHA-1 and DES-CBC:PKCS #5 Password Based Encryption with MD2 and DES-CBC:SHA-1] (pk12util -o), - FAILED
Flags: needinfo?(rrelyea)

Looks like I missed a point for the 'export NSS_ALLOW_WEAK_SIGNATURE_ALG=1'. The new policy code rejects attempts to generate old hash functions, so the tests can't generate them. I had already included those in the test cases ran on debug builds, but optimized builds have a bigger (production level) iteration count, which makes running all the tests much slower, so only a fraction are run and the code that run that fraction didn't have the export.
I thought I had checked the test results. The Debug ones would succeed (and my local tests were against debug builds). I don't know why I didn't notice the Optimized build failures.

Flags: needinfo?(rrelyea)

Hey, we have observed that now there is a test that crashes:

here, for example, https://treeherder.mozilla.org/jobs?repo=nss-try&selectedTaskRun=8GdPNX-9QrCfqAUW5drHug.0&revision=b7114c18e71c5b7c3babca69c1ffe82eb2b68891
in tools.sh test#3 -

tools.sh: #3: Listing Alice.p12 (pk12util -l)  - PASSED
tools.sh: Importing Alice's pk12 Alice.p12 file to ../tools/copydir
certutil -F -d ../tools/copydir -n Alice -f ../tests.pw
certutil: could not authenticate to token NSS Certificate DB.: SEC_ERROR_IO: An I/O error occurred during security authorization.
Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:89
./tools.sh: line 152:   182 Aborted                 (core dumped) certutil -F -d ${3} -n ${2} -f ${R_PWFILE}

It was not the case before,
try run from 15th:
https://treeherder.mozilla.org/jobs?repo=nss&revision=bd06fa3a679c7d788e30cb536023945807411bcb
https://firefoxci.taskcluster-artifacts.net/-wnMaFsUSaaSFI5yiyFIxQ/0/public/logs/live_backing.log

Is it expected?

P.s. if it helps: the test is import_p12_file

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Update:
we found a leakage location. See https://phabricator.services.mozilla.com/D212113.

Unfortunately, the test still returns a problem during authentication (DeleteCertAndKey function).

Flags: needinfo?(rrelyea)

It it doesn't crash, then the tests should continue. The function isn't an actual test, it's just a paranoia removal of the previous imported key before we import a new one. The test that fails is the first case, so failure to delete the non-existant key isn't an issue.

I think I know why it's not failing on my machine... the failures are the first instance of the import. On a normal run of the tools, all the certs and databases are freshly generated, but I think we are running into a case where the database was stored and copied from some test. What I don't know is why the failure is spurratic.

Flags: needinfo?(rrelyea)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: