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)

defect
Points:
3

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: shahmeerbond, Assigned: standard8)

References

Details

(Whiteboard: [context])

Attachments

(1 file, 2 obsolete files)

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/
(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
Flags: sec-bounty?
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\\
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty? → sec-bounty-
Boutny?
(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.
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: nobody → standard8
Iteration: --- → 40.3 - 11 May
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
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)
Whiteboard: [context]
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)
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 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-
Updated for review comments.
Attachment #8598573 - Attachment is obsolete: true
Attachment #8599244 - Flags: review?(mdeboer)
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+
https://hg.mozilla.org/mozilla-central/rev/511678a783bc
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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.