Closed Bug 960146 (CVE-2014-1527) Opened 10 years ago Closed 10 years ago

Preventing scrolling also prevents re-showing hidden address bar

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
27 Branch
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox26 wontfix, firefox27+ wontfix, firefox28+ wontfix, firefox29+ verified, firefox30 verified, firefox-esr24 unaffected, fennec+)

Status:
VERIFIED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox26 --- wontfix
firefox27 + wontfix
firefox28 + wontfix
firefox29 + verified
firefox30 --- verified
firefox-esr24 --- unaffected
fennec + ---

People

(Reporter: jupenur, Assigned: wesj)

References

Details

(Keywords: csectype-dos, csectype-spoof, sec-moderate, Whiteboard: [adv-main29+])

Attachments

(3 files, 2 obsolete files)

Fix toolbar DoS possibility
17.31 KB, patch
cwiiis
: review+
lsblakk
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
abillings
: sec-approval+
Details | Diff | Splinter Review
Backout ab25dcdc319b (bug 960146) for touch event regressions
17.50 KB, patch
bajaj
: approval-mozilla-aurora+
bajaj
: approval-mozilla-beta+
Details | Diff | Splinter Review
Patch part 2
1.44 KB, patch
kats
: review+
abillings
: approval-mozilla-beta+
Details | Diff | Splinter Review 
After the address bar on Android has been hidden (by scrolling down), calling preventDefault on touchstart and DOMMouseScroll events prevents the user from making it visible again. It should prevent scrolling the content but it should still always be possible to reliably make the address bar visible.

As a result of this bug, it would currently be fairly trivial to spoof the browser UI without the user ever realizing it.

Tested on Android 4.3.0; HTC One Build/JJS15J, Firefox 26.01 and 27.0 beta

Test case:

<!DOCTYPE html>
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1">
<body style="height: 10000px;">
    Scroll down until the address bar disappears, then tap on the page.
</body>
<script>
    var prevent = false;
    document.body.onclick = function () { prevent = true; };
    window.addEventListener('DOMMouseScroll', function (e) {
        e.preventDefault();
    });
    document.ontouchstart = function (e) {
        if (prevent) e.preventDefault();
    };
</script>


Workaround:

If your device happens to have a menu button, pressing it will always show the address bar. Not all devices have the button though.
Confirmed.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
tracking-firefox27: --- → ?
tracking-firefox28: --- → ?
tracking-firefox29: --- → ?
Ever confirmed: true
Bugs that interfere with the address bar are generally high severity, what is it that makes this different?
Hmm.. I thought we caught and responded to this higher in the event handling chain. cc'ing kats too.
huh, I thought we did too... I guess not :/ I'll take a look.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Dan can probably comment on the rating.
Flags: needinfo?(dveditz)
Attached patch Fix toolbar DoS possibility (obsolete) — — Splinter Review 
Please be thorough in this review - I think it's ok, but it's been a long time since I touched any of this code and it isn't unlikely I'm missing something huge and obvious.

If you aren't the right reviewer, please pass it on.
Attachment #8361097 - Flags: review?(wjohnston)
tracking-fennec: ? → +
Comment on attachment 8361097 [details] [diff] [review]
Fix toolbar DoS possibility

Kats owns these files and should review this stuff too.
Attachment #8361097 - Flags: review?(bugmail.mozilla)
Comment on attachment 8361097 [details] [diff] [review]
Fix toolbar DoS possibility

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

At first glance this looks like way too big of a change and maybe going down the wrong path. Didn't we at one point have a touch interceptor on LayerView that would make the toolbar scroll on regardless of whether or not content did a preventDefault? I'll try to dig through the code history to see what happened to that.
Looks like the code I remember was removed in the big rewrite of bug 858969. But perhaps we can resurrect a small part of the touch interceptor code? I'll think about it.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Looks like the code I remember was removed in the big rewrite of bug 858969.
> But perhaps we can resurrect a small part of the touch interceptor code?
> I'll think about it.

This seemed like a nicer way to do it versus adding yet another interceptor and complicating the paths further (I would consider this a simplification, I think it removes more than it adds(?)) - is there a reason you prefer the interceptor route?

It seemed to me that it made sense that the PanZoomController should know whether the events had been default-prevented, given most of the action is driven from there.
Comment on attachment 8361097 [details] [diff] [review]
Fix toolbar DoS possibility

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

Ok, looking at it again it makes sense. I was scared by the changes to TouchEventHandler when I first glanced through it but it looks reasonable. Minusing because of the second comment below (we still need the code from preventedTouchFinished somewhere) and I'd like to review the new version.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +187,5 @@
>              }
>  
>          });
>  
> +        mDefaultPrevented = false;

