Closed Bug 742645 Opened 8 years ago Closed 8 years ago

Adding exception accidentally allowed in framed certificate error page

Categories

(Firefox :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
firefox13 + verified
firefox-esr10 --- unaffected

People

(Reporter: hectorz, Assigned: dao)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [qa+])

Attachments

(1 file)

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
Assignee: nobody → dao
OS: Windows 7 → All
Hardware: x86 → All
Blocks: 687958
Keywords: regression
Summary: refactory make it possible to add cert exception in framed certerror → Adding exception accidentally allowed in framed certificate error page
Attached patch patchSplinter Review
Attachment #612514 - Flags: review?(mak77)
Component: General → Security
QA Contact: general → firefox
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()
(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.
(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.
ah I see, it's for the css definition we have there
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.
Attachment #612514 - Flags: review?(mak77) → review+
> just set the hidden attribute in the if.

done

http://hg.mozilla.org/integration/mozilla-inbound/rev/bc1ecf41f400
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/bc1ecf41f400
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Sounds like a low risk fix - please nominate for Aurora 13 uplift as soon as possible for this regression.
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
Attachment #612514 - Flags: approval-mozilla-aurora?
Comment on attachment 612514 [details] [diff] [review]
patch

[Triage Comment]
Approved for Aurora 13 given the low-risk nature of this fix.
Attachment #612514 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does anyone have a testcase QA can use to verify this fix?
(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.
Whiteboard: [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."
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
Depends on: 756841
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.
You need to log in before you can comment on or make changes to this bug.