Closed Bug 1122642 Opened 9 years ago Closed 8 years ago

[h2] better error state for INADEQUATE_SECURITY

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

()

Details

(Whiteboard: [spdy][necko-active])

Attachments

(2 files, 2 obsolete files)

See bug 1121706 comment 29 - when we decide to send INADEQUATE_SECURITY because a server chose h2 when it shouldn't have, we should do better than just stopping the page load with no feedback. Let's add a net error page for that.
We might also want to add one for when we receive INADEQUATE_SECURITY (assuming that ever happens)
Attached patch patch (obsolete) — Splinter Review
Here's a patch that adds the appropriate neterror page. There is one try failure at https://treeherder.mozilla.org/#/jobs?repo=try&revision=650fead5d966&selectedJob=15041309 that looks like it *could* be related, so I'll dig more to figure that out. Also, I have to assume that the strings here are totally not what will be in the actual product, I'll have to get someone (UX?) to look them over and change them entirely.
Hi Aislinn & Philipp - Dolske said you would be the ones to look over/assist with wording for the strings on the patch. Some background on INADEQUATE_SECURITY, so you know what we're talking about here. The h2 spec (RFC 7540 section 9.2 - https://httpwg.github.io/specs/rfc7540.html#TLSUsage) limits the TLS configurations that can be used when speaking http/2. Right now, when we run into a situation where we have to close down an http/2 connection due to INADEQUATE_SECURITY, firefox just stops loading the page and shows whatever was in the viewport before - not exactly helpful (see the duplicates to this bug). So I'm adding a neterror page here to explain why we were unable to load the page - we need to give admins/server authors enough info to figure out the issue without overwhelming with spec language :) Thanks!
Flags: needinfo?(philipp)
Flags: needinfo?(agrigas)
Hey Nick, could you post a screen shot of the page so that I can see the strings in context? Thanks!
Flags: needinfo?(philipp)
Flags: needinfo?(hurley)
Flags: needinfo?(agrigas)
Attached image original strings (obsolete) —
Here are the strings as currently in the patch - certainly not great, probably not even good.
Flags: needinfo?(hurley)
Forgot to ni? when I uploaded the screenshot
Flags: needinfo?(philipp)
Thanks! I'm redirecting this to Bram who has been working on error page copy for a while now. I think he already had copy for a similar case that we could probably re-use.
Flags: needinfo?(philipp) → needinfo?(bram)
Would it be accurate to say something like this?

[URL] uses security technology that is outdated and vulnerable to attack. An attacker could easily reveal information which you thought to be safe. The website administrator will need to fix the server first before you can visit this website.

Error code: [ERR_CODE]
Flags: needinfo?(bram)
I think that seems pretty accurate, yeah. I could quibble with some of the specifics, but not enough to make it worth our while :) I'll get the patch updated so we can move this forward.
Whiteboard: [spdy] → [spdy][necko-active]
Well, that took way longer than hoped (too many distractions). Here's a screenshot of the new strings. Bram, does this look good to you?
Attachment #8705303 - Attachment is obsolete: true
Flags: needinfo?(bram)
Attached patch patchSplinter Review
Newest version of the patch, updated with the new strings.
Attachment #8704329 - Attachment is obsolete: true
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #13)
> Created attachment 8741090 [details]
> Screenshot 2016-04-13 12.33.30.png

This looks good to me.
Flags: needinfo?(bram)
Comment on attachment 8741098 [details] [diff] [review]
patch

Pat - looking for review from you on the netwerk bits
Attachment #8741098 - Flags: review?(mcmanus)
Comment on attachment 8741098 [details] [diff] [review]
patch

Boris - looking for review on the non-netwerk bits (though you're free to forward the review to someone else and/or look over the netwerk bits, as well)
Attachment #8741098 - Flags: review?(bzbarsky)
Comment on attachment 8741098 [details] [diff] [review]
patch

Review of attachment 8741098 [details] [diff] [review]:
-----------------------------------------------------------------

thanks
Attachment #8741098 - Flags: review?(mcmanus) → review+
Comment on attachment 8741098 [details] [diff] [review]
patch

I think "h2" in the various comments needs to be replaced with something that will mean the right thing to people not on the networking team.  I'm assuming it does not mean HTML <h2>, but that's the default operating assumption...

I assume it should be "http/2" or something like that.

What's the resulting UX after this change?  Is the user able to override?  Have you had someone responsible for Firefox UX look this over?

r=me modulo that.
Attachment #8741098 - Flags: review?(bzbarsky) → review+
Thanks!

Boris - you're right, I should make it more explicitly http/2 - will do that before landing.

The UX is in the screenshot also attached to the bug - there is no override ability here, INADEQUATE_SECURITY is a hard fail at the protocol level. Bram has already gone over the strings & UX and indicated in comment 15 that all was good.

Given all that, I'll get the h2->http/2 changes made, and get this landed.
Could I propose that the error message say something like this instead?

HTTP/2 error code: INADEQUATE_SECURITY

To me, the current proposal does not make it clear enough that this is a protocol error and not some sort of internal Firefox-specific error. The inclusion of the NS_ERROR_NET_ part in particular seems unnecessary, and will confuse those who are not sufficiently familiar with HTTP/2.
https://hg.mozilla.org/mozilla-central/rev/1f5f16c88c5b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Could you please add some more information to the error message? In particular the problem with the current one is that Firefox chooses to generate a connection error if the server says it supports blacklisted cipher suites (see https://tools.ietf.org/html/rfc7540#section-9.2.2), but other browsers might not (Chrome doesn't) and ssllabs.com's ssltest reports an A+ grade for a website that wasn't loading in Firefox (it does reveal the actual problem in the handshake simulation, buried further down on the results page, but I did not notice that until after I figured it out).

I was only able to figure this out because I found this issue, looked at the code, and then googled for http 2 INADEQUATE_SECURITY, and found and read rfc 7540. Googling for NS_ERROR_NET_INADEQUATE_SECURITY, the error Firefox displays, did not yield any useful results.

E.g. a "more info" button that lists the blacklisted ciphers that were detected. This would make debugging these problems much easier.
Nikola, it's usually better to file a new bug on still-unresolved problems instead of commenting in an existng resolved bug, because resolved bugs are generally more or less ignored...  I've filed bug 1294708 for your issue.
Thanks
You need to log in before you can comment on or make changes to this bug.