*** CID 1674977:         Memory - illegal accesses  (USE_AFTER_FREE)
/lib/softoken/pkcs11c.c: 5411             in sftk_compareKeysEqual()
5405         result = PR_TRUE;
5406     loser:
5407         if (key1obj) {
5408             sftk_FreeObject(key1obj);
5409         }
5410         if (key2obj) {
5411             sftk_FreeObject(key1obj);   <--- Typo
5412         }
5413         if (att1) {
5414             sftk_FreeAttribute(att1);
5415         }
5416         if (att2) {

>>>     CID 1674977:         Memory - illegal accesses  (USE_AFTER_FREE)
>>>     Calling "sftk_FreeObject" dereferences freed pointer "key1obj".

This looks like a fairly straightforward typo leading to a UAF/ double free.

It looks like the code landed in this Phab patch. It also looks like line 5379 is incorrect (returning a truthy error value in a PRBool function).

The use is right after the free so it doesn't seem like it is too dangerous in practice but I'll mark it sec-high anyways.

sftk_FreeObject() is more idempotent than you would expect a free function to be, It mostly clears the structure an puts it on a free list, which is why this doesn't blow up right away (unfortunately, or I would have seen it in my testing) It only happens in FIPS mode, which isn't a problem for mozilla released firefox.

We definitely should fix this. I have the same code downstream RHEL already, so I'll need to Fix if there. I'll wait until I find out if this will be an embargoed bug.

The fix is obvious (that should be key2obj rather than key 1).

To make it clear, I'm fine with opening this bug and and landing the patch. This actually creates a memory leak, not a double free.

Bob, I think there are a few other problems here too. For one, I don't think this code is properly guarded to be FIPS only. I reach this isKEM check in non-FIPS mode while running

pk11_gtest -s gtests/pk11_gtest/ -d <dbdir> -w  --gtest_filter="*Pkcs11KEMTest/Pkcs11KEMTest.KemConsistencyTest/2*"

Second, when I do reach that line, isKEM evaluates to false and the pairwise consistency check is not evaluated at all.

I rebuilt with FIPS mode enabled and ran modutil -fips true -dbdir . in the db dir and the gtest fails much earlier in keygen. Am I doing something wrong? How do I run this test in FIPS mode?

I should have been clear, I meant that mozilla would only make this call in FIPS mode. When TLS does a keygen, it uses a different mechanism which bypasses integrity checks (CKM_NSS_ML_KEM_KEYPAIRGEN) when not in FIPS mode, so you aren't seeing the issue when running any SSL tests. The pk11_gtest, is testing multiple CKM_ mechanism, including CKM_ML_KEM_KEYPAIRGEN. Since we don't have permanent CKK_ML_KEM keys, the only use of ML_KEM currently is TLS. That is where is get 'mozilla isn't tripping over this code because it isn't running in FIPS mode'.

Putting NSS into FIPS mode can be done in 2 ways: 1) the modutil command you described on the database you are running the tests on. or 2) on Linux putting the system into FIPS mode, or setting "NSS_FIPS" to 1 (or 'y' or 'fips' or 'true' or 'on')

(In reply to Robert Relyea from comment #7)

The pk11_gtest, is testing multiple CKM_ mechanism, including CKM_ML_KEM_KEYPAIRGEN.

Right, and it's still not hitting the issue. So, if I understand correctly, not only is this not an issue for Firefox, but it's not an issue for any NSS consumer. We need a separate bug to fix the fact that the ML-KEM pairwise consistency checks are not functional.

I was confused by your original comment, I thought you were tripping over this issue. Since you aren't I don't think this is security sensitive because we actually aren't getting to this code in either case. I'll look into way it's not tripping over this.

I've verified we can't get into the broken code with the current version of nss. The issue is that this code is triggered by:
isKEM=sftk_isTrue(privateKey, CKA_ENCAPSULATE);
This can't happen because privateKeys will never have CKA_ENCAPSULATE=TRUE (any more than they would have CKA_ENCRYPT, or CKA_WRAP, or CKA_VERIFY=TRUE), it's a public key attribute.

The correct code should be:
isKEM=sftk_isTrue(privateKey, CKA_DECAPSULATE);

(there appears to be other issues as well, but setting this value will create test failures in pk11gtests in the isKEM check code).

As such this bug should not be security sensitive, as this is a simple coverity bug that needs to be fixed before we fix the above bug.

how quickly can we get a decision on openning this up. I have a RHEL update that needs to get out and I want to include this fix, but I don't want to break embargo.

We can clear the security flag now since the code is not reachable.

Yes please!

dan per comment 12 and comment 13 please clear the security flag, thanks.

Looks like John already did that. Is there something he missed?

oh, it is cleared, thanks!

The second issue is covered by bug 2005669