Remove this, it will be intialized to false by default.

@@ -407,5 @@
> -        if (mState == PanZoomState.WAITING_LISTENERS) {
> -            // if we enter here, we just finished a block of events whose default actions
> -            // were prevented by touch listeners. Now there are no touch points left, so
> -            // we need to reset our state and re-bounce because we might be in overscroll
> -            bounce();

This code still needs to go somewhere, probably handleEvent(MotionEvent,boolean). If the event is an UP or a CANCEL, defaultPrevented is set to true, and mState == PanZoomState.WAITING_LISTENERS, then we need to bounce() at the end of that function.

::: mobile/android/base/gfx/TouchEventHandler.java
@@ +74,5 @@
>      // default-prevented or not (or we time out waiting for that).
>      private boolean mHoldInQueue;
>  
> +    // false if the current event block has been default-prevented.
> +    private boolean mAllowDefaultAction;

Append to this comment:

In this case, we still pass the event to both gecko and the pan/zoom controller, but the pan/zoom controller will not use it to scroll content. It may still use it for other things such as making the dynamic toolbar visible.
Attachment #8361097 - Flags: review?(wjohnston) → review-
Comment on attachment 8361097 [details] [diff] [review]
Fix toolbar DoS possibility

Whoops. :)
Attachment #8361097 - Flags: review?(bugmail.mozilla) → review?(wjohnston)
Comment on attachment 8361097 [details] [diff] [review]
Fix toolbar DoS possibility

I'll just wait for a new patch.
Attachment #8361097 - Flags: review-
Comment on attachment 8361097 [details] [diff] [review]
Fix toolbar DoS possibility

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

Assuming you meant to clear the other flag (same mistake I made!)
Attachment #8361097 - Flags: review?(wjohnston) → review-
Attached patch Fix toolbar DoS possibility (obsolete) — — Splinter Review 
Made the changes requested by kats. Won't hurt to have two reviewers.
Attachment #8361097 - Attachment is obsolete: true
Attachment #8363736 - Flags: review?(wjohnston)
Attachment #8363736 - Flags: review?(bugmail.mozilla)
Attachment #8363736 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8363736 [details] [diff] [review]
Fix toolbar DoS possibility

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

Oh. I believe the commit message for sec bugs should just have the bug number and reviewer, nothing else.
Comment on attachment 8363736 [details] [diff] [review]
Fix toolbar DoS possibility

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

I actually like this a little better. I guess that's how things are supposed to work :)
Attachment #8363736 - Flags: review?(wjohnston) → review+
This is definitely more severe than sec-low since it interferes with the primary control surface of the mobile browser, it's more than just spoofing an address.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> I believe the commit message for sec bugs should just have the bug
> number and reviewer, nothing else.

They definitely shouldn't explain or hint at the vulnerability. A generic description of what's being changed can be ok (maybe "fix prevent-default issues"?)

