642815 - Deal with bogus certs issued by Comodo partner - NSS level fix

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

NSS equivalent of bug 642395

Comment 1

[Kai Engert (:KaiE:)]

In bug 642395, we implemented an emergency patch for Mozilla client applications, that could be shipped as fast as possible.

The patch in bug 642395 has disadvantages:

- it does not fix any other NSS based applications

- it is limited to SSL (although that's apparently sufficient this time)

Now that Mozilla is dealt with, 
and while we are waiting for a more general solution in bug 642503,
this bug proposes:

  Implement an urgent hotfix patch at the NSS level,
  that is equivalent (but general purpose) to the one from bug 642395.


(Should it happen that we deliver an NSS version with a patch from this bug
 into Mozilla, the patch from bug 642395 could be removed from Mozilla
 at the same time.)

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v1

This is my attempt to implement a NSS level blacklist patch.

The tricky part is, I could not find a single point of code where all cert verification passes through.

I have tried to identify the places where the check-for-blacklist must be added.


Alexei, could you please review my changes to the PKIX code?
(which calls into the single blacklist-checker)

Bob, could you please review the other code?

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 520216 [details] [diff] [review]
Patch v1

We might skip this.

Bob has started to work on a patch.

Comment 4

[Robert Relyea]

My patch for this issue should be considerably simplier than the one kai suggested.


I'm just completing my analysis now...

bob

Comment 5

[Robert Relyea]

OK, I was a bit confused. There are two bugs for this issue. Dan has one here: bug 642503 and this one.

This one I'm going to attach a very quick and easy patch. The second will have the analysis for why my patch works and what else needs to go on to be able to generically kill certs.

bob

Comment 6

[Robert Relyea]

Attachment: Long version of the patch to kill bogus end user certs.

Here's the long version. I'm about to test the short version of this patch shortly...

Comment 7

[Robert Relyea]

Here is the processing commands to generate the long patch...

rm certdata.txt
touch certdata.txt
addbuiltin -n "Bogus Mozilla Addons" -t p,p,p < addons.mozilla.org.cer >> certda
ta.txt
addbuiltin -n "Bogus Global Trustee" -t p,p,p < GlobalTrustee.cer >> certdata.tx
t
addbuiltin -n "Bogus GMail" -t p,p,p < mail.google.com.cer >> certdata.txt
addbuiltin -n "Bogus Google" -t p,p,p < www.google.com.cer >> certdata.txt
addbuiltin -n "Bogus Skype" -t p,p,p < login.skype.com.cer >> certdata.txt
addbuiltin -n "Bogus Yahoo 1" -t p,p,p < login.yahoo.com.cer >> certdata.txt
addbuiltin -n "Bogus Yahoo 2" -t p,p,p < login.yahoo.com_2.cer >> certdata.txt
addbuiltin -n "Bogus Yahoo 3" -t p,p,p < login.yahoo.com_3.cer >> certdata.txt
addbuiltin -n "Bogus live.com" -t p,p,p < login.live.com.cer >> certdata.txt
addbuiltin -n "Bogus kuix.de" -t p,p,p < kuix.de.cer >> certdata.txt

Comment 8

[Robert Relyea]

er... I didn't run this in the builtins directory, I just appended the new certdata.txt to the one there. Note that the rm above will of course clobber the builtin certs already there, of course.

bob

Comment 9

[Robert Relyea]

OK, the short patch isn't working. That means it will have to wait until I fix bug 642503, so I'm leaving the long patch here.

bob

Comment 10

[Robert Relyea]

Comment on attachment 520361 [details] [diff] [review]
Long version of the patch to kill bogus end user certs.

I figure Kai should be able to review built-in changes.

If the use of CKT_NETSCAPE_VALID confuses you, see comment 3 in bug Bug 642503 .

bob

Comment 11

[Kai Engert (:KaiE:)]

This patch works. With this patch, we get "sec_error_untrusted_cert".

Unfortunately, this error belongs to the category that we allow users to override and "proceed anyway".

Is this behaviour sufficient for the intention of this bug?
The patch used in bug 642395 produces error "revoked" which users can not override.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 520361 [details] [diff] [review]
Long version of the patch to kill bogus end user certs.

r=kaie because this patch works

Remaining question is, does it sufficiently fix the bug?

Comment 13

[Gervase Markham [:gerv]]

IMO revoked certs, or certs which are specifically untrusted by NSS, should not be overrideable.

Gerv

Comment 14

[Kai Engert (:KaiE:)]

(In reply to comment #13)
> IMO revoked certs, or certs which are specifically untrusted by NSS, should not
> be overrideable.

I tend to agree.

NSS uses this error code when there is an explicit trust record, and the trust record is missing "trusted".


Bob, do you think we should remove SEC_ERROR_UNTRUSTED_CERT from the list of error codes that we allow Mozilla to override?

Comment 15

[Kai Engert (:KaiE:)]

Unfortunately, even if we did comment 14 (I just tested), the error message that users would get, is confusing:

"Peer's certificate has been marked as not trusted by the user."

(This is the default error message for that error code.)


Could we invent a new trust flag, that marks the certs as revoked, so that NSS verification code sets the "is revoked" error code?

Comment 16

[Robert Relyea]

The override issue is operating as I would have predicted.

There really 2 types of override here:

1) The user's own certdb can  override.
2) PSM can override.

