Open Bug 1884444 Opened 2 months ago Updated 4 days 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

(Not tracked)

Status:
ASSIGNED

People

(Reporter: rrelyea, Assigned: rrelyea, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1884444 SMIME/CMS and PKCS #12 do not integrate with modern NSS policy r=kaie
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: