727204 - Generic blacklisting mechanism is not yet working

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

It was recently said
  "NSS 3.13.x is capable of blocking certificates by issuer DN and serial number
              by adding an appropriate trust record to NSS' builtin module"

It's believed it was made to work with bug 642503.

But I suspect this isn't working yet, I cannot confirm it in my tests (using tstclnt, not using libpkix).


I'm trying to test with a distrust record for the (good) server certificate currently being used at https://kuix.de

I locally added the following distrust record to the roots module:

CKA_CLASS CK_OBJECT_CLASS CKO_NSS_TRUST
CKA_TOKEN CK_BBOOL CK_TRUE
CKA_PRIVATE CK_BBOOL CK_FALSE
CKA_MODIFIABLE CK_BBOOL CK_FALSE
CKA_LABEL UTF8 "kuix.de from startcom"
CKA_CERT_SHA1_HASH MULTILINE_OCTAL
\055\300\175\163\335\227\315\271\341\365\161\020\376\144\206\073
\166\151\125\244
END
CKA_CERT_MD5_HASH MULTILINE_OCTAL
\125\060\055\020\211\355\246\035\116\017\172\175\003\062\325\032
END
CKA_ISSUER MULTILINE_OCTAL
\060\201\214\061\013\060\011\006\003\125\004\006\023\002\111\114
\061\026\060\024\006\003\125\004\012\023\015\123\164\141\162\164
\103\157\155\040\114\164\144\056\061\053\060\051\006\003\125\004
\013\023\042\123\145\143\165\162\145\040\104\151\147\151\164\141
\154\040\103\145\162\164\151\146\151\143\141\164\145\040\123\151
\147\156\151\156\147\061\070\060\066\006\003\125\004\003\023\057
\123\164\141\162\164\103\157\155\040\103\154\141\163\163\040\062
\040\120\162\151\155\141\162\171\040\111\156\164\145\162\155\145
\144\151\141\164\145\040\123\145\162\166\145\162\040\103\101
END
CKA_SERIAL_NUMBER MULTILINE_OCTAL
\002\003\000\222\251
END
CKA_TRUST_SERVER_AUTH CK_TRUST CKT_NSS_NOT_TRUSTED
CKA_TRUST_EMAIL_PROTECTION CK_TRUST CKT_NSS_NOT_TRUSTED
CKA_TRUST_CODE_SIGNING CK_TRUST CKT_NSS_NOT_TRUSTED
CKA_TRUST_STEP_UP_APPROVED CK_BBOOL CK_FALSE

I tried this with SHA1+MD5 hashes (as shown here), and also without both hashes.

I confirmed the string label is contained in the binary module.

I added this builtin module to the NSS database (modutil) that I'm using for testing (with tstclnt).

However, I can connect with tstclnt just fine, no errors reported.

In my test scneario:
- we never hit function nssTrust_IsSafeToIgnoreCertHash
- we hit function cert_CheckLeafTrust, but cert->trust is NULL

I will add additional debugging information as I continue with my analysis.


Please convince me that I'm making a mistake, or please help me to identify the bug in NSS.

Comment 1

[Wan-Teh Chang]

Kai, I ran into this problem but don't know how to fix it
properly.  You can try my proof-of-concept patch in
bug 642503 comment 34.

Comment 2

[Kai Engert (:KaiE:)]

Wan-Teh, thanks a lot. I confirm using that patch my test case works (and cert is rejected).

In my opinion this shows that this bug is valid, and the active distrust code from bug 642503 is not yet working/sufficient.

Comment 3

[Wan-Teh Chang]

We can already explicitly distrust a certificate.  The catch
is that the certificate also needs to be added to nssckbi.
Just adding a trust record for the certificate to nssckbi
is not enough right now.

Note that PSM doesn't have UI to edit a trust record that
doesn't have a corresponding certificate.  So in addition
to completing my proof-of-concept patch, UI work is also
needed.

Comment 4

[Robert Relyea]

Summary: wtc's proof-of-concept patch in bug 642503 comment 34 is essentially correct. I'll attach that patch with some (very) minor improvements.

Longer detail:

The stan code keeps a very strict separation between 'temp certs' and 'perm certs'. When a cert is first imported, we look for the cert in first the temp store, then we do a pkcs #11 search for the cert. either of these yield the cert, we call STAN_GetCertCertificate(), which (if the CERTCertificate isn't cached), will eventually call the fill_CERTCertificateFields function. If we haven't seen the cert before, we set the cert up as a temp certs. Temp certs have an NSSCryptoContext, perm certs have an instance.

We have the case where we haven't seen the cert before. In this case it will have a crypto context. When we search for the Trust with the crypto context, we only look in the hash store. Since we haven't seen the cert before, there is nothing in the hash store.

I looked into doing the lookup in pkistore.c. This becomes complicated because the fill_CERT* function is called before we have added the new cert to the certificate store. The NSSCertificate structure does not store the trust, so we end up calling the trust domain search function twice, once when we look up trust and the cert doesn't exist, and once when we add the cert to the store.

I also looked into storing the trust back when we find it, but, again, we are calling the fill structure before the cert has placed into the cert store.

In the end I've kept wtc's patch with the following changes:
1) I've removed the 'HACK' comments and included comments why the code was necessary and safe, and
2) got the trust domain from the NSSCryptoContext rather than depending on the fact we have a single default trust domain.

bob

Comment 5

[Robert Relyea]

> Note that PSM doesn't have UI to edit a trust record that
> doesn't have a corresponding certificate.  So in addition
> to completing my proof-of-concept patch, UI work is also
> needed.

We already have this issue. We've ran into the problem at RH where it's possible to have trust records without corresponding certificates.

bob

Comment 6

[Robert Relyea]

Attachment: wtc patch with minor changes.

Comment 7

[Robert Relyea]

BTW wtc: we currently have the situation where we don't have the cert we want to distrust.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 597242 [details] [diff] [review]
wtc patch with minor changes.

r-

this patch contains certificate trust records that are simply for testing purposes

I will attached a patch with that stuff stripped

Comment 9

[Kai Engert (:KaiE:)]

Bob, actually I believe that your patch includes additional unrelated things.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v2

I assume that Bob intended to only change file pki3hack.c for the purpose of this bug. Bob, is this subset correct?

I've also fixed a spelling error in the comment.

Comment 11

[Wan-Teh Chang]

Comment on attachment 597242 [details] [diff] [review]
wtc patch with minor changes.

In security/nss/lib/ckfw/builtins/certdata.txt:

>+# distrust kai
>+CKA_CLASS CK_OBJECT_CLASS CKO_NSS_TRUST
>+CKA_TOKEN CK_BBOOL CK_TRUE
>+CKA_PRIVATE CK_BBOOL CK_FALSE
>+CKA_MODIFIABLE CK_BBOOL CK_FALSE
>+CKA_LABEL UTF8 "kuix.de from startcom"
>+CKA_CERT_SHA1_HASH MULTILINE_OCTAL
>+\055\300\175\163\335\227\315\271\341\365\161\020\376\144\206\073
>+\166\151\125\244
>+END
>+CKA_CERT_MD5_HASH MULTILINE_OCTAL
>+\125\060\055\020\211\355\246\035\116\017\172\175\003\062\325\032
>+END

The trust record (for testing) needs to have no CKA_CERT_SHA1_HASH and
CKA_CERT_MD5_HASH values.  This allows you to catch different encodings
of the certificate.  (There are two ways to encode the NULL parameters
in the RSA algorithm identifier.)

Comment 12

[Kai Engert (:KaiE:)]

While I was away during the previous 3 hours, Bob had sent me a private patch for testing, a patch that is completely different.

That email patch was probably an experiment. That patch which was sent by email only didn't fix the bug.

However, I confirm the patch attached to this bug (v2) fixes the bug.

Comment 13

[Wan-Teh Chang]

Comment on attachment 597248 [details] [diff] [review]
Patch v2

r=wtc.

>+	     * c->issuer and c->serial are empty at this point, but
>+	     * nssTrustDomain_FindTrustForCertificate use them to look up
>+	     * up the trust object, so we point them to cc->derIssuer and

I had a duplicate "up" in the comment.  Sorry.

>+	     * Our caller will fill these in with proper arena copies when we
>+	     * return. */
>+	    c->issuer.data = cc->derIssuer.data;
>+	    c->issuer.size = cc->derIssuer.len;
>+	    c->serial.data = cc->serialNumber.data;
>+	    c->serial.size = cc->serialNumber.len;
>+	    nssTrust = nssTrustDomain_FindTrustForCertificate(context->td, c);

As defensive programming, should we reset these four fields to NULL and 0
after the nssTrustDomain_FindTrustForCertificate(context->td, c) call?

Comment 14

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #11)
> The trust record (for testing) needs to have no CKA_CERT_SHA1_HASH and
> CKA_CERT_MD5_HASH values.

I tested without hashes. I confirm the patch v2 works with missing hashes.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: patch v3

addressed wtc's requests

Comment 16

[Robert Relyea]

Comment on attachment 597248 [details] [diff] [review]
Patch v2

f+ rrelyea

Comment 17

[Robert Relyea]

> As defensive programming, should we reset these four fields to NULL and 0
> after the nssTrustDomain_FindTrustForCertificate(context->td, c) call?

I thought of that, but then I thought that we should should also defensively check the state of c->issuer.data and not fill them in if set. In the end I felt the comment was sufficient as the defensive programming might make the code less readable.

bob

Comment 18

[Robert Relyea]

I presume we don't want to slow down 3.13.2 for this patch, has the tag already been cut for it?

bob

Comment 19

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #18)
> I presume we don't want to slow down 3.13.2 for this patch, has the tag
> already been cut for it?

No, but if we include this patch in 3.13.2, we will have to repeat the whole cycle of "test locally", "test in mozilla-central", and wait for results.

I had originally hoped that I would be able to finalize the releases of NSS 3.13.2 and NSPR 4.9 8 hours ago, but I'm still waiting for the NSPR release tag.

I have to leave the house in 11 hours for my flight (it's evening already here).

If we don't release within the next 2 hours, it will be difficult for me to help prior to Monday.

Comment 20

[Kai Engert (:KaiE:)]

The NSS trunk is open for checkins for 3.13.3

I think you have concluded that the changes in my patch v3 are unnecessary.

Please feel free to mark my v3 as obsolete, and check in v2.

Thanks

Comment 21

[Robert Relyea]

Checking in pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.106; previous revision: 1.105
done