646460 - Don't allow to override after treating certificates as revoked

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

Bug 642395 added code which decides the application level (PSM) that a cert is revoked.

The NSS SSL code does not expect this error code, and decides to call into the application again, asking how to handle the error.

PSM's bad cert handler checks for the errors returned by NSS, which says "not trusted", and concludes, we should allow to override.


The fix is:
In the beginning of the bad-cert-handler, check whether the error status is "revoked". If it is, avoid any override handling, but exit the bad-cert-handler immediately.


This was found when using code from bug 642815, which blacklists certs at the NSS level.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Brian, we had previously attempted to fix a similar issue in bug 642395 comment 36 to 40.

Here is a fix that should work with both scenarios.

Calling cancel_and_failure is not necessary, and would be wrong, because it suppresses error reporting.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #1)
> Calling cancel_and_failure is not necessary, and would be wrong,
> [for errors other than the ones explicitly handled below,] 
> because it suppresses error reporting.

Please add this comment to top of the function so that we don't make the mistake we made in https://bugzilla.mozilla.org/attachment.cgi?id=520017&action=diff again. r+

Comment 3

[Daniel Veditz [:dveditz]]

I wonder if we should store a list of certs/fingerprints we've found which have been positively revoked. The user visited that site once and might try again, and next time might be on a network where revocation checking is blocked. Is there ever a legitimate case where a cert is revoked and then un-revoked? Maybe a revocation by accident? Even then it may be better to issue a replacement cert.

I suppose this is completely off-topic for this bug. When I first read the summary this is what I thought you were talking about.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #3)
> I wonder if we should store a list of certs/fingerprints we've found
> which have been positively revoked. ... I suppose this is completely
> off-topic for this bug. When I first read the summary this is what
> I thought you were talking about.

That's not what this bug report is about, but it is a good idea. I have incorporated it into my notes regarding the proposed improvements to revocation checking.

Comment 5

[Robert Relyea]

Dan,

There are 2 types of 'revocation' statuses:

Revoked.
On Hold.

