Allow untrusted events to trigger a link

VERIFIED FIXED in Firefox 6

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla7
regression
Points:
---
Dependency tree / graph
Bug Flags:
sec-review +

Firefox Tracking Flags

(firefox6+ fixed, firefox7+ fixed)

Details

([landed m-c 7/06] [semi-widespread Web regression, cmnt #66-7])

Attachments

(5 attachments, 4 obsolete attachments)

Created attachment 541396 [details]
testcase

Webkit and Opera do this. Haven't tested on IE.
Assignee: Olli.Pettay → dzbarsky
The trustness check was added in Bug 265176.
Blocks: 265176
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Which was fixed before Bug 289940, which would have fixed it too, I think.
Blocks: 666985
With bug 583514 fixed, I think we need to do this for web compat.  Lots of sites seem to expect that click() on an <a>, if present, will trigger the link, and it doesn't right now due to this bug...

I don't think there's really anything you can do with click events on anchors that you can't do by setting location.href or calling window.open, from a security perspective.  We'd need to check that you can't bypass the popup blocker with it, I guess.
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
Blocks: 667632
David, if you're busy with other things, assign this to me.
Assignee: dzbarsky → Olli.Pettay

Comment 5

6 years ago
Looks like this is causing bug 666955 / bug 666674, which breaks some key functionality in a commonly used Ajax / web control library for ASP.NET (http://www.telerik.com/products/aspnet-ajax/ajax.aspx).

For my company, this means we will likely have to recommend our customers not to use Firefox 5 with our multi-million user educational intranet application.  I suspect the RadAjax controls controls are fairly common on corporate intranet applications.  I'd therefore strongly vote for a 5.01 fix, but I know that Mozilla doesn't care about corporates :-).
(I don't know why you think Mozilla doesn't care about corporates.
 If some mozillian says something like that, it doesn't necessarily mean
 everyone in the project agrees with that.)


Anyway, patch coming, and I certainly think this must be fixed for Fx6, which
will be released in August.
Created attachment 542772 [details] [diff] [review]
patch

Uploaded to tryserver. We may have tests which check that 
untrusted events *don't* active links.
Created attachment 542801 [details] [diff] [review]
patch
Attachment #542772 - Attachment is obsolete: true
Blocks: 666955, 666674

Comment 9

6 years ago
> We'd need to check that you can't bypass the popup blocker with it, I guess.

The test should probably cover both synthetic ctrl+click and synthetic normal clicks on links with target=_blank.

Similarly, I don't want sites to be able to synthesize alt+click and cause Firefox to download something it normally wouldn't download.
The patch allows synthetic clicks to trigger link only when
!ctrl && !alt && !meta && !shift.
And the test does cover the ctrl case (alt, meta and shift are handled the same way)

I'm not sure how we should handle target="_blank" .
The patch gives the same _blank handling what Chrome has.

Comment 12

6 years ago
What is Chrome's behavior?
A new tab is opened, even if click is dispatched using a timer.

Comment 14

6 years ago
Let's not do that.
> I'm not sure how we should handle target="_blank" .

The same way as window.open(), I would think....  Why is the popup blocker not kicking in?
In particular, OnLinkClickEvent::OnLinkClickEvent saves the popup control state, and then pushes it before doing OnLinkClickSync.  So I'd expect the popup blocker to work here...
Ah, Chrome's behavior depends on whether the test is local file or not.
If not, _blank pages aren't opened.
Better patch coming.
Hmm, no, the problem wasn't what I was thinking about.
A testcase with some timers http://mozilla.pettay.fi/moztests/a_click.html
Created attachment 542881 [details] [diff] [review]
patch

Push the cx to stack if untrusted event triggers the link.
With that popup blocker works the same way as with window.open() when
target="_blank"
Attachment #542801 - Attachment is obsolete: true
Attachment #542881 - Flags: review?(bzbarsky)
Pushed to try.
(In reply to comment #17)
> Ah, Chrome's behavior depends on whether the test is local file or not.
> If not, _blank pages aren't opened.
Hmm, I'm not sure about this anymore. _blank is opened sometimes, but
not always.
Just to clarify, comment 23 was about Chrome.
I would really prefer a required argument to TriggerLink and for that matter on OnLinkClick.  For the latter, there are only two callers, and one of them already passes all the args anyway and you're changing the other one.

Also, would it make more sense to make the booleand "aIsTrusted"?  There would be fewer negatives in the code that way, I think....
Oh, to be clear in general this looks fine.
Created attachment 542904 [details] [diff] [review]
v4
Attachment #542881 - Attachment is obsolete: true
Attachment #542904 - Flags: review?(bzbarsky)
Attachment #542881 - Flags: review?(bzbarsky)
Comment on attachment 542904 [details] [diff] [review]
v4

r=me
Attachment #542904 - Flags: review?(bzbarsky) → review+
Compiling now a patch for Fx5 (with nsILinkHandler_5_0 so that there is no
need for an interface change)
Attachment #542904 - Flags: checkin?

Updated

6 years ago
Keywords: regression
Created attachment 542911 [details] [diff] [review]
For fx5
Attachment #542911 - Flags: review?(bzbarsky)
Keywords: checkin-needed
Comment on attachment 542911 [details] [diff] [review]
For fx5

r=me
Attachment #542911 - Flags: review?(bzbarsky) → review+
Are we really taking patches for Firefox 5? I though Firefox 6 will be the update.
There's discussion that we might do a Firefox 5 respin *for this bug*.
(In reply to comment #33)
> There's discussion that we might do a Firefox 5 respin *for this bug*.

Oh sorry, I didn't know.
Technically this is not a regression, or it is a very old regression.
Some web pages just happen to rely click() to do one thing (dispatch click event 
as we do), and untrusted click to activate link (which we haven't done since Bug 
265176).
Created attachment 543104 [details] [diff] [review]
param rename

The previous patch for trunk missed a parameter rename.
Attachment #542904 - Flags: checkin?
Attachment #543104 - Flags: checkin?
(In reply to comment #35)
> Technically this is not a regression, or it is a very old regression.

I see dates like 2004.  Is this a regression from 4-5?  If it's that old I doubt we need to spin a 5.0.1 for this.
The issue is that we added .click to <a>, and that click() dispatches
untrusted click, like it should, but does not trigger the link.

And for unknown reasons web pages have code like
if (a.click) {
  a.click();
} else {
  window.location = a.href;
}
The behavior in this bug is not new.

Sites exercising this codepath is new, and is a regression from 4 to 5.
OK, thanks.
(In reply to comment #32)
> Are we really taking patches for Firefox 5? I though Firefox 6 will be the
> update.

Firefox 6 is the "scheduled" update. There's always the possibility of an interim release ("chemspill") if we broke the web. It's expensive (in time and resources) so we have to balance how broken vs. how soon a planned fix can come.
I put this on the agenda for today's Aurora meeting since that's the right audience.  First time someone has wanted a chemspill for compatibility.
Keywords: sec-review-needed
http://hg.mozilla.org/mozilla-central/rev/7deef688bbf5

Comment 44

6 years ago
This seems like the kind of patch that could have unintended compatibility effects, too.  Could we back out bug 583514 for Firefox 5.0.x instead?
No. It changed interfaces.
tracking-firefox6+ per triage meeting

Does comment 43 mean this is no longer checkin-needed and should be RESOLVED-FIXED?  If not, what is checkin-needed?

Please make an approval-aurora request for the appropriate patch so this can get into Aurora soon.
tracking-firefox6: ? → +
Comment on attachment 542911 [details] [diff] [review]
For fx5

We could take this patch to aurora.
Attachment #542911 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
Attachment #543104 - Flags: checkin?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 668223

Comment 48

6 years ago
If a modifier key is physically pressed and then the link is triggered by an untrusted event, does the link act as if the key was pressed or not?
It acts as if the key was not pressed.

Updated

6 years ago
Blocks: 666769

Updated

6 years ago
tracking-firefox7: ? → +

Comment 50

6 years ago
Comment on attachment 542911 [details] [diff] [review]
For fx5

Please re-request approval on the patch after all reviews are completed. (Security review still pending.)
Attachment #542911 - Flags: approval-mozilla-aurora?

Comment 51

6 years ago
test7() seems backwards. It should be testing that the link does *not* open (at least not in a new window).  Why is it passing?
test7 relies on dom.disable_open_during_load to be false,
and that is set false for mochitests.
(we have way too many prefs for popup handling, and the naming of them isn't
quite right).

I'll add a test for dom.disable_open_during_load=true
Created attachment 544189 [details] [diff] [review]
+additional test
Attachment #544189 - Flags: review?(mounir)
Comment on attachment 544189 [details] [diff] [review]
+additional test

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

Could you explicitly set dom.disable_open_during_load to false in test7?
In addition, I do not really like the setTimeout(test9, 1000). Can't you use the click event and go X time to the event loop to assume nothing happened?
(In reply to comment #54)
> Could you explicitly set dom.disable_open_during_load to false in test7?
Why?


> In addition, I do not really like the setTimeout(test9, 1000). Can't you use
> the click event and go X time to the event loop to assume nothing happened?
I don't understand what you mean. X time to the event loop?
Created attachment 544209 [details] [diff] [review]
+additional test
Attachment #544189 - Attachment is obsolete: true
Attachment #544209 - Flags: review?(mounir)
Attachment #544189 - Flags: review?(mounir)
Comment on attachment 544209 [details] [diff] [review]
+additional test

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

::: content/base/test/test_bug666604.html
@@ +137,5 @@
> +      ok(false, "Click() should not activate a link");
> +      setTimeout(test9, 0);
> +    }
> +  testlink.click();  
> +  hitEventLoop(10, test9);

I would have make gActivated being a variable, testlink.href setting it to "true" and hitEventLoop calling:
function() {
  ok(!gActivated, "...");
  gActivated = false;
  test9();
}

Though, what you did should work too.
Attachment #544209 - Flags: review?(mounir) → review+
http://hg.mozilla.org/mozilla-central/rev/eb4d059af819

Comment 59

6 years ago
Does not work in firefox 6 beta,  confirmed to work in the Aurora version.
That would be a neat trick, since this fix didn't land in aurora yet.

Olli, is there a security review scheduled for this at least?
status-firefox6: --- → affected
status-firefox7: --- → affected
(In reply to comment #60)
> That would be a neat trick, since this fix didn't land in aurora yet.

It landed before the merge, no?
Oh, the testcase is what landed after the merge.  Nevermind me....
status-firefox7: affected → ---
Target Milestone: --- → mozilla7
I did ask for a sec review. No idea when it could happen.
status-firefox7: --- → fixed
sec-review-complete, fix is limited in scope to links (as opposed to changing events themselves) and doesn't regress old related security bugs.

Didn't intend for "sec-review-needed" to block landing (we do many reviews post-landing), would have put an explicit review request on the patch if I did.
Keywords: sec-review-needed → sec-review-complete
Attachment #542911 - Flags: approval-mozilla-beta?
Discussed this request in approval triage today. Late-landing this in the beta cycle, which is supposed to be exclusively for backouts of bustage caused in this branch feels like a bad idea, but blizzard thinks that we need to be willing to make exceptions for major web compatibility fixes.

We in the room couldn't get a good sense of how widespread this bustage was, though - it feels like a weird pattern to synthesize click events on links, but we know one framework seems to use it. Smaug, bz - any intuitions here on it being more widespread than we think? Otherwise it feels like letting people get this fix in 7 is the right choice, since we don't take changes on aurora/beta that aren't current-version-bustage-fixes.
calling <a>.click() doesn't cause us to follow the link.
In Fx4 we didn't have <a>.click();

For some mysterious reason code mentioned in comment 38 is used quite often.

Also, see all the bugs this bug fixed.
Johnathan, we have 6 independent reports of this issue, from sites using at least 3 different frameworks that all have essentially the same code, structured like comment 38.  It seems to be a common workaround for a bug in some old browser (old IE version, I'm guessing based on other code involved) that doesn't send a referrer on location.href sets.  As a result, sites that want to track the referrer do this hack with .click if it's present and setting location if it's not present (this last presumably to work with gecko-based browsers).

So yes, it's a weird pattern, but it's being done for very logical reasons, and the usage seems to be somewhat widespread.

Updated

6 years ago
Blocks: 670528

Updated

6 years ago
Whiteboard: [landed m-c 7/06] [semi-widespread Web regression, cmnt #66-7]

Updated

6 years ago
Attachment #542911 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
http://hg.mozilla.org/releases/mozilla-beta/rev/084847ea02b4
http://hg.mozilla.org/releases/mozilla-beta/rev/daef8d0f83b0
status-firefox6: affected → fixed
Blocks: 395917

Comment 69

6 years ago
In terms of working around the issue in 5.x, what would be the best way to detect if a given event object will actually be allowed to open a new link without resorting to browser + version detection?

Previously I relied on event.isTrusted === false but it seems this new fix retains synthetic events as untrusted but simply changes their policy, so now what? :/
> what would be the best way to detect if a given event object will actually be
> allowed to open a new link

Condition it off event.isTrusted after testing via object-detection (e.g. by using an untrusted click event in a hidden iframe) whether untrusted events are allowed to trigger links?

There's really no good way to object-detect security policy without trying to do something that the policy might disallow and seeing whether it's allowed, in general.

Comment 71

6 years ago
Version detection may be the lesser evil here then - it seems to be a fairly moz-specific peculiarity at least
Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110810 Firefox/8.0a1
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Win XP, Win 7, Mac OS X 10.7 and Ubuntu 11.04 using the test case attached in the description. 

Setting resolution to VERIFIED FIXED.
Status: RESOLVED → VERIFIED

Updated

6 years ago
Depends on: 684208
Flags: sec-review+

Updated

4 years ago
Depends on: 812202
You need to log in before you can comment on or make changes to this bug.