10.57 KB, patch
Robert Relyea: superreview+
Samuel Sidler (old account; do not CC): approval18.104.22.168+
|Details | Diff | Splinter Review|
25.64 KB, image/png
26.22 KB, image/png
10.43 KB, patch
Alexander Sack: approval1.8.0.next+
|Details | Diff | Splinter Review|
There appears to be a vulnerability in the way that the peer trust model interacts with alternative subject names. The issue is that NSS only has a single bit which trusts a cert for all of its subject names, not just the name the user intended to trust the cert for. An example would come from an https site with a malicious self-signed cert. despot.mozilla.org would be a good example of a good launch site--someplace where users would not find a self-signed cert to be unusual. The cert would not only contain its own, legitimate, domain, but would include an alternate name of a site the attacker wishes to spoof. The user accepts the cert for purposes of authenticating the legitimate domain, but PSM has NSS mark the cert as trusted for domains that were never mentioned to the user. This affects all peer-trust areas, SSL, S/MIME, and code signing. Bug 230655 at least gives the user an way to see what the domains are, without having to decode ASN.1 in their head. At a minimum, PSM would need to list all of the alternative names before giving the user the opportunity to mark the cert as peer-trusted--this is related to bug 238142. Better would be to have, instead of a peer-trust bit on the cert, a list of names for which the cert is peer-trusted. This would require either an extensive change to NSS or for slightly more extensive changes to PSM to take over the peer-trust-database functions of NSS.
Sounds like the problem is that PSM asks the user to trust the cert without giving the user all the relevant info that the user should consider before making that decision. There is no other PKI product in the world (known to me) that allows the user to trust some of the names in a cert but not others. I object to making the trust mode MORE complicated. The user should either trust the cert entirely, or not at all. But the user SHOULD see all the relevant info before making that decision. SO, this seems like a UI problem, not a trust DB problem.
*** Bug 308244 has been marked as a duplicate of this bug. ***
OK, let me summarize: * This is a sec issue, and not a minor one * It is known since 04/2004 (at least) * It is marked "Security-Sensitive", thus not public. * Once a year somebody (like me) rediscovers it, doesn't find it in bugzilla, reports it, gets a "oh we know that, it' a dupe" * It is WONTFIX for mozilla-1.8/firefox-2 (bug #402347) OMG. PS: Someone opened the last dupe, so you can open this one, too. Then it won't get rediscovered and duped over and over again the next three years.
M. Zalewski explained better than i ever could why this should get fixed in 1.8.x, too: <http://www.securityfocus.com/archive/1/483942/30/0/threaded>
I'd like to access bug #406655 this one depends on, is it possible to put me on its CC list?
We're clearly not going to get a patch for this before we want to ship 22.214.171.124, but I don't think waiting for FF3 is all that great an approach. Try for next branch release.
It appears to me that this bug is a duplicate of bug 238142, which is open (not security sensitive). Bug 238142 has been marked as a duplicate of bug 398718 (although it isn't obvious to me that they *are* duplicates), and Bug 398718 is now resolved fixed, even though this situation with subject alt names persists (AFAICT) on branch and trunk. I don't see how this bug, or the others named above, can be resolved (especially fixed) while this problem persists. It seems the REAL work of solving the subject alt name bug has taken place or will take place in bug 400917 & Bug 411246. So, I'm marking this bug as depending on 411246.
(In reply to comment #8) > > It seems the REAL work of solving the subject alt name bug has taken place > or will take place in bug 400917 & Bug 411246. No. The "subject alt name bug" is already fixed in FF 3, because in FF 3: - on error pages we list all valid DNS names - we bind each override strictly to a single hostname and pair The single purpose of bug 411246 is to fix a small difference between display code and engine verification code. This bug is asking for a solution for FF 2.x, and I'm not yet aware of one.
I would hope that when a host name mismatch error occurs in FF3, it displays diagnostic info that includes the name it was looking for and the names it found. Is that not true for FF3?
(In reply to comment #10) > I would hope that when a host name mismatch error occurs in FF3, it displays > diagnostic info that includes the name it was looking for and the names it > found. Is that not true for FF3? Nelson, I don't understand why you ask this question, given my comment 9. Maybe my comment 9 is confusing? When I use FF 3 beta to connect to a mismatch host (paypal.com) I get an error page with this text: Secure Connection Failed paypal.com uses an invalid security certificate. The certificate is only valid for www.paypal.com. (Error code: ssl_error_bad_cert_domain)
In reply to comment 11, The error page text shown above does not state the DNS name that the browser was trying to find inside the certificate. Sometimes users are confused about the name of the host in the url, e.g. for URLs like this one: https://firstname.lastname@example.org/ I think the dialog should explain not only what names it found in the cert, but also the name it was trying to find there (but did not).
(In reply to comment #12) > The error page text shown above does not state the DNS name that the browser > was trying to find Wrong, I think it does!!! The error message says paypal.com uses an invalid security certificate. This shows the hostname that we were looking for in the certificate.
Hmmm. Perhaps you're right. Perhaps it needs to state more clearly that the name it expected to find was (in this example) paypal.com. But I agree that the string is there, if one knows how to interpret it.
Why are you guys arguing about FF3, isn't this a branch-only bug now? I thought the trunk mechanism tied the cert to the single host that we got the cert from so this is no longer an issue. On branch (ff2 and nss 3.11.x) we still have a spoofing problem. Would a "fix" be as easy as listing the alt names in the approval dialog? I know we're not going to be taking NSS 3.12 and all the improved support it has on branch any time soon.
ditto comment 7, moving out yet another release :-(
Is there any realistic way to fix this on the Mozilla1.8 branch? It seems like our choices are to back-port enough of the trunk mechanism that makes the cert-site binding, or easier-but-worse list all the alt names in the initial dialog ("you are also agreeing that this cert is valid for the following sites"). The latter is a localization change, of course, so that doesn't really fly either. I'm guessing this is a WONTFIX on the branch. :-(
Ok, here is an idea that does not require string changes. The current error message always displays the cert's common name, like: "... the security certificate presented belongs to ### ..." Instead of always displaying a single domain name, we could extract all names from the cert (using code we added on trunk) and display it like this: "... the security certificate presented belongs to (site1, site2, site3) ..." I'm trying to produce a patch.
Kai, I like that suggestion (displaying all the names for which it is thought to be valid). Since those "names" include IP addresses (both IPv4 and IPv6), let me suggest that you also plan to include any and all of those in that list of "names" in that error message.
(In reply to comment #19) > Kai, I like that suggestion (displaying all the names for which it is > thought to be valid). Since those "names" include IP addresses (both > IPv4 and IPv6), let me suggest that you also plan to include any and > all of those in that list of "names" in that error message. The code we implemented, and which is checked in on trunk, handles both IP addresses and DNS names. I plan to use an exact copy of that code, so you request is addressed.
Created attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] This patch works for me.
Created attachment 317788 [details] screen shot This screen shot illustrates the new UI. You get it when you attempt to connect to https://126.96.36.199/
Comment on attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] Note that function GetSubjectAltNames is an exact copy of what we have on trunk, and is therefore reviewed already. http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSIOLayer.cpp#892
Comment on attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] Nice! Does this also fix the "You have attempted to connect to www.foo.com, however the certificate belongs to www.foo.com" bugs? That's a nice side-effect. >+// names => a single name or a list of names >+// multipleNames => whether multiple names were delivered comment nit: this is "nameCount" now rather than a boolean. The .idl needed the comment more than here, I think (if you're only going to pick one place). Looks great, thanks for the patch! r=dveditz
(In reply to comment #25) > Nice! Does this also fix the "You have attempted to connect to www.foo.com, > however the certificate belongs to www.foo.com" bugs? That's a nice > side-effect. Yes, it does :-)
Comment on attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] Approved for 188.8.131.52. a=ss for release-drivers. Leaving nomination for 184.108.40.206 to the 1.8.0 drivers.
Comment on attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] Note that approval for this patch does not make sense for 1.8.0 branch, because the patch does not apply. At least the patch will have to get merged, not sure if more work is required.
Comment on attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] I think it would be good if Bob could r+ this bug. I have already talked to him shortly, he agrees with the plan. Since function GetSubjectAltNames in this patch is being copied from the trunk, you can skip reading it.
Comment on attachment 317786 [details] [diff] [review] Patch v1 [for 1.8 branch] did not review the details of the 'GetSubjectAltNames' function except to verify semantic assumptions fo the code that called it.
checked in to 1.8 branch for Firefox 220.127.116.11
Created attachment 319665 [details] [diff] [review] Patch v1 + bustage fix, merged to 1.8.0 branch If you want to fix 1.8.0 branch for Firefox 1.5.0.x, too, this patch makes it work for me.
Comment on attachment 319665 [details] [diff] [review] Patch v1 + bustage fix, merged to 1.8.0 branch a=asac for 1.8.0 branch Please commit.
checked in to 1.8.0 branch marking fixed
Kai, I'm looking at the UI on Fx20014 and Fx20015pre while visiting https://18.104.22.168/ and it looks the same. Should it be so? When I visit https://22.214.171.124/ I can see the difference.
Juan, I get the expected UI on both sites. Please describe more what exactly you are getting and why you think it's wrong (or a screenshot).
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:126.96.36.199pre) Gecko/2008061004 BonEcho/188.8.131.52pre Never mind. I needed to OK to accept the certificate before I could see the difference when visiting https://184.108.40.206/ I was expecting the first modal dialog to show me the difference. Trunk doesn't use the modal dialog and you see it right away. Changing the keyword to verified220.127.116.11