549641 - Firefox raises alarm (in error console) about SSL servers being vulnerable to CVE-2009-3555

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Ben Bucksch (:BenB)]

Firefox claims the SSL servers are vulnerable to CVE-2009-3555, even if the server is not vulnerable (or even if TLS renegotiation is entirely turned off?). This is a clearly false alarm.

It's qualified by the word "potential", but that does little to mitigate the false alarm, as can be seen in bug 549109.

False alarms are damaging in the long term, esp. in security matters, as most of us know. Therefore:


1) IMHO, Firefox should not show a warning unless it has reason to believe the server is indeed vulnerable. If that warning appears even if the most obvious (and often only) fix for servers is applied, namely showing even if TLS renegotiation is disabled, I think it should not show at all. The word "potential" does very little to mitigate the false alarm.

2) At the absolute minimum, please make the warning more specific. Please mention that we expect servers to implement RFC 5746. The warning as-is without that reference achieves very little other than raising alarm and making admins confused, as you can see in the bug 549109.

Bug 549109 shows that the alarm did help to secure a critical server which before was unsecured, so I think the warning is very good and useful in general, but it should be more targetted and not show for servers where admins took appropriate steps to secure the server.

Comment 1

[Ben Bucksch (:BenB)]

See bug 549092#c5 for another false alarm.

Comment 2

[Daniel Veditz [:dveditz]]

A Firefox bug filed in the Thunderbird product? ;-) Moving to PSM which is what's generating the warnings.

It's not a false alarm in that we positively know that the server does not support RFC 5746, and that clients cannot be made secure until they stop connecting to such servers. It doesn't matter if server-A doesn't do renegotiation -- that helps server-A but doesn't help the user when he surfs to server-B and gets pwned.

I'm certainly open to changing the text to something else, like "Warning: server does not support RFC 5746 (see CVE-2009-3555)"

> Bug 549109 shows that the alarm did help to secure a critical server which
> before was unsecured, so I think the warning is very good and useful in
> general, but it should be more targetted and not show for servers where admins
> took appropriate steps to secure the server.

There is no way for the client to know the admin has taken appropriate steps to secure the server other than the server implementing the new renegotiation spec. I suppose the client could probe such servers to see if a renegotiation fails--at the cost of making SSL handshakes even more expensive than they are already!--but that's not foolproof because they might do renegotiation elsewhere on the site.

Comment 3

[Daniel Veditz [:dveditz]]

The appropriate level of warning should be discussed in the security or crypto newsgroups -- it doesn't seem very productive to hash it out in a bug.

Comment 4

[Kai Engert [:KaiE:]]

Just wanted to clarify, the prober you suggest won't be sufficient to feel secure.

A server may behave differently, based on which side of the connection attempt to initiate the negotiation.

In other words, a server may have disabled client initiated renegotiations, and the test performed by the client may succeed.

