1296263 - SEGV after loading module in crypto policy

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[David Woodhouse]

With bug 1279520 we introduced support for a system crypto policy. As discussed in https://bugzilla.redhat.com/show_bug.cgi?id=1157720#c13 it is intended that:

"You can also force loading other PKCS #11 modules (like trust stores) with this, but remember that this affects *ALL* nss programs (including programs that initialized without a database)."

And this is a desirable goal, because it helps us address bug 248722.

However, it doesn't work. If I add, for example, opensc-pkcs11.so to the system policy file to load it by default, I get this segfault:

Starting program: /usr/bin/certutil -d sql:/etc/pki/nssdb -L -h PIV_II\ \(PIV\ Card\ Holder\ pin\)
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
warning: the debug information found in "/usr/lib/debug//usr/lib64/libcrypto.so.1.0.2h.debug" does not match "/lib64/libcrypto.so.10" (CRC mismatch).


Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

Enter Password or Pin for "PIV_II (PIV Card Holder pin)":

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7608ac4 in PK11_DoPassword (slot=0x861140, session=8784112, loadCerts=1, wincx=0x7fffffffdb80, alreadyLocked=0, contextSpecific=0) at pk11auth.c:629
629		    nssTrustDomain_UpdateCachedTokenCerts(slot->nssToken->trustDomain,
Missing separate debuginfos, use: dnf debuginfo-install openssl-libs-1.0.2h-3.fc24.x86_64
(gdb) p slot->nssToken
$1 = (NSSToken *) 0x0


I can naively fix this my moving the #ifdef POLICY_FILE chunk up by a few lines so that it comes before the call to STAN_LoadDefaultNSS3TrustDomain().

Comment 1

[David Woodhouse]

Attachment: fix-policy-load-modules.patch

This makes things work.

Note also the caveat in in the above citation... "but remember that this affects *ALL* nss programs (including programs that initialized without a database)." I've fixed that too, by passing !noModDB as the final argument to SECMOD_LoadModule() instead of an unconditional PR_TRUE. Perhaps that should have been a separate patch, to prevent it from being overlooked in the moving of the code?

Comment 2

[Franziskus Kiefer [:franziskus]]

Elio, could you have a look at this?

Comment 3

[Elio Maldonado]

Comment on attachment 8782414 [details] [diff] [review]
fix-policy-load-modules.patch

Review of attachment 8782414 [details] [diff] [review]:
-----------------------------------------------------------------

r+, Looks good to me. Let me ask Bob whom I believe has been collaborating with David and if I'm not mistaken this was discovered as a result of this effort.

Comment 4

[Robert Relyea]

Moving it up above the STAN_LoadDefaultNSS3TrustDomain() is good. It should come after the SECOID_Init() call. I think it currently works because it ends up initing the OID table itself.

The !noMod should be a separate patch. The 'Note: this affects all NSS applications' was a functionality note, not a bug. We intended it to affect all NSS applications, so the !noMod should be dropped.

bob

Comment 5

[David Woodhouse]

Attachment: 0001-Bug-1296263-Fix-loading-of-PKCS-11-modules-from-syst.patch

First patch, just moving the code and fixing the indentation.

Comment 6

[David Woodhouse]

Attachment: 0002-Bug-1296263-Do-not-load-additional-modules-in-NoDB-i.patch

Second patch, fixing it *not* to load additional modules when called from NSS_NoDB_Init().

Comment 7

[Kai Engert (:KaiE:)]

Assigning to David, because he is working on it.

Comment 8

[David Woodhouse]

Strictly speaking, I'm not "working on it"; it's done, working, and awaiting review and commit.

I would *really* like this to get into the Fedora 25 release to let us fix https://bugzilla.redhat.com/show_bug.cgi?id=1173577

Comment 9

[Kai Engert (:KaiE:)]

(In reply to David Woodhouse from comment #8)
> Strictly speaking, I'm not "working on it"; it's done, working, and awaiting
> review and commit.

FYI, it's the usual procedure to assign a bug to the person who provided the patches to fix the bug.

Comment 10

[Robert Relyea]

Comment on attachment 8784362 [details] [diff] [review]
0001-Bug-1296263-Fix-loading-of-PKCS-11-modules-from-syst.patch

Review of attachment 8784362 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea

Comment 11

[Robert Relyea]

Comment on attachment 8784363 [details] [diff] [review]
0002-Bug-1296263-Do-not-load-additional-modules-in-NoDB-i.patch

Review of attachment 8784363 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like the patch was made compared to the wrong thing. It looks like it has an additional POLICY_FILE block in it. Thus the r-

My first thought that I didn't agree with the patch at all, but on further reflection, I think David is right. We *DO* need to load policy in the nodb init case, but we shouldn't load additional pkcs #11 modules. I'd want to make sure we are loading policy before approving the one line change which I think David is proposing. If the policy is getting loaded, then I think we're good to go. with that one.

r- only because I think the patch was malformed.

Comment 12

[David Woodhouse]

Attachment: 0002-Bug-1296263-Do-not-load-additional-modules-in-NoDB-i.patch

Apologies for that; evidently I screwed up when splitting the original patch into two parts. It was still working fine though; it doesn't hurt if you load policy twice, it seems.

If you are not loading policy in the NoDB case, that's an orthogonal issue — you *should* be, and if/when you fix that, this one-liner would prevent you from *introducing* a new bug when you do so.

Comment 13

[Robert Relyea]

Comment on attachment 8799486 [details] [diff] [review]
0002-Bug-1296263-Do-not-load-additional-modules-in-NoDB-i.patch

Review of attachment 8799486 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I've looked at the source code and I'm 95% sure that this change would prevent policy from being loaded in the noDB case.

The reason is the policy strings are read when the policy file is read as a moduleDB string, so the policy actually looks like a PKCS #11 module that does't actually load anything, but processes the policy file. If you turn recurse off, then policy doesn't get loaded.

I'm OK with the semantic David is trying to get (policy gets loaded, but other PKCS #11 modules do not if we are using noDBinit), but this patch does not accomplish that. There are a couple of ways we could do this:

We could add a NSS flag that says only load policy only modules, then set that flag in the noDBInit case. (changes to SECMOD_LoadModule in pk11wrap/pk11pars.c. It would definately be more complicated than this change.

Comment 14

[Robert Relyea]

> If you are not loading policy in the NoDB case, that's an orthogonal issue — you *should* be, and if/when
> you fix that, this one-liner would prevent you from *introducing* a new bug when you do so.

My worry was that your change would prevent policy from loading. I've check the code and verified that it would indeed prevent policy from loading.

Comment 15

[David Woodhouse]

Hmmm... I had established the opposite, although of course I'm happy to defer to your better knowledge. But can you help me understand why I got it wrong?

The policy module file looks like this:

library=
name=Policy
NSS=flags=policyOnly,moduleDB
config="disallow=ALL allow=HMAC-SHA1:HMAC-SHA256:HMAC-SHA384:HMAC-SHA512:SECP256R1:SECP384R1:SECP521R1:aes256-gcm:chacha20-poly1305:aes256-cbc:camellia256-cbc:aes128-gcm:aes128-cbc:camellia128-cbc:des-ede3-cbc:SHA1:SHA256:SHA384:SHA512:ECDHE-RSA:ECDHE-ECDSA:RSA:DHE-RSA:DHE-DSS:tls-version-min=tls1.0:dtls-version-min=dtls1.0:DH-MIN=1023:DSA-MIN=1023:RSA-MIN=1023"

library=/usr/lib64/pkcs11/opensc-pkcs11.so
name=p11-kit-proxy
NSS=trustOrder=100


My understanding was that when we explicitly load that with SECMOD_LoadModule(), it *will* get loaded. The final argument to SECMOD_LoadModule() is about *recursion*, and whether we load the *other* modules indicated in the file. I thought the main one would *always* be loaded, either way?

Comment 16

[Robert Relyea]

> My understanding was that when we explicitly load that with SECMOD_LoadModule(), it *will* get loaded.
> The final argument to SECMOD_LoadModule() is about *recursion*, and whether we load the *other* modules
> indicated in the file. I thought the main one would *always* be loaded, either way?

What get's parsed in SECMOD_LoadModule is the string we pass it, which says 'load softoken as a database module and pass it a database file of "policy.cfg"'. If recurse is FALSE, then softoken will get loaded, but it won't read any of the strings in policy.cfg. If recurse is TRUE, then softoken will return the modules strings from policy.cfg. Those module strings are in turn passed to SECMOD_LoadModule. The first one with the config= line will be parsed in SECMOD_LoadModule() which will load the policy (happens as a side effect of SECMOD_CreateModuleEx). We need a flag that says 'only load the policy files' which we can set if NoDBInit() is used.

bob

Comment 17

[Daiki Ueno [:ueno]]

Attachment: nss-check-policy-file.patch

(In reply to Robert Relyea from comment #16)

> What get's parsed in SECMOD_LoadModule is the string we pass it, which says
> 'load softoken as a database module and pass it a database file of
> "policy.cfg"'. If recurse is FALSE, then softoken will get loaded, but it
> won't read any of the strings in policy.cfg. If recurse is TRUE, then
> softoken will return the modules strings from policy.cfg. Those module
> strings are in turn passed to SECMOD_LoadModule. The first one with the
> config= line will be parsed in SECMOD_LoadModule() which will load the
> policy (happens as a side effect of SECMOD_CreateModuleEx). We need a flag
> that says 'only load the policy files' which we can set if NoDBInit() is
> used.

In Fedora, we keep a local patch that adds such flag (attached).  I am not sure where this patch was originally discussed, though.  Could this patch perhaps replace the second patch here?

Comment 18

[Robert Relyea]

Comment on attachment 8985905 [details] [diff] [review]
nss-check-policy-file.patch

Review of attachment 8985905 [details] [diff] [review]:
-----------------------------------------------------------------

r=rrelyea

Comment 19

[Daiki Ueno [:ueno]]

Comment on attachment 8985905 [details] [diff] [review]
nss-check-policy-file.patch

Thank you.  For the record, the change originally came from:
https://bugzilla.redhat.com/show_bug.cgi?id=1157720#c10

So I am pushing this under your name (with r=ueno).

Comment 20

[Daiki Ueno [:ueno]]

Landed as:
https://hg.mozilla.org/projects/nss/rev/d99e54ca9b6d
https://hg.mozilla.org/projects/nss/rev/93cbd336eaca