Closed
Bug 1146305
Opened 9 years ago
Closed 9 years ago
IDN links should be properly displayed in Hello rooms for context in conversations
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox40 fixed)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: shahmeerbond, Assigned: standard8)
References
Details
(Whiteboard: [context])
Attachments
(1 file, 2 obsolete files)
17.36 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36 OPR/27.0.1689.76 Steps to reproduce: The IDN: http://ebаy.com/ is a homograph for the latin ebay.com. if you click that first link, youm might think that you are going to ebay.com. in fact, you are going to a homograph url http://xn--eby-7cd.com/ I pasted it in the hello.firefox.com conversation panel Actual results: The IDN was not decoded into it's correct homograph due to which an attacker can misuse this to perform an open redirect on the field Expected results: The IDN should have been decoded properly and the Homograph should displayed in it's correct format that is http://xn--eby-7cd.com/
Comment 1•9 years ago
|
||
(In reply to Muhammad Shahmeer from comment #0) > I pasted it in the hello.firefox.com conversation panel > > > Actual results: > > The IDN was not decoded into it's correct homograph due to which an attacker > can misuse this to perform an open redirect on the field By "pasted it in the hello.firefox.com conversation panel" do you mean that you put it into the "Room name" field? AFAIK that is the only place to enter text that is shared with the other room participant in Hello currently. That field does not allow links and is not "clickable" for the other room participant. So I'm not sure what you mean by "perform an open redirect on the field". Can you elaborate?
Component: Untriaged → Client
Product: Firefox → Loop
Version: Firefox 38 → unspecified
Updated•9 years ago
|
Flags: sec-bounty?
Reporter | ||
Comment 2•9 years ago
|
||
Yes i know the links are not highlighted. But they are not decoded as well on the application level. And yes i mean the same thing that i enter the text in the same field\\
Comment 3•9 years ago
|
||
OK, so it sounds like this is not actually an "attack" in any sense of the word. You can display potentially misleading URLs in the "room name" field, but once they are navigated to they are displayed as we would otherwise. We will be adding URL fields to rooms in the future, and when we do so we should be sure to show them properly.
Group: core-security
Summary: IDN homograph attack Firefox Hello → IDN links should be properly displayed in Hello rooms
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty? → sec-bounty-
Reporter | ||
Comment 4•9 years ago
|
||
Boutny?
Comment 5•9 years ago
|
||
(In reply to Muhammad Shahmeer from comment #4) > Boutny? This isn't a security bug and has no risk so there is no bounty for this.
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Comment hidden (abuse) |
Comment hidden (abuse) |
Comment hidden (abuse) |
Comment hidden (abuse) |
Comment hidden (abuse) |
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Assignee | ||
Updated•9 years ago
|
Blocks: 1115340
Rank: 6
OS: Windows 8.1 → All
Priority: -- → P1
Hardware: x86_64 → All
Summary: IDN links should be properly displayed in Hello rooms → IDN links should be properly displayed in Hello rooms for context in conversations
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → standard8
Iteration: --- → 40.3 - 11 May
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Comment 16•9 years ago
|
||
This does some formatting of the links in the new context work that's in progress. It uses the browsers new URL to handle punycode automatically - if we feed in a IDN based url into new URL it formats it to punycode/ACE if applicable. Hence we don't need to include nor maintain a library for it. I've also added in decoding of URIs as well - at the moment I'm assuming we want the full url on the standalone (due to where it might point on a site), and hence, we don't want %20 etc appearing in the url. Note: I've done the conversion in the desktop conversation window as well. We should in future be using this for the link-clicker, so I think it is reasonable to implement it now (arguably, we want it for the decoding anyway).
Attachment #8598000 -
Flags: review?(mdeboer)
Assignee | ||
Updated•9 years ago
|
Whiteboard: [context]
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8598000 [details] [diff] [review] IDN links should be properly displayed in Hello rooms for context in conversations. Following today's UX demo review, I need to make a couple of adjustments, so I might as well do that before this gets reviewed.
Attachment #8598000 -
Flags: review?(mdeboer)
Assignee | ||
Comment 18•9 years ago
|
||
Updated version that only displays the domain names directly (the full urls are available as tooltips) as per the UX review.
Attachment #8598000 -
Attachment is obsolete: true
Attachment #8598573 -
Flags: review?(mdeboer)
Comment 19•9 years ago
|
||
Comment on attachment 8598573 [details] [diff] [review] IDN links should be properly displayed in Hello rooms for context in conversations. Review of attachment 8598573 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! The only reason why it's r- is because the changes I propose below are substantial enough to warrant a final check. ::: browser/components/loop/content/shared/js/utils.js @@ +276,5 @@ > + * domain to punycode, and then decoding the url. > + * > + * @param {String} url The url to format. > + * @return {String} The formatted url. > + * @throws May throw if "new URL()" fails. Well, that's not how it's used :) @@ +278,5 @@ > + * @param {String} url The url to format. > + * @return {String} The formatted url. > + * @throws May throw if "new URL()" fails. > + */ > + function formatURLForDisplay(url) { I *think* `formatURL` would do it too for me... @@ +283,5 @@ > + // We're using new URL to pass this through the browser's ACE/punycode > + // processing system. If the browser considers a url to need to be > + // punycode encoded for it to be displayed, then new URL will do that for > + // us. This saves us needing our own punycode library. > + var urlObject = new URL(url); I'd like to suggest putting this in a try...catch block: ```js var urlObject; try { urlObject = new URL(url); } catch (ex) { console.error("Error occurred whilst parsing URL:", ex); return null; } ... ``` @@ +286,5 @@ > + // us. This saves us needing our own punycode library. > + var urlObject = new URL(url); > + > + // Finally, ensure we look good. > + return {hostname: urlObject.hostname, location: decodeURI(urlObject.href)}; Since this object literal consists of multiple props, can you make it span multiple lines? ::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx @@ +255,5 @@ > + locationInfo = sharedUtils.formatURLForDisplay(this.props.roomContextUrl.location); > + } catch (ex) { > + // If we can't decode the URL for some reason, don't display anything. > + return null; > + } So this would become: ```js var locationInfo = sharedUtils.formatURL(this.props.roomContextUrl.location); if (!locationInfo) { return null; } ... ``` ::: browser/components/loop/test/standalone/standaloneRoomViews_test.js @@ +105,5 @@ > + > + var view = mountTestComponent({ > + roomName: "Mike's room", > + roomContextUrls: [{ > + description: "Mark's super page", lol
Attachment #8598573 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 20•9 years ago
|
||
Updated for review comments.
Attachment #8598573 -
Attachment is obsolete: true
Attachment #8599244 -
Flags: review?(mdeboer)
Comment 21•9 years ago
|
||
Comment on attachment 8599244 [details] [diff] [review] IDN links should be properly displayed in Hello rooms for context in conversations. v2 Review of attachment 8599244 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good! ::: browser/components/loop/test/shared/utils_test.js @@ +162,5 @@ > + .eql({ > + location: "http://xn--oogle-qmc.com/", > + hostname: "xn--oogle-qmc.com" > + }); > + }) nit: missing semicolon. @@ +163,5 @@ > + location: "http://xn--oogle-qmc.com/", > + hostname: "xn--oogle-qmc.com" > + }); > + }) > + }); Can you land this with a testcase that covers passing in a malformed URL where `formatURL` returns `null`?
Attachment #8599244 -
Flags: review?(mdeboer) → review+
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/511678a783bc
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 24•9 years ago
|
||
Setting qe-verify- flag, since this seems to be covered by automated tests. Please add back if Manual QA is specifically needed for this fix.
Flags: qe-verify+ → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•