Closed
Bug 646460
Opened 13 years ago
Closed 13 years ago
Don't allow to override after treating certificates as revoked
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
People
(Reporter: KaiE, Assigned: KaiE)
References
Details
(Whiteboard: [qa-examined-192] [qa-needs-STR])
Attachments
(1 file, 3 obsolete files)
938 bytes,
patch
|
briansmith
:
review+
sayrer
:
approval-mozilla-aurora+
dveditz
:
approval2.0+
dveditz
:
approval1.9.2.18+
dveditz
:
approval1.9.1.20+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Attachment #523002 -
Flags: review?(bsmith)
Comment 2•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 7•13 years ago
|
||
> 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.
Assignee | ||
Comment 8•13 years ago
|
||
>
> > 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.
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #523002 -
Attachment is obsolete: true
Attachment #523002 -
Flags: review?(bsmith)
Attachment #525152 -
Flags: review?(bsmith)
Assignee | ||
Updated•13 years ago
|
Attachment #525152 -
Flags: approval1.9.2.17?
Attachment #525152 -
Flags: approval1.9.1.19?
Assignee | ||
Comment 10•13 years ago
|
||
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!
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #525152 -
Attachment is obsolete: true
Attachment #525152 -
Flags: review?(bsmith)
Attachment #525152 -
Flags: approval1.9.2.17?
Attachment #525152 -
Flags: approval1.9.1.19?
Attachment #525154 -
Flags: review?(bsmith)
Attachment #525154 -
Flags: approval1.9.2.17?
Attachment #525154 -
Flags: approval1.9.1.19?
Assignee | ||
Comment 12•13 years ago
|
||
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
Updated•13 years ago
|
blocking1.9.1: --- → .20+
blocking1.9.2: --- → .18+
blocking2.0: --- → ?
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Comment 13•13 years ago
|
||
Comment on attachment 525154 [details] [diff] [review] Patch v1 with comment fixed (for real this time) Waiting for review before looking at approval requests
Attachment #525154 -
Flags: approval1.9.2.17?
Attachment #525154 -
Flags: approval1.9.1.19?
Updated•13 years ago
|
status-firefox5:
--- → affected
tracking-firefox5:
--- → ?
Comment 14•13 years ago
|
||
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•13 years ago
|
||
Dan, can you tell us what this gets us and what the risks are?
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
Assignee: nobody → kaie
Attachment #525154 -
Attachment is obsolete: true
Attachment #525154 -
Flags: review?(bsmith)
Attachment #527348 -
Flags: review?(bsmith)
Comment 18•13 years ago
|
||
Comment on attachment 527348 [details] [diff] [review] Patch v3 - more and better comments r+
Attachment #527348 -
Flags: review?(bsmith) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #527348 -
Flags: approval2.0?
Attachment #527348 -
Flags: approval-mozilla-aurora?
Comment 19•13 years ago
|
||
> 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•13 years ago
|
||
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•13 years ago
|
||
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.
Attachment #527348 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 22•13 years ago
|
||
trunk = mozilla-central http://hg.mozilla.org/mozilla-central/rev/5b8ade677818 aurora http://hg.mozilla.org/releases/mozilla-aurora/rev/6e14d840b803
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•13 years ago
|
||
I think a checkin to mozilla-aurora means: I must set status-firefox5 to fixed.
Assignee | ||
Comment 24•13 years ago
|
||
backed out from trunk (no problem with this patch, just backed out everything I did today). will reland soon
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 25•13 years ago
|
||
Kai, it looks like you didn't backout this one from mozilla-aurora
Assignee | ||
Comment 26•13 years ago
|
||
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.
Assignee | ||
Comment 27•13 years ago
|
||
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.
Attachment #527348 -
Flags: approval1.9.2.18?
Attachment #527348 -
Flags: approval1.9.1.20?
Assignee | ||
Comment 28•13 years ago
|
||
(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
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 29•13 years ago
|
||
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.
Attachment #527348 -
Flags: approval2.0?
Attachment #527348 -
Flags: approval1.9.2.18?
Attachment #527348 -
Flags: approval1.9.2.18+
Attachment #527348 -
Flags: approval1.9.1.20?
Attachment #527348 -
Flags: approval1.9.1.20+
Assignee | ||
Comment 30•13 years ago
|
||
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•13 years ago
|
||
Per security group discussion, requesting landing on mozilla-2.0.
Assignee | ||
Comment 32•13 years ago
|
||
Comment on attachment 527348 [details] [diff] [review] Patch v3 - more and better comments I think Cameron intended to set the approval2.0 ? flag
Attachment #527348 -
Flags: approval2.0?
Comment 33•13 years ago
|
||
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"
Attachment #527348 -
Flags: approval2.0? → approval2.0+
Updated•13 years ago
|
blocking2.0: ? → -
Comment 34•13 years ago
|
||
Kai, is there a good STR for this? Do you have a site that can be used for testing with a cert for this?
Whiteboard: [qa-examined-192] [qa-needs-STR]
Comment 35•13 years ago
|
||
Al, I think we should be able to write an automated test for this. Re-opening because we don't have the test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 36•13 years ago
|
||
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
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•