The latter can be turned off (it's used in cases where someone has lost their token and then latter safely recovered it). 

On Hold is usually marked in an extension (the normal status is revoked for both). NSS currently does not parse that extension. The use of on hold is exceedingly rare in all but the token scenario.

bob

Comment 6

[Wan-Teh Chang]

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

> static SECStatus
> nsNSSBadCertHandler(void *arg, PRFileDesc *sslSocket)
> {
>+  // if other code decided that cert was revoked, don't do anything else
>+  if (PR_GetError() == SEC_ERROR_REVOKED_CERTIFICATE)
>+    return SECFailure;

Nit: replace "other code" by "AuthCertificateCallback".
I would simply remove "other code decided that".

You should also remove SEC_ERROR_UNTRUSTED_ISSUER from
the switch statement in nsNSSBadCertHandler:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp?mark=3429#3425

Comment 7

[Kai Engert (:KaiE:)]

> Nit: replace "other code" by "AuthCertificateCallback".
> I would simply remove "other code decided that".

Ok.


> You should also remove SEC_ERROR_UNTRUSTED_ISSUER from
> the switch statement in nsNSSBadCertHandler:
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp?mark=3429#3425


You are proposing a semantic change, but in my opinion this semantic changes is not apprproiate for stable branches.

Given that I would like this fix on the stable branch, I would like to discuss this potential change elsewhere.

Comment 8

[Kai Engert (:KaiE:)]

> 
> > You should also remove SEC_ERROR_UNTRUSTED_ISSUER from
> > the switch statement in nsNSSBadCertHandler:
> > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp?mark=3429#3425
> 
> 
> You are proposing a semantic change, but in my opinion this semantic changes is
> not apprproiate for stable branches.
> 
> Given that I would like this fix on the stable branch, I would like to discuss
> this potential change elsewhere.


And I'm not convinced this is the right change, because a comment in NSS claims, error code SEC_ERROR_UNTRUSTED_ISSUER can be seen with self signed certs, and we do want to allow people to override that.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v1 with comment fixed

Comment 10

[Kai Engert (:KaiE:)]

It would be great if we could get this r+ and approved today,
in order to be in time for 3.6.17,
because Fedora would like to ship an updated NSS that blacklists the roots for all applications,
but Firefox needs this patch to make sure users won't be able to override.
Thanks!

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Patch v1 with comment fixed (for real this time)

Comment 12

[Kai Engert (:KaiE:)]

Brian, I think you might have confused SEC_ERROR_UNTRUSTED_CERT with SEC_ERROR_UNTRUSTED_ISSUER.

I'm OK to remove SEC_ERROR_UNTRUSTED_CERT from mozilla-central, but I propose to do that in another bug, as this patch here should land into 4.0.x

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 525154 [details] [diff] [review]
Patch v1 with comment fixed (for real this time)

Waiting for review before looking at approval requests

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

My understanding (from email with Kai) is that Kai will upload another patch with the comment fixed "really for real". (see comment 2) ;)

Comment 15

[Asa Dotzler [:asa]]

Dan, can you tell us what this gets us and what the risks are?

Comment 16

[Kai Engert (:KaiE:)]

Asa, this patch is helpful when Firefox is running with blacklisted sources from two different sources.

The first source is the Firefox builtin blacklist.
The second source is an NSS builtin blacklist.

Linux distributions want to ship an NSS with a builtin blacklist, in order to allow other NSS-based applications to benefit from the blacklist, too.

Unfortunately, the NSS based blacklist gives a weaker signal, it only says "untrusted", because right now, NSS is not yet able to embed "cert is revoked".

Unfortunately, Firefox/PSM has a bug.

When both blacklist sources are available, the NSS decision wins. Firefox will say "cert is untrusted".

This is unfortunate, because we really want it to say "revoked", not allowing a user to override.


The patch here fixes that bug.

The patch here ensures, that when both blacklists are available, the "revoked" status wins, and the other source is not able to weaken that decision.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Patch v3 - more and better comments

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 527348 [details] [diff] [review]
Patch v3 - more and better comments

r+

Comment 19

[Robert Relyea]

> When both blacklist sources are available, the NSS decision wins. Firefox will
> say "cert is untrusted".

Also, this error is subtly different from the error you get if you just don't chain to a trusted cert (like the case of self-signed certs, or a cert chaining to an unknown issuer). The latter 2 cases are still overridable.

This only the case were a leaf cert (soon to include any type of cert) has been explicitly marked as 'do not trust'.

bob

Comment 20

[JP Rosevear [:jpr]]

I think 6 extra weeks delay here will not make a big difference.  A better approach in the short term is to loop in the linux distros so they pick up this patch for all their releases.  Need debian and ubuntu contacts too if someone knows them.

Comment 21

[Robert Sayre]

Comment on attachment 527348 [details] [diff] [review]
Patch v3 - more and better comments

We're going to take this on Aurora, since it might land on 3.5 and 3.6, and we don't want to regress users that upgrade. The patch is also very small.

Comment 22

[Kai Engert (:KaiE:)]

trunk = mozilla-central
http://hg.mozilla.org/mozilla-central/rev/5b8ade677818

aurora
http://hg.mozilla.org/releases/mozilla-aurora/rev/6e14d840b803

Comment 23

[Kai Engert (:KaiE:)]

I think a checkin to mozilla-aurora means: I must set status-firefox5 to fixed.

Comment 24

[Kai Engert (:KaiE:)]

backed out from trunk (no problem with this patch, just backed out everything I did today). will reland soon

Comment 25

[Peter van der Woude [:Peter6]]

Kai, it looks like you didn't backout this one from mozilla-aurora

Comment 26

[Kai Engert (:KaiE:)]

yes. it worked on aurora, I didn't see a need to backout there. The reason for my backout wasn't related to this bug, just some other things broke that I happened to land at the same time to mozilla-central.

Comment 27

[Kai Engert (:KaiE:)]

Comment on attachment 527348 [details] [diff] [review]
Patch v3 - more and better comments

Dan explained to me that blocking+ is not sufficient to land on 1.9.1/1.9.2 branches.

Requesting explicit approval for those branches.

Comment 28

[Kai Engert (:KaiE:)]

(In reply to comment #24)
> backed out from trunk (no problem with this patch, just backed out everything I
> did today). will reland soon

relanded 

http://hg.mozilla.org/mozilla-central/rev/92d5e760e0c0

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 527348 [details] [diff] [review]
Patch v3 - more and better comments

Approved for 1.9.2.18 and 1.9.1.20, a=dveditz for release-drivers

Mozilla is not planning on releasing a 3.5.20 but if you want to land on the 1.9.1 branch you've got permission to do so.

Clearing the mozilla-2.0 request as we don't plan any further releases from that branchlet.

Comment 30

[Kai Engert (:KaiE:)]

1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44d164c30861

1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ce0dd43871e2

Comment 31

[Cameron Kaiser [:spectre]]

Per security group discussion, requesting landing on mozilla-2.0.

Comment 32

[Kai Engert (:KaiE:)]

Comment on attachment 527348 [details] [diff] [review]
Patch v3 - more and better comments

I think Cameron intended to set the approval2.0 ? flag

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 527348 [details] [diff] [review]
Patch v3 - more and better comments

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

When landed please add a link to the changeset and flip the status2.0 field to ".x-fixed"

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Kai, is there a good STR for this? Do you have a site that can be used for testing with a cert for this?

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Al, I think we should be able to write an automated test for this. Re-opening because we don't have the test.

Comment 36

[Kai Engert (:KaiE:)]

I would prefer to keep bugs closed for issued that are clearly solved.
Keeping solved issues open often causes confusion.
Please open a separate bug for getting a test added, if you think it's necessary.
(I'm not convinced it's necessary.)

The issue is difficult to reproduce, because if requires a special mix of Firefox and NSS.

Mozilla has never shipped a mix that shows the issue.

On Fedora we have prevented the issue by applying a fix locally.

So in order to reproduce the failure, you would have to artifically create a broken environment, by e.g. using a Firefox lacking this fix, combining it with a NSS 3.12.0