Both can happen with this patch. For now I would keep kai's other patch in place. This patch will cover other users of NSS. (including plugins).

When I work on the full general patch, we can discuss how these over rides should occur.

In 2) PSM. We can fix this by changing the error code to revoked in this case (I don't think we need another bit. We know we are rejecting the chain because it has explicitly been marked as untrusted).

In 1) we may need to have a second builtins slot (No need to change the module). That second slot would have a smaller trust order number (and thus higher precedence) than the user's database. This change would be a bit more trickey since trust order is on a module basis, not a slot basis ;(.

bob

Comment 17

[Kai Engert (:KaiE:)]

(In reply to comment #16)
> The override issue is operating as I would have predicted.

I understand you're saying:
You have expected that Firefox will allow to override.


> There really 2 types of override here:
> 
> 1) The user's own certdb can override.
> 2) PSM can override.
> 
> Both can happen with this patch.

(1) is probably a theoretical issue, 
    because most users touch NSS certdb using PSM, only
    (or similar software)


> For now I would keep kai's other patch in place. 
> This patch will cover other users of NSS. (including plugins).

I'm confused.

I made two patches:

(a) a PSM level patch, that only covers SSL, and only covers
    Mozilla applications.
    That patch is shipped in Firefox.
    That patch does NOT cover other users of NSS (i.e. NOT Evolution)

(b) an equivalent hack patch, attached to this bug, obsolete 
    attachment 520216 [details] [diff] [review].
    This one, if shipped with NSS, would cover any NSS users,
    including Evolution.


The only patch that covers other users of NSS is (b).
Do you suggest we should ship (b)


> When I work on the full general patch, we can discuss how these over rides
> should occur.

I thought your "current patch" == "the general patch".
Sorry, I probably have misunderstood.

Which patch would you prefer to shipp in NSS with Linux distributions?
My patch (b) attachment 520216 [details] [diff] [review]
or (c) your current patch attachment 520361 [details] [diff] [review]
?


> In 2) PSM. We can fix this by changing the error code to revoked in this case
> (I don't think we need another bit. We know we are rejecting the chain because
> it has explicitly been marked as untrusted).

I don't understand. I thought our plan is to change the error code that is returned by NSS' certificate verification functions. How can we do that in PSM?


> In 1) we may need to have a second builtins slot (No need to change the
> module). That second slot would have a smaller trust order number (and thus
> higher precedence) than the user's database. This change would be a bit more
> trickey since trust order is on a module basis, not a slot basis ;(.

Sorry, I can't follow. I think you're explaining tricks that you want to use in the "full general patch" that you intend to write at a later time.

Comment 18

[Robert Relyea]

> (1) is probably a theoretical issue, 
>     because most users touch NSS certdb using PSM, only
>     (or similar software)

I'm thinking the even more general case. If PSM stored one of these certs with any trust values at all, then that value would override the builtins.

> I made two patches:

I mean keep (a). (b) is DOA a this point. My current patch in this bug should cover Evolution sufficiently.

> I thought your "current patch" == "the general patch".
> Sorry, I probably have misunderstood.

No see bug 642503. This is the fully general solution with all the bells and whistles. This match is just a 'general' one that works with other NSS clients and can be picked up quickly by NSS distributions if necessary. In this case I'm less worried about overrides. Most applications don't override, and if they do, it's not as sophisticated as PSM.

bob

Comment 19

[Kai Engert (:KaiE:)]

Ok, thanks for clarification.

Comment 21

[Kai Engert (:KaiE:)]

NSS trunk:

Checking in cmd/addbuiltin/addbuiltin.c;
/cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v  <--  addbuiltin.c
new revision: 1.15; previous revision: 1.14
done
Checking in lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.74; previous revision: 1.73
done
Checking in lib/ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.71; previous revision: 1.70
done
Checking in lib/ckfw/builtins/nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.27; previous revision: 1.26
done


NSS 3.12 branch:

Checking in mozilla/security/nss/cmd/addbuiltin/addbuiltin.c;
/cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v  <--  addbuiltin.c
new revision: 1.14.68.1; previous revision: 1.14
done
Checking in mozilla/security/nss/lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.67.2.7; previous revision: 1.67.2.6
done
Checking in mozilla/security/nss/lib/ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.64.2.7; previous revision: 1.64.2.6
done
Checking in mozilla/security/nss/lib/ckfw/builtins/nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.24.2.3; previous revision: 1.24.2.2
done