Severe security bugs should go through the sec-approval process so we don't cause our own firedrills by checking-in too close to a release to ship the fix. In this case we're right up against the last beta of Firefox 27 so if we can't live with the patch in the tree for 7-8 weeks before users get the fix then we should wait.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(dveditz)
Keywords: sec-low → sec-high
Monday (Jan 27) is the last mobile beta for Fx27 so it needs to land before then (with enough time to be transplanted on Aurora and Beta branches) or else it should wait until mid-February to land.
(In reply to Daniel Veditz [:dveditz] from comment #19)
> Monday (Jan 27) is the last mobile beta for Fx27 so it needs to land before
> then (with enough time to be transplanted on Aurora and Beta branches) or
> else it should wait until mid-February to land.

I'm unclear as to whether I should check this in or not... Could you clarify? It's ready to check-in now, should I do so and request aurora/beta uplift, or should I wait?
Flags: needinfo?(dveditz)
(In reply to Chris Lord [:cwiiis] from comment #20)
> I'm unclear as to whether I should check this in or not... Could you
> clarify? It's ready to check-in now, should I do so and request aurora/beta
> uplift, or should I wait?

Go to patch details, then set sec-approval? and fill out the form and wait for approval before landing.  For Aurora and Beta, either some of that will be approved at the same time, or you'll want to fill it out separately after it lands on m-c.  I think that's how release management prefers handling it.
Flags: needinfo?(dveditz)
Same patch with altered commit message that doesn't point out the security implications of the fixed issue.
Attachment #8363736 - Attachment is obsolete: true
Attachment #8364394 - Flags: review+
Comment on attachment 8364394 [details] [diff] [review]
Fix toolbar DoS possibility

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

UI could be spoofed quite easily. The spoof would be broken as soon as the user pressed any hardware keys (including return and home, not just menu).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't believe so.

Which older supported branches are affected by this flaw?

This bug exists in all Firefox for Android versions since 23, I think.

If not all supported branches, which bug introduced the flaw?

Bug 858969

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

A backport should be fairly trivial as this code hasn't changed much(/at all?) between release and now.

How likely is this patch to cause regressions; how much testing does it need?

I don't think this patch will cause regressions, but I think it should bake for a couple of weeks. There's the possibility that scrolling or zooming may be possible in situations where previously it wasn't, or impossible when it was (the latter is less likely).
Attachment #8364394 - Flags: sec-approval?
I don't know if it makes any difference, but as far as I can tell, hardware buttons don't break the spoof as you described. Only the menu button does it reliably. The home button doesn't seem to break it at all, an the back button only does if it triggers a page load. That can be prevented using history.pushState.
(In reply to juhonurm from comment #24)
> I don't know if it makes any difference, but as far as I can tell, hardware
> buttons don't break the spoof as you described. Only the menu button does it
> reliably. The home button doesn't seem to break it at all, an the back
> button only does if it triggers a page load. That can be prevented using
> history.pushState.

hmm, I hadn't considered the history.pushState thing, that's a good point. I thought we re-showed the toolbar on application resume, but if we don't, then this is worse than I thought :/
And the thing about the menu button is, not all devices have one. On the HTC One, for example, it's behind a long press of the home button and even then only accessible if you specifically enable it.
(In reply to Chris Lord [:cwiiis] from comment #23)

> How likely is this patch to cause regressions; how much testing does it need?
> 
> I don't think this patch will cause regressions, but I think it should bake
> for a couple of weeks. There's the possibility that scrolling or zooming may
> be possible in situations where previously it wasn't, or impossible when it
> was (the latter is less likely).

This makes me leery of giving sec-approval+ now and then having it approved for Aurora and Beta with only a single beta left. 

I need release management to weigh in but this probably going to not get approval to go in until around 2/17.
If you want Aurora and Beta approval for this path, you should nominate it too or else add the appropriate patches (at least Aurora since it this goes in during Feb, Aurora will be Beta at that point).
Flags: needinfo?(release-mgmt)
(In reply to Al Billings [:abillings] from comment #28)
> If you want Aurora and Beta approval for this path, you should nominate it
> too or else add the appropriate patches (at least Aurora since it this goes
> in during Feb, Aurora will be Beta at that point).

Given comment #23 regarding "testing needed", I think this needs bake time, so we should land it mid feb to get it fixed in Firefox 28 instead. Also we have wrapped up 27 beta's at this point and goto build with RC candidate Monday.
Flags: needinfo?(release-mgmt)
sec-approval+ for landing on 2/17 or later on trunk. Once it is on trunk, nominate a patch for Aurora and Beta.
Whiteboard: [land after 2/16/14]
Attachment #8364394 - Flags: sec-approval? → sec-approval+
Go ahead with uplift nominations so we can swing by and get this next week.
Flags: needinfo?(chrislord.net)
I won't be able to handle this until Tuesday, possibly Wednesday (on PTO with no  laptop, then travelling for a work week). Leaving my n? to remind me (and this comment in case it's urgent so that someone else can handle it.)
As discussed on IRC, moving over to kats to land this while I'm on a work week.
Flags: needinfo?(chrislord.net) → needinfo?(bugmail.mozilla)
Whiteboard: [land after 2/16/14]
Comment on attachment 8364394 [details] [diff] [review]
Fix toolbar DoS possibility

[Approval Request Comment]
Bug caused by (feature/regressing bug #): fennec dynamic toolbar implementation
User impact if declined: possibility for websites to lock the user into the page and spoof the URL bar (particularly on devices with no hardware buttons)
Testing completed (on m-c, etc.): Cwiiis tested it locally i imagine. It should be tested on m-c before uplifting.
Risk to taking this patch (and alternatives if risky): if it works fine on m-c, then pretty low risk to uplift. the patch seems to apply cleanly to both aurora and beta which is a good sign. affects fennec front-end code only.
String or IDL/UUID changes made by this patch: none
Attachment #8364394 - Flags: approval-mozilla-beta?
Attachment #8364394 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ab25dcdc319b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox26: affected → wontfix
status-firefox30: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attachment #8364394 - Flags: approval-mozilla-beta?
Attachment #8364394 - Flags: approval-mozilla-beta+
Attachment #8364394 - Flags: approval-mozilla-aurora?
Attachment #8364394 - Flags: approval-mozilla-aurora+
Argh! The patch didn't have any author info so it landed with me as the author instead of Chris. I didn't bother checking that when I imported it. n00b fail.
I was overexcited in comment 18, this is not our usual sec-high severity bug so I'm giving it a more appropriate sec-moderate. It is a bad bug, though, which deserves a bug bounty.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-high → csectype-dos, sec-moderate
Depends on: 977226
Unfortunately, this had to be backed out because of some bad touch event issues that appeared (see bug 977226 and bug 978154).

https://hg.mozilla.org/integration/fx-team/rev/47ac0c7dd66e

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This one
User impact if declined: Touch event regressions breaking web pages
Testing completed (on m-c, etc.): Verified fix locally
Risk to taking this patch (and alternatives if risky): Reverts to previous state, so we'll fix the regression, but reintroduce this security bug.
String or IDL/UUID changes made by this patch: None
Attachment #8383924 - Flags: approval-mozilla-beta?
Attachment #8383924 - Flags: approval-mozilla-aurora?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8383924 [details] [diff] [review]
Backout ab25dcdc319b (bug 960146) for touch event regressions

Given the fallouts in see bug 977226 and bug 978154, let's back this out. Given its a security bug backout I've checked-in with :dveditz on this approach.
Attachment #8383924 - Flags: approval-mozilla-beta?
Attachment #8383924 - Flags: approval-mozilla-beta+
Attachment #8383924 - Flags: approval-mozilla-aurora?
Attachment #8383924 - Flags: approval-mozilla-aurora+
As noted in bug 977226, there's some useful information and a minimal test case at https://bugzilla.mozilla.org/show_bug.cgi?id=978154#c0 that should be tested before we re-land this.
If this is backed out, shouldn't the status flags for Aurora and Beta say "affected" and not "fixed" or am I misunderstanding the state?
They are affected.
I see 'affected'. Try reloading, Al?
Yeah, my bad.
We're out of time for FF28 beta landings and as this is a sec moderate that went unfixed in 27 as well, it will remain so for this release and we can look to FF29 for a proper fix.
status-firefox28: affected → wontfix
Attached patch Patch part 2 — — Splinter Review 
I think we just need to avoid talking to the gesture detectors if preventDefault has been called on a block.
Attachment #8387094 - Flags: review?(bugmail.mozilla)
Comment on attachment 8387094 [details] [diff] [review]
Patch part 2

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

Makes sense.
Attachment #8387094 - Flags: review?(bugmail.mozilla) → review+
Reassigning this to wesj, as I don't have the time to deal with this right now and it looks like he has it in hand.

A small request, any chance to get my author info added to my original patch on re-landing? :) ("Chris Lord <chrislord.net@gmail.com>")
Assignee: chrislord.net → wjohnston
Accidentally landed an empty changeset (I forgot, qimport doesn't like security bugs):

https://hg.mozilla.org/integration/fx-team/rev/28537b55795f - chris'
https://hg.mozilla.org/integration/fx-team/rev/9d936fa0d693 - empty chagest
https://hg.mozilla.org/integration/fx-team/rev/e0bef1ed08cd - wes'
https://hg.mozilla.org/mozilla-central/rev/28537b55795f
https://hg.mozilla.org/mozilla-central/rev/e0bef1ed08cd
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox30: affected → fixed
Resolution: --- → FIXED
This should probably go through the approval process again given the previous issues.
Flags: needinfo?(wjohnston)
Yeah. I wanted to give it some bake time first. Will hold this needinfo to remind me to update it in a week...
Comment on attachment 8387094 [details] [diff] [review]
Patch part 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): fennec dynamic toolbar implementation

User impact if declined: possibility for websites to lock the user into the page and spoof the URL bar (particularly on devices with no hardware buttons)

Testing completed (on m-c, etc.): Been on mc a week. No complaints. This modifies CWiii's patch slightly (and applies on top of it).

Risk to taking this patch (and alternatives if risky): Pretty low risk comparatively. Chris' patch is the hard work. This is a small fix to it.

String or IDL/UUID changes made by this patch: none
Attachment #8387094 - Flags: approval-mozilla-beta?
Attachment #8387094 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wjohnston)
Comment on attachment 8387094 [details] [diff] [review]
Patch part 2

Beta is 29 and it is fixed on Aurora, which is 30 now so the aurora approval is redundant.
Attachment #8387094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8387094 - Flags: approval-mozilla-aurora?
Confirmed bug in Fennec 29, 2014-01-05.
Verified fixed in Fennec 29 and 30, 2014-04-15.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Whiteboard: [adv-main29+]
Alias: CVE-2014-1527
Group: core-security
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: