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

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: sdna.muneaki.nishimura, Assigned: ron.waisberg)

Tracking

({sec-moderate})

Trunk
mozilla55
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 disabled, firefox53 disabled, firefox54 disabled, firefox55 fixed)

Details

(Whiteboard: currently disabled behind a pref)

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
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=breaking_news@flyweb.github.io/
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.
Reporter

Comment 1

3 years ago
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.
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Flyweb
Flags: needinfo?(kvijayan)
Product: Firefox → Core
Flags: sec-bounty?
Maybe Justin could look into this while Kannan is away? I don't know what layer of code this might be going wrong in.
Flags: needinfo?(jdarcangelo)
Reporter

Comment 3

2 years ago
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?
Flags: needinfo?(abillings)
Group: dom-core-security
Keywords: sec-moderate
Whiteboard: currently disabled behind a pref
Assigning to myself to fix.
Assignee: nobody → jdarcangelo
Status: NEW → ASSIGNED
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(kvijayan)
Assignee

Comment 5

2 years ago
Posted patch bug1326154.patch (obsolete) — Splinter Review
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).
Attachment #8841159 - Flags: review?(jdarcangelo)
Assignee

Comment 6

2 years ago
Posted patch bug1326154-fixed.patch (obsolete) — Splinter Review
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".
Attachment #8841243 - Flags: review?(jdarcangelo)
Attachment #8841159 - Flags: review?(jdarcangelo) → review-
Attachment #8841243 - Flags: review?(jdarcangelo) → review+
Assignee: jdarcangelo → ron.waisberg
Ron: See Comment 7 above and provide the necessary information to land this patch. Thanks!
Flags: needinfo?(ron.waisberg)
Assignee

Comment 9

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

Name: Ron Waisberg
Email: ron.waisberg@gmail.com
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.
Flags: needinfo?(ron.waisberg)
Attachment #8841243 - Flags: checkin?
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!
Flags: needinfo?(ron.waisberg)
Attachment #8841159 - Attachment is obsolete: true
Attachment #8841243 - Flags: checkin?
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!
Flags: needinfo?(cbook)
hm still hitting the problem ? Ryan could you take a look if you are able to checkin ? Thanks!
Flags: needinfo?(cbook) → needinfo?(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.
Flags: needinfo?(ryanvm)
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.
Flags: needinfo?(kvijayan)
Justin: can you prod this bug along, please?
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(abillings)
Posted patch fix-flyweb-bug-1326154.patch (obsolete) — Splinter Review
Updated patch that applies.
Attachment #8841243 - Attachment is obsolete: true
Flags: needinfo?(kvijayan)
Comment on attachment 8870455 [details] [diff] [review]
fix-flyweb-bug-1326154.patch

Forwarding prior r+ from justin, since this is the same patch.
Attachment #8870455 - Flags: review+
Flags: needinfo?(ron.waisberg)
Flags: needinfo?(jdarcangelo)
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.
Attachment #8870455 - Flags: checkin?
Also, please attach a patch that includes proper commit information when requesting checkin.
Attached corrected patch with author and commit message. Carrying over R+.
Attachment #8870455 - Attachment is obsolete: true
Attachment #8870520 - Flags: review+

Comment 21

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbae2c2731e4
Fix to prevent FlyWebService from pointing to a URL with a different origin. r=djvj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cbae2c2731e4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

11 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.