595988 - NSS trusts CAs it shouldn't (trusts system db over user db)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[David Woodhouse]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.9.2.7) Gecko/20100803 Fedora/3.6.7-1.fc13 Firefox/3.6.7
Build Identifier: 

When nsssysinit is being used, if a certificate is present in the system database in /etc/pki/nssdb and *also* in the user's database in $HOME/.pki/nssdb, shouldn't the trust bits on the latter override the former? They don't -- it works the other way round instead.

Worse, even if I *remove* the certificate from the system database so it should *only* be in the user's database, the trust bits from the system database are *still* overriding the user's trust bits!

More detail at https://bugzilla.redhat.com/show_bug.cgi?id=633043

Some of this may be related to nsssysinit-related patches in the Fedora package (without which nsssysinit is broken anyway)

Reproducible: Always

Comment 1

[Elio Maldonado]

Attachment: Prefer user's db trust preference over system's db - submitted to fedora

Comment 2

[Elio Maldonado]

Attachment: steps to reproduce and verify the fix

Comment 3

[Elio Maldonado]

Download the ca-cert for testing from http://www.cacert.org/index.php?id=3

Comment 4

[Elio Maldonado]

Comment on attachment 506416 [details] [diff] [review]
Prefer user's db trust preference over system's db - submitted to fedora

Bob, Aside from removing if #ifdef-ed out code this should be identical to you fix. Intended for TRUNK and the NSS_3_12_BRANCH.

Comment 5

[Robert Relyea]

Comment on attachment 506416 [details] [diff] [review]
Prefer user's db trust preference over system's db - submitted to fedora

r+ for the nsssysinit.c patch.

I really like the other patch, but I can't really r+ it since I wrote it.
Also, We've determined that the trust order is an attribute of the module, not the slot, so both modules will have the same effective trust order in the current code. Fixing this should be a separate bug. The fix would be pretty extensive.

bob

wtc, the review request is an fyi, but a review would be appreciated.
Elio, you are required to review it.:)

patch description:

This patch actually fixes a regression caused by the InitContext patch. We want to be able to change the default internal database in the InitContext case when we initialize NSS in both a library and in the application, where the  library initializes first. The code originally changed the default internal database whenever we loaded a new database through the 'second init' interface where we ask NSS to load softoken again even though it is already loaded. The second time we call the softoken load user database interface. If we were successful, we would mark that database as the default. If we have a default database, the old code would replace it.

The bug happens in nsssysinit when we first initialize the user's database, then we initialize the system database. The root database initialization happens through the second init case and causes the default database to change to the system database. The code now always marks the default database explicitly in the application init case, and only marks the first such database as default.

A future enhancement is to add the ability to mark a slot explicitly as the default database at load time. That would be a separate bug.

bob

Comment 6

[Robert Relyea]

r+ in nsssysinit removal of the dead code. The rest of the patch is also my code, so elio you get to review that one too;).


bob

Comment 7

[Wan-Teh Chang]

Comment on attachment 506416 [details] [diff] [review]
Prefer user's db trust preference over system's db - submitted to fedora

1. Please document pk11_FirstInternalKeySlot.  Also, the
function name should contain a verb.  The current name
sounds like a getter, but it's actually a setter (a no-op
unless it's the first call).

2. With this patch, the pk11_SetInternalKeySlot function
is only used to reset pk11InternalKeySlot to NULL.  So
perhaps we just need a pk11_ClearInternalKeySlot(void)
function.

3. In mozilla/security/nss/lib/sysinit/nsssysinit.c:

>-#define ORDER_FLAGS "trustOrder=75 cipherOrder=100"
>+#define ORDER_FLAGS "cipherOrder=100"

This macro contains only one order flag now.  Perhaps it
should be renamed CIPHER_ORDER_FLAG.

So NSS user database will have trust order 75, and
NSS system database will have trust order 80.  This will
cause NSS to trust the user db before the system db, right?

Does "cipher order" have the same bug?  Or are the user db
and system db guaranteed to have the same cipher
implementation (libsoftokn3.so), so it doesn't matter which
token is searched first?

