Adding exception accidentally allowed in framed certificate error page

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Security
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: hectorz, Assigned: dao)

Tracking

({regression, sec-moderate})

Trunk
Firefox 14
regression, sec-moderate
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13+ verified, firefox-esr10 unaffected)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → dao
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 612514 [details] [diff] [review]
patch
Attachment #612514 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
status-firefox13: --- → affected
tracking-firefox13: --- → ?
(Assignee)

Updated

5 years ago
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()
(Assignee)

Comment 3

5 years ago
(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+
(Assignee)

Comment 7

5 years ago
> 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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 9

5 years ago
Sounds like a low risk fix - please nominate for Aurora 13 uplift as soon as possible for this regression.
tracking-firefox13: ? → +
(Assignee)

Comment 10

5 years ago
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+
(Assignee)

Comment 12

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/a043b27b4372
status-firefox13: affected → fixed
Does anyone have a testcase QA can use to verify this fix?
(Reporter)

Comment 14

5 years ago
(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."
status-firefox13: fixed → verified

Comment 16

5 years ago
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.
status-firefox-esr10: --- → unaffected
Keywords: sec-moderate
You need to log in before you can comment on or make changes to this bug.