304286 - Certificate failures during automatic check for updates should not give user choice to connect anyway

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Use a Windows nightly for a while.

Result: I get a certificate warning telling me that Firefox saw a certificate
for "chameleon" when it expected to see a certificate for
"aus-staging.mozilla.org".  I'm guessing this happens when Firefox automatically
checks for updates.  I see the same dialog if I check for updates manually.

Expected: At most, show a dialog stating that Firefox wasn't able to check for
updates, and display details about the certificate mismatch in the JavaScript
console.

This seems like a security hole.  I imagine that many users would click "OK" if
they saw a similar dialog out of the blue.  This negates most of the benefit of
using https to check for updates.  Users should not be prompted with a
certificate warning just because an attacker is using a man-in-the-middle attack
between the user and the automatic update service.

I don't know how bad it is if a user clicks "OK" because I don't know what other
safeguards are in place.

Comment 1

[Daniel Veditz [:dveditz]]

Obviously by time we ship the beta we need to be pointing at a server with a
real cert so users don't get this every time. But Jesse's right, we must not
give the user the chance to "OK" this dialog.

These dialogs come from the bowels of PSM, though, it might be hard to intercept
them :-(

(In order to allow nightly testing once this is fixed we'll have to get an
aus-staging cert, or change the update URL to "chameleon", which seems like it
would be OK for testing purposes anyway).

Comment 2

[Darin Fisher]

We're going to be switching over to aus2.mozilla.org very soon now.  It has a
properly configured cert, so this should not be a problem.  I don't think we
really need to fix anything here.  The aus-staging.mozilla.org pref is going to
be ignored by the browser once we make the switch to aus2.mozilla.org, so once
we do that no one should see this cert warning again.  The
aus-staging.mozilla.org site was just a quick hack to get something in place to
test AUS functionality.  It is by no means the permanent home for this service.

Marking WONTFIX.

Comment 3

[Darin Fisher]

Hrm.. so you'd argue that we should prevent the update if this dialog appears. 
Why don't we also prevent browsing to HTTPS URLs when this dialog appears? 
Because the update service can do considerable harm?  Hmm...

Comment 4

[Jesse Ruderman]

Yes, that's what I'm arguing.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

last I checked PSM did not allow configuring that prompt per-channel. I think
it'd be great to be able to do that.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

No one *should* see this warning again once we switch to a proper cert, but we
should still suppress its display for AUS2 as an additional safeguard in the
event of server failure.  As I recall, server failure shortly after the Firefox
1.0 release resulted in a number of irate users complaining about confusing
dialog popups.

Not only did that waste our time handling the complaints, it presented a poor
user experience that we should avoid when designing the next version of this
critical automated system.  AUS2 should fail silently when it runs into a server
mismatch (and other problems that block the update) while automatically updating
the application.  Manual updates should, on the other hand, notify the user why
they failed, so that users know why the application isn't doing what they asked
it to do.

Comment 7

[Darin Fisher]

If you want this fixed for beta, then please find another engineer to work on
it.  I won't have time until after beta because I'll be away on vacation.

Does this really need to be a security sensitive bug?

Comment 8

[Mike Connor [:mconnor]]

Minusing for b4, moving out to 1.9a1 since the change appears to be high risk in
an area without active module ownership.  If something comes along that's safe,
we can consider it, but this doesn't seem more exploitable than any other MITM
type of attack.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Mike, in my opinion the issue here is not primarily security.  When Firefox
pings a web server as part of an automated process (rather than in response to a
user request to load a web page), like when checking for updates, it shouldn't
report problems accessing that server, since such reports are likely to be
confusing and aggravating to end users.

For example, imagine that AUS2's SSL certificate becomes corrupted (or expires,
or is otherwise suspicious) a year from now when there are millions of users
running Firefox 1.5 on any given day.

If the certificate remains corrupted for a day, millions of users are going to
see this message.  It's going to pop up suddenly, without warning.  It will
appear to be related to the last web page the user loaded (since Firefox does
not notify the user that it is automatically checking for updates, so there's no
clear connection between the message and the auto-check).  Most users will find
the dialog difficult to comprehend.  It'll be a frustrating experience.  Users
will complain.