Comment 8

[Wan-Teh Chang]

Comment on attachment 506416 [details] [diff] [review]
Prefer user's db trust preference over system's db - submitted to fedora

r- for the function name and lack of documentation of
pk11_FirstInternalKeySlot.

Comment 9

[Robert Relyea]

wtc, thanks for the review!

> 1. Please document pk11_FirstInternalKeySlot. 

OK, though it is an internal function (not user callable).

> 2. With this patch, the pk11_SetInternalKeySlot function
> is only used to reset pk11InternalKeySlot to NULL.  So
> perhaps we just need a pk11_ClearInternalKeySlot(void)
> function.

I hope to use this function for the future enhancment describe in comment 4

> This will cause NSS to trust the user db before the system db, right?

yes, if the trust order actually worked on a per slot basis, unfortunately it currently works on a per module basis.

> Does "cipher order" have the same bug?

in some sense yes, though the software crypto slot will be selected before the other two. Only the presence of some key (like the private key) would cause the userdb or system db to be selected, and in that case it would be the slot that has that key that would be selected, 

bob

Comment 10

[Elio Maldonado]

Please rename parse_paramters to parse_parameters.

Comment 11

[Robert Relyea]

Attachment: V2: nsssysinit - make sure userdb is the defualt

Comment 12

[Elio Maldonado]

As stated above, the userdb with trust order 75 wins over systemdb with 80 because the trust order currently works on a per module basis. The module specs for the user's defined PKCS #11 modules are left unspecified on the list returned by get_list. We expect the default to be 100 so they loose.
In softoken the trustOrder is set to 100. 
if (hasRootCerts && !extended) {  trustOrder = 100; }
Good, put elsewhere I see a different deafult value. In pk11pars.h 
#define SECMOD_DEFAULT_TRUST_ORDER 50 
and the code
trustOrderPair=secmod_formatIntPair("trustOrder",trustOrder,
SECMOD_DEFAULT_TRUST_ORDER);
Plus in pk11pars.c
mod->trustOrder  = secmod_argReadLong("trustOrder",nssc,
					SECMOD_DEFAULT_TRUST_ORDER,NULL);
On both calls the 3rd arg gets picked if none is found. 
Could this be a problem?

Comment 13

[Robert Relyea]

The 100 only applies to tokens that provides lists of root certs (like the builtins). They have the special object the causes hasRootCerts to be set.

Normal PKCS #11 modules have higher precedence than the internal module by default. Most PKCS #11 module do not include trust objects (which are NSS specific), so it's a noop. The idea is that smart cards could include trusted roots and inserting the card would also provide automatic trust for those roots.

Anyway this is the way things have been since 2001 or so.

Note, the 75 doesn't win over the 80 in the case we are talking about. Initialization order is what wins currently. This is because they are the same module. The values for the second occurance is discarded.

bob

Comment 14

[Elio Maldonado]

Comment on attachment 506594 [details] [diff] [review]
V2: nsssysinit - make sure userdb is the defualt

pk11_FirstInternalKeySlot coment: change overrided to overriden in the comment
Please address Comment 10 while you are at it.

I happen to find that
if (!pk11InternalKeySlot && slot) { 
	pk11InternalKeySlot = PK11_ReferenceSlot(slot); 
}
makes the intent more evident but that's only a personal preference. The code works as specified and desired. Verification test passed and so did all.sh. With these little changes it's an r+ from me.

Comment 15

[Robert Relyea]

Trunk:

Checking in pk11wrap/pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.31; previous revision: 1.30
done
Checking in pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.14; previous revision: 1.13
done
Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.102; previous revision: 1.101
done
Checking in sysinit/nsssysinit.c;
/cvsroot/mozilla/security/nss/lib/sysinit/nsssysinit.c,v  <--  nsssysinit.c
new revision: 1.4; previous revision: 1.3
done


Branch:

Checking in pk11wrap/pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.30.2.1; previous revision: 1.30
done
Checking in pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.13.2.1; previous revision: 1.13
done
Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.101.2.1; previous revision: 1.101
done
Checking in sysinit/nsssysinit.c;
/cvsroot/mozilla/security/nss/lib/sysinit/nsssysinit.c,v  <--  nsssysinit.c
new revision: 1.2.2.1; previous revision: 1.2
done

Comment 16

[Robert Relyea]

OK, I missed Elio's request to fix some typos... here's the updated checking logs:

Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.103; previous revision: 1.102
done
<parameter spelling is already correct in the trunk>


Branch:

Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.101.2.2; previous revision: 1.101.2.1
done
Checking in sysinit/nsssysinit.c;
/cvsroot/mozilla/security/nss/lib/sysinit/nsssysinit.c,v  <--  nsssysinit.c
new revision: 1.2.2.2; previous revision: 1.2.2.1
done

Comment 17

[Elio Maldonado]

Attachment: effective patch after the two sets of checkins for branch

I have reconstituted the patch after the two batches of Bob's cvs checkins. This is what I actually need to apply downstream in fedora - for the time being.

Comment 18

[Wan-Teh Chang]

Comment on attachment 506594 [details] [diff] [review]
V2: nsssysinit - make sure userdb is the defualt

r=wtc.

Just two nits.

1. I still think the pk11_FirstInternalKeySlot function
name sounds like a "getter" function, a function that
returns the first internal key slot.

It would be nice to add a verb to the function name to
suggest that it is a "setter" function.  Perhaps
pk11_FirstSetInternalKeySlot or
pk11_SetInternalKeySlotIfFirst?

2. In pk11wrap/pk11slot.c

>+ * Set a new default internal keyslot. If one has already been set, clear it.
>+ * passing NULL falls back the NSS normally selected default internal key

Add "on" after "falls back".

Comment 19

[Robert Relyea]

1. Used wtc's suggested name for pk11_SetInternalKeySlotIfFirst.

2. added an appropriate preposition after falls back.

Thanks for the review wtc!

bob




Checking in pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.32; previous revision: 1.31
done
Checking in pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.15; previous revision: 1.14
done
Checking in pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.104; previous revision: 1.103
done


branch:




Checking in lib/pk11wrap/pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.30.2.2; previous revision: 1.30.2.1
done
Checking in lib/pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.13.2.2; previous revision: 1.13.2.1
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.101.2.3; previous revision: 1.101.2.2
done

Comment 20

[Robert Relyea]

Attachment: Fix JSS failures in tinderbox.

Tinderbox is failing because of some JSS tests. When JSS switches into or out of FIPS mode, it then checks to make sure everything worked out ok. This change forces a different default database, but the new code doesn't allow that default database to be changed.

This patch handles the switchover correctly.

Comment 21

[Elio Maldonado]

The JSS tests are sensitive to this change yet our NSS FIPS suite wasn't. I'm trying to understand this better. The JSS CryptoManager uses SECMOD_DeleteInternalModule to switch from FIPS mode and back, as in
http://mxr.mozilla.org/security/source/security/jss/org/mozilla/jss/CryptoManager.c#911
and this is the same technique as modutil uses
http://mxr.mozilla.org/security/source/security/nss/cmd/modutil/pk11.c#55
I think our own NSS FIPS test suite needs some test for this.

Comment 22

[Robert Relyea]

Our own test suite uses NSS existing tools to switch into and out of FIPS mode. The tools just switch and exit (FIPS mode is then tested by other tools).

JSS has additional checks to see if the result is consistent.

There are a few things that JSS checks that aren't in our normal test suite, which is why we run tinderbox. (and why JSS test failures are interesting even if the number of JSS users are small).

This bug will show up if you have a browser and you switch to FIPS mode it the browser and keep running.

bob

Comment 23

[Elio Maldonado]

Comment on attachment 510807 [details] [diff] [review]
Fix JSS failures in tinderbox.

r+ from me, fix a small typo in
>+	    /* if and explicit internal key slot has been set, reset it */
s/and/an/