[h2] better error state for INADEQUATE_SECURITY

RESOLVED FIXED in Firefox 48

Status

()

Core
Networking: HTTP
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nwgh, Assigned: nwgh)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
We might also want to add one for when we receive INADEQUATE_SECURITY (assuming that ever happens)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1203541
Duplicate of this bug: 1214278
(Assignee)

Comment 4

2 years ago
Created attachment 8704329 [details] [diff] [review]
patch

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.
(Assignee)

Comment 5

2 years ago
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)
(Assignee)

Comment 7

2 years ago
Created attachment 8705303 [details]
original strings

Here are the strings as currently in the patch - certainly not great, probably not even good.
Flags: needinfo?(hurley)
(Assignee)

Comment 8

2 years ago
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)

Comment 10

2 years ago
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)
(Assignee)

Comment 11

2 years ago
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.
Duplicate of this bug: 1244334
Whiteboard: [spdy] → [spdy][necko-active]
(Assignee)

Comment 13

2 years ago
Created attachment 8741090 [details]
Screenshot 2016-04-13 12.33.30.png

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)
(Assignee)

Comment 14

2 years ago
Created attachment 8741098 [details] [diff] [review]
patch

Newest version of the patch, updated with the new strings.
(Assignee)

Updated

2 years ago
Attachment #8704329 - Attachment is obsolete: true

Comment 15

2 years ago
(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)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8741098 [details] [diff] [review]
patch

Pat - looking for review from you on the netwerk bits
Attachment #8741098 - Flags: review?(mcmanus)
(Assignee)

Comment 17

2 years ago
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+
(Assignee)

Comment 20

2 years ago
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.

Comment 22

2 years ago
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.

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f5f16c88c5b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 25

2 years ago
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.

Comment 27

2 years ago
Thanks
You need to log in before you can comment on or make changes to this bug.