But still, the server may initiate renegotiations on its own, only when the server sees a need to (e.g. when a specific resource gets requested, which the prober doesn't know about).

In my understanding there is no way for a prober to know that.

The only fix is to implement RFC 5746, both in the client and the server.

Comment 5

[Daniel Veditz [:dveditz]]

Thanks, that's what I was trying to say with the "that's not foolproof" bit. I fully support some warning message when encountering such hosts but wouldn't mind explaining things differently if that helps.

<site> : no RFC 5746 support, potentially vulnerable to CVE-2009-3555

is that any better? If not let's stick with what we've got and see what people say.

Comment 6

[Ben Bucksch (:BenB)]

That message would make a wrong causal link between "no RFC 5746 support" and "vulnerable". That link may exist from our perspective, but not from the admin's.

Therefore, I'd propose:
"potentially vulnerable to CVE-2009-3555. Site should implement RFC 5746 support. <http://www.mozilla.org/.../tls-renegotiation>"

(Note: not a wiki page!) On that page, explain the situation, for users and admins, and maybe also publish that you may in the future not support SSL servers which don't support that RFC.

Comment 7

[Kai Engert [:KaiE:]]

... although your proposal would triple the number of bytes we dump into the error console. I don't personally mind, but today we dump this for a lot of sites, so trying to keep it should short would be good.

Could we use a minimal text and point people to the help page, using a tinyurl?

Comment 8

[Ben Bucksch (:BenB)]

While I agree that short is good, I don't think there's a particular need to save bytes.
And all URLs we publish should be stable and under our exclusive control.

Removing anything would remove critical information bits (CVE, RFC, no causal link, URL), e.g. why I filed this bug.
How about:
"<site> possibly vulnerable to CVE-2009-3555. Implement RFC 5746 <http://www.mozilla.org/tls-renegotiation>"
I don't think we can or should make it any shorter.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to comment #6)
> > <site> : no RFC 5746 support, potentially vulnerable to CVE-2009-3555
>
> That message would make a wrong causal link between "no RFC 5746 support" and
> "vulnerable". That link may exist from our perspective, but not from the
> admin's.

There absolutely is a link. If you don't support RI you ARE vulnerable if your site allows renegotiation. Your site is ONLY safe if you have taken proactive steps to disable renegotiation because renegotiation is on by default. Depending on your server software you might even have had to upgrade your server to be able to turn it off.

The message is, you ARE vulnerable to CVE-2009-3555 unless you have taken steps to do something; implementing rfc 5746 is the recommended way of doing something because then people communicating with you can tell that you've done it. But you must do SOMEthing.

If it makes people happy to swap s/potentially/possibly/ then fine, they mean pretty much the same to me. I really don't want to see a huge message because it's already going to clutter up the console. Admins only need to find the link once, but it's on the console every visit. All we need is enough information so admins can yahoo what the message means. 

Personally I'm fine with the current short message -- it does the job -- but we should put but a MDC page about what it means.

Comment 10

[Eric Shepherd [:sheppy]]

This warning actually comes up when you access MDC. I'm not clear on what it means. :)

Comment 11

[Ben Bucksch (:BenB)]

> Your site is ONLY safe if you have taken proactive
> steps to disable renegotiation because renegotiation is on by default.

Exactly. The mozilla.org sites are safe (bug 549109), yet we say that they are "potentially vulnerable". This is particularly embarrassing for versioncheck.mozilla.org (which regularly happens and spams the error console) and addons.mozilla.org.
The message suggests a problem which doesn't exist.

With the security release upcoming, can we please fix this before we spam millions of users (who check the error console) and frighten them about a problem which may not even exist? We should at least lead them the right way, meaning reference "RFC 5746" in the message and give a link with explanations.

Comment 12

[Ben Bucksch (:BenB)]

Apparently wtc agrees, see bug 535649 comment 43.

Comment 14

[John J. Barton]

Is this bug about removing the info message altogether? If not how can we make it so? The current situation is not appropriate.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: Patch v1

We believe it's useful to have a warning by default, it has already proven to be helpful, several servers have been upgraded as a consequence.

Regarding the frequency of the message and being distracted by it, please note there are three classes of messages in the error console:
- errors
- warnings
- messages

The message about CVE-2009-3555 is shown as a plain message. If you click one of the icons that will filter only errors, or only warnings, you won't see this informational message.

Also, we'd like to avoid adding code that would reduce the warning to once per session and site, as it would add unnecessary complexity on the stable branches, where we try to be conservative with new logic.

Still, we'd like to address two concerns that have been raised, and the following compromise is being suggested.

- instead of saying "potentially vulnerable to CVE-2009-3555"
  this patch changes it to say:
      "server does not support RFC 5746, see CVE-2009-3555"

- for developers who experience trouble caused by the amount of additional
  console log message, we want to offer an additional pref.
  This patch adds pref
    security.ssl.warn_missing_rfc5746
  It will have the value "1" by default, which means "show the warnings".
  Developers can set it to "0" to turn the warning off.

Comment 16

[John J. Barton]

(In reply to comment #15)
> Created an attachment (id=436588) [details]
> Patch v1
> 
> We believe it's useful to have a warning by default, it has already proven to
> be helpful, several servers have been upgraded as a consequence.

As a matter of principle, this is still ridiculous. Creating some success on your agenda does not justify inflicting these messages on everyone else.

> 
> Regarding the frequency of the message and being distracted by it, please note
> there are three classes of messages in the error console:
> - errors
> - warnings
> - messages
> 
> The message about CVE-2009-3555 is shown as a plain message. If you click one
> of the icons that will filter only errors, or only warnings, you won't see this
> informational message.

Your claim these messages are insignificant misses the reality that the Firefox Error Console is not the only client of nsIConsoleService. For example, the primary mechanism we have for debugging Javascript debuggers is logging from our application including feeds from nsIConsoleService. 

> 
> Also, we'd like to avoid adding code that would reduce the warning to once per
> session and site, as it would add unnecessary complexity on the stable
> branches, where we try to be conservative with new logic.
> 
> Still, we'd like to address two concerns that have been raised, and the
> following compromise is being suggested.
> 
> - instead of saying "potentially vulnerable to CVE-2009-3555"
>   this patch changes it to say:
>       "server does not support RFC 5746, see CVE-2009-3555"

That message provides no useful information to anyone other than the people reading this bug report. Your message should provide a URL to a mozilla maintained page providing specific instructions that will cause the message to be prevented. Otherwise you are just wasting more of everyone's time.

> 
> - for developers who experience trouble caused by the amount of additional
>   console log message, we want to offer an additional pref.
>   This patch adds pref
>     security.ssl.warn_missing_rfc5746
>   It will have the value "1" by default, which means "show the warnings".
>   Developers can set it to "0" to turn the warning off.

Thank you, I will use it as soon as it becomes available.

Comment 17

[Kai Engert [:KaiE:]]

For developers who would like to filter the message related to CVE-2009-3555, I'd like to make you aware of a tool that could help you.

I found Firefox add-on Console² available from addons.mozilla.org
It offers four ways to filter away the message:

- you can click "messages" to filter out any informational messages
  and see all errors and all alerts

or

- you chould click "chrome" to filter out any messages that are produced
  from Mozilla internally (not produced by content of websites)

or

- you could enable "Options / Hide Duplicates" to filter repeated 
  messages

or 

- add -CVE-2009-3555 to search field

Comment 18

[Daniel Veditz [:dveditz]]

> security.ssl.warn_missing_rfc5746
> It will have the value "1" by default, which means "show the warnings".

Why an int pref instead of a boolean?

Comment 19

[Kai Engert [:KaiE:]]

(In reply to comment #18)
> > security.ssl.warn_missing_rfc5746
> > It will have the value "1" by default, which means "show the warnings".
> 
> Why an int pref instead of a boolean?

This would allow us to have different warning settings in the future, without adding yet another pref.

Comment 20

[Ben Bucksch (:BenB)]

> This would allow us to have different warning settings in the future, without
> adding yet another pref.

Do you expect to introduce a level 2 which is even more chatty than now?

Comment 21

[Kai Engert [:KaiE:]]

(In reply to comment #20)
> > This would allow us to have different warning settings in the future, without
> > adding yet another pref.
> 
> Do you expect to introduce a level 2 which is even more chatty than now?

I don't know yet what we might want to do,
but when we discussed this a couple of days ago, we realized it's unfortunate that all the other prefs we had introduced recently were bools, not int, which makes it necessary to introduce yet another pref to suppress the warnings.

Comment 22

[Matt McCutchen]

(In reply to comment #4)
> Just wanted to clarify, the prober you suggest won't be sufficient to feel
> secure.
[...]
> But still, the server may initiate renegotiations on its own, only when the
> server sees a need to (e.g. when a specific resource gets requested, which the
> prober doesn't know about).
> 
> In my understanding there is no way for a prober to know that.

Let me clarify this point, for I almost missed it.  An attacker would request a resource, let the server initiate renegotiation, and then hand over to the client to complete what the client thinks is an initial handshake.  Thus, having the client complain when it sees the server initiate renegotiation does not close the hole.

Comment 23

[Matt McCutchen]

(In reply to comment #2)
> I suppose the client could probe such servers to see if a renegotiation
> fails--at the cost of making SSL handshakes even more expensive than they are
> already!--but that's not foolproof because they might do renegotiation
> elsewhere on the site.

That probe would still be useful: if the renegotiation succeeds, I can tell the server admin that their server is actually vulnerable, and not merely failing to advertise its invulnerability.  Currently, I am doing a separate test with "openssl s_client" (http://blog.ivanristic.com/2009/12/testing-for-ssl-renegotiation.html).

Comment 24

[Ben Bucksch (:BenB)]

Matt, this bug is not about a general discussion about server vulnerability and its detection. Please do that on the newsgroups. This bug is merely about how (or whether) we *report* the facts no the error console.

Comment 25

[Ben Bucksch (:BenB)]

> "server does not support RFC 5746, see CVE-2009-3555"

Sounds good to me, esp. if you add ..." and <http://www.mozilla.org/tlsrenego>" or ..." and <http://www.mozilla.org/rfc5746>"

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 436588 [details] [diff] [review]
Patch v1

Asking Honza for additional review, as Bob didn't have time yet.

Honza, FYI, I consider file netwerk/base/public/security-prefs.js
 to be part of PSM, even though the file lives in netwerk (if we can we might want to move that file to PSM some day)

Comment 27

[Eric Shepherd [:sheppy]]

Is IT/webdev aware that several of our own servers result in this warning? It's quite disturbing to see it coming up on mozilla sites. :)

Comment 28

[Ben Bucksch (:BenB)]

sheppy, read comment 0. Bug 549109 and bug 555952.

Comment 29

[Eric Shepherd [:sheppy]]

Sorry, I flicked back through stuff but apparently not to the top.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 436588 [details] [diff] [review]
Patch v1

> +++ b/security/manager/ssl/src/nsNSSCallbacks.cpp
>@@ -871,25 +871,30 @@ void PR_CALLBACK HandshakeCallback(PRFil
>     nsNSSSocketInfo* infoObject = (nsNSSSocketInfo*) fd->higher->secret;
>-    nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>-    if (infoObject && console) {
>+    nsCOMPtr<nsIConsoleService> console;
>+    if (wantWarning) {
>+      console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>+    }
>+    if (wantWarning && infoObject && console) {
>       nsXPIDLCString hostName;
>       infoObject->GetHostName(getter_Copies(hostName));
> 
>       nsAutoString msg;
>       msg.Append(NS_ConvertASCIItoUTF16(hostName));
>-      msg.Append(NS_LITERAL_STRING(" : potentially vulnerable to CVE-2009-3555"));
>+      msg.Append(NS_LITERAL_STRING(" : server does not support RFC 5746, see CVE-2009-3555"));
> 
>       console->LogStringMessage(msg.get());
>     }

Wouldn't be nicer to simply put this code block to a branch if (wantWarning) { } ?

Comment 31

[Robert Relyea]

Comment on attachment 436588 [details] [diff] [review]
Patch v1

r+..

one style issue which Honras already pointed out. The current style is easier for current reviewers. Future reviewers would probably expect the nesting. But I leave it to you decide if this is acceptable PSM style.

bob

Comment 32

[Robert Relyea]

^Honras^Honza^

bob

Comment 33

[John J. Barton]

In http://gsfn.us/t/106av Thunderbird users are discovering that their outgoing email is broken because of CVE-2009-3555.

Comment 34

[Reed Loden [:reed]]

(In reply to comment #33)
> In http://gsfn.us/t/106av Thunderbird users are discovering that their outgoing
> email is broken because of CVE-2009-3555.

Roland's reply (http://getsatisfaction.com/mozilla_messaging/topics/thunderbird_cannot_send_email#reply_2313669) clearly says "The CVE-2009-3555 error is not related to this issue.", so your above comment seems wrong.

Comment 35

[Ben Bucksch (:BenB)]

Correct, their impression is wrong, but nevertheless that's the conclusion they draw. That's what this bug is about: the false impression.

Comment 36

[Kai Engert [:KaiE:]]

(In reply to comment #30)
> 
> Wouldn't be nicer to simply put this code block to a branch if (wantWarning) {
> } ?


I'll switch to nested "if"s and add one more indent level.

    nsCOMPtr<nsIConsoleService> console;
    if (infoObject && wantWarning) {
      console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
      if (console) {
        nsXPIDLCString hostName;
        infoObject->GetHostName(getter_Copies(hostName));

        nsAutoString msg;
        msg.Append(NS_ConvertASCIItoUTF16(hostName));
        msg.Append(NS_LITERAL_STRING(" : server does not support RFC 5746, see CVE-2009-3555"));

        console->LogStringMessage(msg.get());
      }
    }

Comment 37

[Kai Engert [:KaiE:]]

http://hg.mozilla.org/mozilla-central/rev/ccd19364cb13

Comment 38

[Kai Engert [:KaiE:]]

Comment on attachment 436588 [details] [diff] [review]
Patch v1

Requesting approval for log message improvement (and pref to disable) for stable branches.

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 436588 [details] [diff] [review]
Patch v1

not approved for the stable branches, a lot of code churn for what is effectively a simple string change for 99.99% of our users.

Comment 41

[Kai Engert [:KaiE:]]

(In reply to comment #40)
> (From update of attachment 436588 [details] [diff] [review])
> not approved for the stable branches, a lot of code churn for what is
> effectively a simple string change for 99.99% of our users.

The string change is a single line, if you'd like that, only.

The other 99% of the patch are the option to suppress the warning.

Comment 42

[Daniel Veditz [:dveditz]]

For the branches, yes please. Anyone actually bothered enough to find out about the pref can download a trunk beta by time we get 1.9.2.6 released.

Comment 43

[Kai Engert [:KaiE:]]

Attachment: Branch patch, string change only

This is a subset of the patch which changes the string, only, intended for stable branches. Obvious single line change to use the identical string as used on trunk.
rs=me

Requesting branch approval

Comment 44

[Daniel Veditz [:dveditz]]

Comment on attachment 451590 [details] [diff] [review]
Branch patch, string change only

Thanks! Approved for 1.9.2.6 and 1.9.1.11, a=dveditz for release-drivers

Comment 45

[Kai Engert [:KaiE:]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bc5ae6a0542e

Comment 46

[Kai Engert [:KaiE:]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7e4fa2fc106d

Comment 47

[Eric Shepherd [:sheppy]]

Now documented:

https://developer.mozilla.org/en/Preferences/Mozilla_preferences_for_uber-geeks