1326154 - The uiUrl option of navigator.publishServer allows to launch arbitrary URL

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Flyweb, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.3.12 (KHTML, like Gecko) Version/10.0.2 Safari/602.3.12

Steps to reproduce:

1) Enable FlyWeb (dom.flyweb.enabled=true)
2) Open http://mallory.csrf.jp/flyweb.html
3) Push one of the button to activate a FlyWeb service
4) Launch nearby FlyWeb service called 'foo' from FlyWeb menu on Nightly


Actual results:

'uiUrl' option of navigator.publishServer allows to set an arbitrary URL including chrome:, file:, and about:.
That URL is shown when a victim or other persons in the same local network as the victim try to connect the rogue FlyWeb service.

The 3rd button of the above PoC exploits an address bar spoofing vector in about:reader by accessing following URL.
about:reader?url=http://cnn.example.com%26story=[e-mail]/
The current implementation of about:reader is not unsafe but it's prone to similar kind of attacks.


Expected results:

URL with http: or https: schema should be allowed to be set.

Comment 1

[Muneaki Nishimura]

Attachment: Evil MDNS packet including javascript URL in the PATH key TXT record

Further investigation revealed that FlyWeb enabled Firefox opens an arbitrary URL sent through a MDNS discovery packet including malicious PATH key in the TXT record. The attachment picture shows a MDNS packet including javascript URL in the PATH key.

Comment 2

[Andrew McCreight [:mccr8]]

Maybe Justin could look into this while Kannan is away? I don't know what layer of code this might be going wrong in.

Comment 3

[Muneaki Nishimura]

As I mentioned in an e-mail last year FlyWeb is still in feasibility study and it's not landed on Aurora or Beta. Also it's disabled by default even on Nightly.
So I think there would be no risks if the bug is made unhidden.
How do ynu think about it and can you open the bug?

Comment 4

[Justin D'Arcangelo [:justindarc]]

Assigning to myself to fix.

Comment 5

[ron.waisberg]

Attachment: bug1326154.patch

When NS_NewURI() is passed a `uriString` that is an absolute URI, the `baseURI` parameter is ignored and not prepended (http://i.imgur.com/W4N4mOe.png).

Comment 6

[ron.waisberg]

Attachment: bug1326154-fixed.patch

Updated to avoid extraneous forward slash when mPath was something like "/controls.html" (would result in a spec of "http://<UUID>//controls.html") as well as adding a necessary forward slash when mPath was missing it e.g. "controls.html".

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please attach that includes appropriate commit information when requesting checkin (name, email, commit message).
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 8

[Justin D'Arcangelo [:justindarc]]

Ron: See Comment 7 above and provide the necessary information to land this patch. Thanks!

Comment 9

[ron.waisberg]

Comment on attachment 8841243 [details] [diff] [review]
bug1326154-fixed.patch

Name: Ron Waisberg
Email: [e-mail]
Commit Message: Fixed improper building of URL by concatenating mHostname with mPath. Previously, the user provided parameter mPath was passed into NS_NewURI() which would ignore and not concatenate the baseURI if mPath was an absolute URI.

Comment 10

[Carsten Book [:Tomcat]]

seems this need a rebase now since i hit conflicts:

patching file dom/flyweb/FlyWebService.cpp
Hunk #1 FAILED at 784
1 out of 1 hunks FAILED -- saving rejects to file dom/flyweb/FlyWebService.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1326154-fixed.patch

could you take a look, thanks!

Comment 11

[Justin D'Arcangelo [:justindarc]]

Carsten, I am able to apply bug1326154-fixed.patch to inbound without any issues. Also, the affected file (dom/flyweb/FlyWebService.cpp) has not changed for several months so it shouldn't be a rebasing issue. Can you please re-check this? Thanks!

(In reply to Carsten Book [:Tomcat] from comment #10)
> seems this need a rebase now since i hit conflicts:
> 
> patching file dom/flyweb/FlyWebService.cpp
> Hunk #1 FAILED at 784
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/flyweb/FlyWebService.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh bug1326154-fixed.patch
> 
> could you take a look, thanks!

Comment 12

[Carsten Book [:Tomcat]]

hm still hitting the problem ? Ryan could you take a look if you are able to checkin ? Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply for me either. Are you working from a git repo? I'm wondering if the patch format is confusing Mercurial for some reason.

Comment 14

[Justin D'Arcangelo [:justindarc]]

Kannan, can you take a look at why Ron's patch isn't applying? I was able to apply to m-c using mq, but the sheriffs have not been able to. I'm not sure what the issue is.

Comment 15

[Daniel Veditz [:dveditz]]

Justin: can you prod this bug along, please?

Comment 16

[Kannan Vijayan [:djvj]]

Attachment: fix-flyweb-bug-1326154.patch

Updated patch that applies.

Comment 17

[Kannan Vijayan [:djvj]]

Comment on attachment 8870455 [details] [diff] [review]
fix-flyweb-bug-1326154.patch

Forwarding prior r+ from justin, since this is the same patch.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8870455 [details] [diff] [review]
fix-flyweb-bug-1326154.patch

In the future, you can just set the checkin-needed bug keyword, which plays more nicely with the automated bug marking tools.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Also, please attach a patch that includes proper commit information when requesting checkin.

Comment 20

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1326154.patch

Attached corrected patch with author and commit message. Carrying over R+.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbae2c2731e4
Fix to prevent FlyWebService from pointing to a URL with a different origin. r=djvj

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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