SEGV after loading module in crypto policy

NEW
Assigned to

Status

NSS
Libraries
P3
normal
2 years ago
7 months ago

People

(Reporter: David Woodhouse, Assigned: David Woodhouse)

Tracking

(Blocks: 1 bug)

3.26
Unspecified
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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().
(Assignee)

Updated

2 years ago
Blocks: 248722
(Assignee)

Comment 1

2 years ago
Created attachment 8782414 [details] [diff] [review]
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?
Elio, could you have a look at this?
Flags: needinfo?(emaldona)

Updated

2 years ago
Flags: needinfo?(emaldona)

Comment 3

2 years ago
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.
Attachment #8782414 - Flags: review+

Updated

2 years ago
Flags: needinfo?(rrelyea)
Attachment #8782414 - Flags: review?

Comment 4

2 years ago
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
Flags: needinfo?(rrelyea)
(Assignee)

Comment 5

2 years ago
Created attachment 8784362 [details] [diff] [review]
0001-Bug-1296263-Fix-loading-of-PKCS-11-modules-from-syst.patch

First patch, just moving the code and fixing the indentation.
Attachment #8782414 - Attachment is obsolete: true
Attachment #8782414 - Flags: review?
(Assignee)

Updated

2 years ago
Attachment #8784362 - Flags: review?(rrelyea)
(Assignee)

Comment 6

2 years ago
Created attachment 8784363 [details] [diff] [review]
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().
Attachment #8784363 - Flags: review?(rrelyea)

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

2 years ago
Assigning to David, because he is working on it.
Assignee: nobody → dwmw2
(Assignee)

Comment 8

2 years ago
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(kaie)

Comment 9

2 years ago
(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.
Flags: needinfo?(kaie)

Updated

2 years ago
Flags: needinfo?(rrelyea)

Comment 10

2 years ago
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
Attachment #8784362 - Flags: review?(rrelyea) → review+

Comment 11

2 years ago
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.
Attachment #8784363 - Flags: review?(rrelyea) → review-
(Assignee)

Comment 12

2 years ago
Created attachment 8799486 [details] [diff] [review]
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.
Attachment #8784363 - Attachment is obsolete: true
Attachment #8799486 - Flags: review?(rrelyea)

Comment 13

2 years ago
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.
Attachment #8799486 - Flags: review?(rrelyea) → review-

Comment 14

2 years ago
> 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.
Flags: needinfo?(rrelyea)
(Assignee)

Comment 15

2 years ago
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

2 years ago
> 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

Updated

7 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.