742645 - Adding exception accidentally allowed in framed certificate error page

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Hector Zhao [:hectorz]]

The ability to add exceptions to framed certerrors was removed in Bug 633691 https://hg.mozilla.org/mozilla-central/rev/290d329672e5 ; the about:certerror expanders refactory in Bug 687958 https://hg.mozilla.org/mozilla-central/rev/501247d9125c made only the h2 header being removed instead of the whole expert content.

cc Dao as advised in https://bugzil.la/687958#c14

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 612514 [details] [diff] [review]
patch

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

a couple questions:
1. is collapsed not fine here? Asking cause hidden is adding complexity, the page already has a toggle() method using collapsed.
2. Why is hide() working as a toggle function, it is only hiding. should either just set hidden or be named something like toggleHidden()

Comment 3

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 612514 [details] [diff] [review]
> patch
> 
> Review of attachment 612514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a couple questions:
> 1. is collapsed not fine here? Asking cause hidden is adding complexity, the
> page already has a toggle() method using collapsed.

'collapsed' wouldn't hide the heading.

> 2. Why is hide() working as a toggle function, it is only hiding.

Oops, yes, it should just hide, not toggle.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #3)
> 'collapsed' wouldn't hide the heading.

hm could then we switch everything to hidden? I'm fine both ways, just trying to avoid added complexity.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ah I see, it's for the css definition we have there

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 612514 [details] [diff] [review]
patch

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

It's fine, but I don't think you need the whole hide() function, just set the hidden attribute in the if.

Comment 7

[Dão Gottwald [:dao]]

> just set the hidden attribute in the if.

done

http://hg.mozilla.org/integration/mozilla-inbound/rev/bc1ecf41f400

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/bc1ecf41f400

Comment 9

[Alex Keybl [:akeybl]]

Sounds like a low risk fix - please nominate for Aurora 13 uplift as soon as possible for this regression.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 612514 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): bug 687958
User impact if declined: being vulnerable to bug 633691 (which I can't access, so I'm not sure how severe this is)
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low risk

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 612514 [details] [diff] [review]
patch

[Triage Comment]
Approved for Aurora 13 given the low-risk nature of this fix.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/a043b27b4372

Comment 13

[u279076]

Does anyone have a testcase QA can use to verify this fix?

Comment 14

[Hector Zhao [:hectorz]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> Does anyone have a testcase QA can use to verify this fix?

Visit this page:
data:text/html,<iframe width="700" height="700" src="https://dynamic.12306.cn/"></iframe>

In a iframe the whole part titled "I Understand the Risks" should be hidden. Before the fix, only the title is hidden, descriptions and "Add Exception…" button can be seen.

Comment 15

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0

Verified on Ubuntu 12.04, Mac OS 10.6, Windows 7 in Firefox 13 beta 3 using URL in comment 14.

The iframe does no longer display any description from the I understand the risks category. Last displayed section is now "Technical details."

Comment 16

[Jo Hermans]

What was the reason that the button was removed ? A security problem ?  Bug 633691 isn't accessible yet.

It's currently causing problems in a few internal webpages of my company (*), and IT-support people are now advising people to uninstall Firefox, they claim that it has a bug. The workaround is to use 'open frame in new tab' of course.

(*) thanks due to their use of self-signed certificates, even though they have correct ones available - sigh

Comment 17

[Daniel Veditz [:dveditz]]

Jo: bug 633691 is now accessible. This is a defense against click-jacking primarily, and secondarily against spoofing attacks since you can't tell where a frame is really from. It is not a bug.

Needing to add an exception is problematic from the start, but if you must deal with servers like that the only safe way is to configure it once as an intentional step when you can confirm it's the site you think it is (hopefully checking the cert fingerprint against information you get out of band, but we know no one does that). Approving exceptions in the flow of an app is not safe.