883674 - RFC5746 renegotiation extension warnings are sent to the error console instead of the web console

Status: NEW

(Core :: Security: PSM, defect, P3)

Description

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

Three related problems:

1. The code that logs RFC5746 renegotiation extension warnings to the console is in PSM's HandshakeCallback (nsNSSCallbacks.cpp). This is not where UI code should go, and in particular, at this point we don't have a reference to the window/tab to send the web console message to.

2. The warning is not localized.

3. The message is sent to the error console instead of the web console.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

FWIW, it's now the Browser's Console it's sent to.

Comment 2

[:Cykesiopka]

Attachment: bug883674_nss-server-renego-xtn-hack_v1.patch

This adds an option to NSS that controls whether the NSS server sends the RFC 5746 extension. This is necessary for testing (mochitest uses ssltunnel, ssltunnel uses NSS), but like I mentioned in the patch itself, is not desirable for release builds.

For convenience, this patch is against the copy of NSS in m-i, but a separate NSS bug should probably be filed for this.

In addition, I have not run NSS' test suite against this change.

Comment 3

[:Cykesiopka]

Attachment: bug883674_RFC5746-renego-warn-web-console_v1.patch

The actual changes to send the error message to the web console.

Also includes a fix to filter out duplicate "SSL" class messages, to prevent console spam.

Comment 4

[:Cykesiopka]

Attachment: bug883674_ssltunnel_v1.patch

Comment 5

[:Cykesiopka]

Attachment: bug883674_tests_v1.patch

Comment 6

[:Cykesiopka]

^ These are the WIPs I have so far, mainly posted for reference. If anyone can think of a good way to mitigate my concerns for reduced compliance in attachment 8652839 [details] [diff] [review], please let me know.

Alternatively, if anyone would like to take over, please feel free.

Comment 7

[Martin Thomson [:mt:]]

RFC 5746 warnings shouldn't be generated at all.

If a server does not intend to support renegotiation, sending the signal is not necessary.  I don't care that there is a "MUST" in RFC 5746, as long as they correctly reject an attempt to renegotiate, they are safe.  What isn't safe is that as a client, we do not require that the server to indicate support for RFC 5746 before we respond to a HelloRequest.  I would rather fix that.

I would like to see this message removed rather than fixed.  I have no opinion on correcting other messages.

Comment 8

[Masatoshi Kimura [:emk]]

The problem is that we can't tell whether the server supports the insecure renegotiation unless the server supports RFC 5746. See the lengthy discussion in bug 549641 and bug 554594.

Comment 9

[Martin Thomson [:mt:]]

Those two bugs are 6 years old.  Lots of servers are disabling renegotiation; few if any don't support 5746.  The need for the warning has long passed.  Like I said, we have a way to make this safe and I'd rather do that than to spend any time fixing the warning.

Comment 10

[sjw]

See bug 665859 comment #8 for some stats.
A warning in console would help the admins to fix this issue. I think this one is blocking bug 665859.

Comment 11

[Martin Thomson [:mt:]]

Yep, those stats show that this is not a problem that needs a warning.  To put this in perspective, I get *tons* of instances of this warning from Akamai servers.  Those guys don't renegotiate, and they don't have the flaw.