Of course, if nothing ever goes wrong with the server-side of AUS2, none of this
will ever happen.  But relying on that is a risky proposition.  We had this
happen one day shortly after 1.0 was released (so my description above of the
consequences isn't idle speculation), and although we'll have a better server
architecture in place for 1.5, we can't guarantee it'll never break, nor should
the client be so fragile as to depend on that.

I'm not suggesting that we silently succeed when we hit this problem or do
anything else to reduce user security.  I'm saying we should silently fail, just
as we silently check.

Or, at most, we should display a context-sensitive dialog box explaining to the
user that Firefox ran into a problem checking for updates and suggesting that
the user check for updates manually on Mozilla's web site (or some such advice).

From your comment when minusing this bug it sounds like you reviewed with a
focus on the security concerns.  Rerequesting blocking-1.8b4; please reconsider
with an eye to this important usability issue.

Comment 10

[Mike Connor [:mconnor]]

Really, the key was that the fix seems to be potentially high-risk, since we
don't have expertise in PSM on hand to take a change of this nature at this
stage.  The theoretical "the cert might break" case doesn't outweigh the real
risk in mucking with PSM and changing how it interacts with channels, and
without active PSM hackers the risk increases dramatically.

I'm okay with the potential for a negative experience, since I trust our
sysadmins to minimize the risk here, and to react quickly if the cert gets
broken.  Darin WONTFIXed this on the usability issue, and left it reopened for
the security issues.

If this was two months ago, I'd want a fix, but that's not where we're at, and
the justification just isn't there for a theoretical risk that we can minimize
on our end.

Comment 11

[Chase Phillips]

The bug that caused this bug (a mismatch for cert v. host) has been fixed by the
switch to a default app update URL which refers to 'aus2.mozilla.org'.  I'm not
going to resolve this bug as fixed, however, since it's focused on the
presentation of the message itself.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> The bug that caused this bug (a mismatch for cert v. host) has been fixed by the
> switch to a default app update URL which refers to 'aus2.mozilla.org'.

No, the testing cert mismatch merely demonstrated this problem exists. The
entire point of using SSL on the AUS server is to protect against bogus updates
through some DNS poisoning or MITM attack. The result of such an attack MUST NOT
be a prompt that many users will simply OK.

There is a parallel argument that we should also not show such a prompt for
individual websites, but the scale of potential disaster in the AUS case argues
that we should fix this without requiring a resolution on the similar web
surfing case.

Comment 13

[Scott MacGregor]

It's a little unclear to me what the remaining issues are here. This late in the
release cycle, any changes for this would have to be extremely simple / straight
forward. Anything at all risky and we would not block on this anymore. Just too
late.

Leaving the nomination alone pending feedback.

Comment 14

[Darin Fisher]

mscott: the risk is that a user may click through an SSL certificate mismatch
dialog, which would allow the attacker to install software on the user's system.
 since we know that the cert will always be valid, we don't need to show the
user this UI.  it is just confusing UI, and it may lead to bad things.  it
should be easy to fix this.

Comment 15

[Asa Dotzler [:asa]]

Darin, when do you think you could get to this? If it's not pretty soon, can you
suggest what you think the right fix would be so we can get someone else on it?

Comment 16

[Darin Fisher]

I'm not sure what the right fix is, but somehow we need to probably have the
Downloader class in nsUpdateService.js implement an interface returned via
"getInterface" that causes PSM to suppress the dialog.

Comment 17

[Darin Fisher]

Attachment: v1 patch

This patch involves 3 changes:

 1) Make nsNSSIOLayer.cpp check the notification callbacks for a 
    nsIBadCertListener instance before defaulting to PSM's implementation.

 2) Fix the assertion in nsHttpConnection::GetInterface to match reality.
    Document reality.

 3) Make the "Checker" class in nsUpdateService.js supply a nsIBadCertListener
    implementation that returns false for all methods.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 199361 [details] [diff] [review]
v1 patch

+    nsCOMPtr<nsIBadCertListener> handler = do_GetInterface(callbacks);

does this require a proxy too?

do JS XPCOM objects have a threadsafe nsISupports implementation?

Comment 19

[Darin Fisher]

> +    nsCOMPtr<nsIBadCertListener> handler = do_GetInterface(callbacks);
> 
> does this require a proxy too?

callbacks is a proxy.  take a look at nsNSSSocketInfo::SetNotificationCallbacks


> do JS XPCOM objects have a threadsafe nsISupports implementation?

yes

Comment 20

[Darin Fisher]

biesi: yes, this code is fragile because it expects the notification callbacks
implementation to hand out threadsafe objects.  that requirement has existed
since necko and PSM were created.  ultimately, this code should all be fixed
when we have an interface requestor proxy that returns proxied objects.  see bug
243591 and bug 16773.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, ok... I missed that part, I only looked at GetNotificationCallbacks and the
socket transport.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 199361 [details] [diff] [review]
v1 patch

r=biesi, but shouldn't you set the out parameter of confirmUnknownIssuer to
something?

Comment 23

[Darin Fisher]

> r=biesi, but shouldn't you set the out parameter of confirmUnknownIssuer to
> something?

Should I?  I thought about it when I wrote the patch, and the fact is that the
out parameter is meaningless when the function returns false.  So, I didn't bother.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

I dunno, it's an XPCOM function, and aren't all of them supposed to set their
out params? could just set it to UNINIT_ADD_FLAG.

(does this bug really need to be security-sensitive?)

Comment 25

[Darin Fisher]

> I dunno, it's an XPCOM function, and aren't all of them supposed to set their
> out params? could just set it to UNINIT_ADD_FLAG.

We don't set out params when returning nsresult failure codes.  What's the harm
in not setting the out param?


> (does this bug really need to be security-sensitive?)

I think the concern is that this bug could be used to trick existing 1.5 beta 1
and beta 2 users into installing rogue software on their systems.  So, it is
good perhaps to keep quiet about it until we have most people on rc1?

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, but this code uses a success nsresult code. the harm would probably be
uninitialized memory reads. but, I suppose the code here won't look at the param
in the false return case. ok.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 199361 [details] [diff] [review]
v1 patch

sr=dveditz

Comment 28

[Darin Fisher]

fixed-on-trunk

Comment 29

[Darin Fisher]

fixed1.8

Comment 30

[Ben Bucksch (:BenB)]

Thanks, guys, for this fix! I had the exact same problem with my own updater, and looked into it (and biesi did, too), but we were missing the NSS hookup 1).

Comment 31

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #25)
> > (does this bug really need to be security-sensitive?)
> 
> I think the concern is that this bug could be used to trick existing 1.5 beta 1
> and beta 2 users into installing rogue software on their systems.  So, it is
> good perhaps to keep quiet about it until we have most people on rc1?

Can this bug be made public now, please?

Comment 32

[Jesse Ruderman]

See also bug 366191, "Something tries to MITM Firefox's automatic connection to addons.mozilla.org, resulting in an annoying expired-certificate dialog".