Added characters that look like padlocks to URL unescaping blacklist.

Added characters that look like padlocks to URL unescaping blacklist.

This blacklists the following Unicode characters:
- U+1F50F LOCK WITH INK PEN
- U+1F510 CLOSED LOCK WITH KEY
- U+1F512 LOCK
- U+1F513 OPEN LOCK

This prevents LOCK characters from appearing in a URL in the Chrome UI,
potentially looking like an SSL padlock icon (e.g., "google.com/🔒" is
now displayed as "google.com/%F0%9F%94%92"). This presented a spoofing
risk due to a few complications:
1. In RTL mode, the end of the URL (path/query) is aligned right up
   against the right edge of the Omnibox, where the SSL padlock is
   usually displayed.
2. On Mac, ChromeOS, and Android, LOCK characters are displayed in
   colour, making them more convincing.

Note: These characters will still be unescaped when using the
SPOOFING_AND_CONTROL_CHARS unescape rule (used for decoding data URLs,
previously known as CONTROL_CHARS).

BUG=495934, 421332
TBR=jam@chromium.org

Committed: https://crrev.com/7c2cbc445a81424c7df48ebe61ec4d0dcadd5dff
Cr-Commit-Position: refs/heads/master@{#335870}

[Matt Giuca, 2015-06-16 02:07:09]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-16 02:07:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180393003/1

[Matt Giuca, 2015-06-16 02:52:23]

mgiuca@chromium.org changed reviewers:
+ mmenke@chromium.org, pkasting@chromium.org

[Matt Giuca, 2015-06-16 02:52:24]

mmenke: OWNERS.
pkasting: Because this change affects the Omnibox.

Note: This is a follow-up to https://codereview.chromium.org/1158023004/ (where
I added the same characters to the IDN blacklist for domain names).

[Matt Giuca, 2015-06-16 02:53:24]

PS: You can see before/after screenshots at http://crbug.com/495934#c18

[commit-bot: I haz the power, 2015-06-16 03:10:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 03:10:55]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2015-06-16 06:36:15]

UNESCAPE_CONTROL_CHARS seems like a bit of a misnomer for unescaping these, but
I see why you keyed it off this instead of adding another enum value.

I wonder if we should name this UNESCAPE_DANGEROUS_CHARS or something.

LGTM

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode181
net/base/escape.cc:181: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
3, &second_byte))
Nit: Combine these next two conditionals.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode186
net/base/escape.cc:186: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
6, &third_byte))
Nit: And these.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode191
net/base/escape.cc:191: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
9, &fourth_byte))
Nit: And maybe these (more optional).

Also optional: Combine all the conditionals.

[Matt Giuca, 2015-06-16 07:55:34]

> I wonder if we should name this UNESCAPE_DANGEROUS_CHARS or something.

I'm happy to do that now if you'd like.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode181
net/base/escape.cc:181: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
3, &second_byte))
Do you mean

if (!...(..., &second_byte) || second_byte != 0x9f)
  return false;

?

Or do you actually mean combining these calls to UnescapeUnsignedCharAtIndex
into a single giant if statement?

I'm hesitant to do any of these changes as I feel they make the code less
readable (than simply checking each failing condition separately and returning
false at that time). Also there is the precedent of the other two functions
which I copied this one from.

I'll let Matt give an opinion here.

[Peter Kasting, 2015-06-16 08:05:08]

Renaming the enum is up to you.  DANGEROUS_CHARS is a bit vague and threatening
about what, precisely, the danger is; OTOH I don't have a better idea.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode181
net/base/escape.cc:181: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
3, &second_byte))
On 2015/06/16 07:55:34, Matt Giuca wrote:
> Do you mean
> 
> if (!...(..., &second_byte) || second_byte != 0x9f)
>   return false;

Yes, I meant this.  This reads more clearly to me than two conditionals.

> Or do you actually mean combining these calls to UnescapeUnsignedCharAtIndex
> into a single giant if statement?

That's the very last "optional" comment below; I'm not convinced this would be a
good change.

[mmenke, 2015-06-17 19:50:17]

Sorry, forgot about this one.  Split a lot of ways this week.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode181
net/base/escape.cc:181: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
3, &second_byte))
On 2015/06/16 08:05:08, Peter Kasting wrote:
> On 2015/06/16 07:55:34, Matt Giuca wrote:
> > Do you mean
> > 
> > if (!...(..., &second_byte) || second_byte != 0x9f)
> >   return false;
> 
> Yes, I meant this.  This reads more clearly to me than two conditionals.
> 
> > Or do you actually mean combining these calls to UnescapeUnsignedCharAtIndex
> > into a single giant if statement?
> 
> That's the very last "optional" comment below; I'm not convinced this would be
a
> good change.

I think 4 ifs  (1 per character is) a little easier to read than one massive if.
 Could keep the last two separate.

I don't feel strongly about any of the options here, though.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode250
net/base/escape.cc:250: // We also ban certain spoofable characters that could
be used to imitate
nit:  Should avoid "we" in comments.  Just use passive voice.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode251
net/base/escape.cc:251: // parts of the browser UI.
nit:  Mentioning the browser seems like a bit of a layering violation, though
not exactly a serious one.  Security UI?  Agent UI?

[Matt Giuca, 2015-06-22 07:27:03]

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode181
net/base/escape.cc:181: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
3, &second_byte))
Done. (Not a fan of this change, but don't want to bikeshed.)

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode186
net/base/escape.cc:186: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
6, &third_byte))
On 2015/06/16 06:36:15, Peter Kasting wrote:
> Nit: And these.

Done.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode191
net/base/escape.cc:191: if (!UnescapeUnsignedCharAtIndex(escaped_text, index +
9, &fourth_byte))
On 2015/06/16 06:36:15, Peter Kasting wrote:
> Nit: And maybe these (more optional).
> 
> Also optional: Combine all the conditionals.

Done.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode250
net/base/escape.cc:250: // We also ban certain spoofable characters that could
be used to imitate
On 2015/06/17 19:50:16, mmenke wrote:
> nit:  Should avoid "we" in comments.  Just use passive voice.

Done.

https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode251
net/base/escape.cc:251: // parts of the browser UI.
I think we want to be fairly specific here. Yes, it is a bit of a layering
violation, but these characters are banned for a fairly specific reason, which
is that web browsers commonly use similar iconography to indicate security.

(Note: I didn't say which web browser, so it's not Chrome-specific.)

[Peter Kasting, 2015-06-22 07:35:22]

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newc...
net/base/escape.cc:196: fourth_byte != 0x93)) {
Nit: Simpler:

  return UnescapeUnsignedCharAtIndex(escaped_text, index + 9, &fourth_byte) &&
      (fourth_byte == 0x8F || fourth_byte == 0x90 || fourth_byte == 0x92 ||
       fourth_byte == 0x93);

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newco...
net/base/escape.h:98: // characters (such as LOCK).
Nit: This might call out the spoofable part more:

Unescapes characters that can be used in spoofing attempts (such as LOCK) and
control characters (such as BiDi control characters and %01).  This INCLUDES
NULLs.  This is used for rare cases such as data: URL decoding where the result
is binary data.

(This suggests maybe SPOOFING_AND_CONTROL_CHARS would work as a new enum name...
although now that I read the bit about "includes nulls", I'm not as disinclined
toward my original DANGEROUS_CHARS name as I was.)

[mmenke, 2015-06-22 17:05:08]

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newc...
net/base/escape.cc:257: // be used to imitate parts of the browser UI.
On 2015/06/22 07:27:03, Matt Giuca wrote:
> I think we want to be fairly specific here. Yes, it is a bit of a layering
> violation, but these characters are banned for a fairly specific reason, which
> is that web browsers commonly use similar iconography to indicate security.
> 
> (Note: I didn't say which web browser, so it's not Chrome-specific.)

It's a layering violation for net/ to know about anything in content/ (Which
includes things like the UI thread and IO thread...as well as the fact its
primary use case is being used in a browser).

Also worth noting the network stack is used in apps other than browsers.

[Matt Giuca, 2015-06-23 04:03:19]

Patchset #4 (id:60001) has been deleted

[Matt Giuca, 2015-06-23 04:14:11]

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newc...
net/base/escape.cc:196: fourth_byte != 0x93)) {
On 2015/06/22 07:35:22, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return UnescapeUnsignedCharAtIndex(escaped_text, index + 9, &fourth_byte) &&
>       (fourth_byte == 0x8F || fourth_byte == 0x90 || fourth_byte == 0x92 ||
>        fourth_byte == 0x93);

Done.

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newc...
net/base/escape.cc:257: // be used to imitate parts of the browser UI.
OK well the entire reason for these chars being banned is that they clash with
web browser UI, so it doesn't make sense for this comment *not* to mention web
browsers. (ie. if you use this module in a non-web-browser app, then the locks
probably don't need to be banned.)

But I take your point, so I changed "parts of the browser UI" to "parts of a web
browser's UI."

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newco...
net/base/escape.h:98: // characters (such as LOCK).
OK, I'm getting that you want me to rename this ;)

I chose to call it NON_DISPLAY_CHARS. I don't like names like "DANGEROUS_CHARS"
because characters are only dangerous in certain contexts (in this case, if you
display them). Also control chars aren't dangerous per se, some of them probably
would just show as boxes. What these all have in common is that they are
characters that should not be displayed in UI (all the documentation surrounding
CONTROL_CHARS says "don't use this if you are going to display them in the UI"),
so I think NON_DISPLAY_CHARS is an appropriate name and could later be expanded
to include other things that shouldn't be displayed, like non-breaking spaces.

WDYT?

[Peter Kasting, 2015-06-23 04:35:53]

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newco...
net/base/escape.h:98: // characters (such as LOCK).
On 2015/06/23 04:14:10, Matt Giuca wrote:
> OK, I'm getting that you want me to rename this ;)
> 
> I chose to call it NON_DISPLAY_CHARS. I don't like names like
"DANGEROUS_CHARS"
> because characters are only dangerous in certain contexts (in this case, if
you
> display them).

The whole point of this function is to prepare a URL for display, so this
objection seems a bit odd.

> Also control chars aren't dangerous per se, some of them probably
> would just show as boxes.

Embedded control characters could change the appearance and function of the URL
either in Chrome or in other applications you copied and pasted the URL to.  I
don't think "characters might show as boxes" is the limit of the danger here,
even in the omnibox.

> What these all have in common is that they are
> characters that should not be displayed in UI (all the documentation
surrounding
> CONTROL_CHARS says "don't use this if you are going to display them in the
UI"),
> so I think NON_DISPLAY_CHARS is an appropriate name and could later be
expanded
> to include other things that shouldn't be displayed, like non-breaking spaces.
> 
> WDYT?

I don't like NON_DISPLAY_CHARS (sorry...).  I think it reads as "non-displaying
characters", e.g. control characters, zero-width characters, soft hyphens, etc. 
But these banned characters are displaying characters.

I think either of my original suggestions is clearer.

[Matt Giuca, 2015-06-23 06:21:25]

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newco...
net/base/escape.h:98: // characters (such as LOCK).
On 2015/06/23 04:35:53, Peter Kasting wrote:
> The whole point of this function is to prepare a URL for display, so this
> objection seems a bit odd.

No it isn't, it's used about equally for the two purposes of a) preparing a URL
for display, and b) decoding binary data that was encoded in a URL (like data:
URLs). The distinction at the call site is that in case (a) you do not use
CONTROL_CHARS, and in case (b), you do use CONTROL_CHARS.

So it seemed appropriate to rename CONTROL_CHARS to NON_DISPLAY_CHARS, for times
where you are not preparing a URL for display.

> I don't like NON_DISPLAY_CHARS (sorry...).  I think it reads as
"non-displaying
> characters", e.g. control characters, zero-width characters, soft hyphens,
etc. 
> But these banned characters are displaying characters.

Fair enough.

> I think either of my original suggestions is clearer.

I still don't like DANGEROUS_CHARS because it implies that a client using this
function for use case (b) (decoding a data URL) is doing something dangerous.
Whereas in fact it is perfectly safe as long as you don't display the decoded
string in a context of a URL.

Let's go with SPOOFING_AND_CONTROL_CHARS then.

[Peter Kasting, 2015-06-23 06:55:15]

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newco...
net/base/escape.h:98: // characters (such as LOCK).
On 2015/06/23 06:21:24, Matt Giuca wrote:
> On 2015/06/23 04:35:53, Peter Kasting wrote:
> > The whole point of this function is to prepare a URL for display, so this
> > objection seems a bit odd.
> 
> No it isn't, it's used about equally for the two purposes of a) preparing a
URL
> for display, and b) decoding binary data that was encoded in a URL (like data:
> URLs). The distinction at the call site is that in case (a) you do not use
> CONTROL_CHARS, and in case (b), you do use CONTROL_CHARS.

How I think of it: That second case _is_ preparing the URL for display.  You're
prepping a data: URL for display as data, to be displayed to the user in the
content area.

Semantics :P

> Let's go with SPOOFING_AND_CONTROL_CHARS then.

SGTM.

[mmenke, 2015-06-23 15:22:55]

LGTM

[Matt Giuca, 2015-06-24 00:57:22]

mgiuca@chromium.org changed reviewers:
+ jam@chromium.org

[Matt Giuca, 2015-06-24 00:57:23]

jam@chromium.org: TBR for mechanical changes (renaming constant).

[Matt Giuca, 2015-06-24 00:57:32]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-06-24 00:57:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1180393003/#ps120001
(title: "Rename NON_DISPLAY_CHARS to SPOOFING_AND_CONTROL_CHARS.")

[commit-bot: I haz the power, 2015-06-24 00:59:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180393003/120001

[commit-bot: I haz the power, 2015-06-24 02:36:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 02:36:15]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Matt Giuca, 2015-06-24 03:18:48]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-06-24 03:19:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180393003/120001

[commit-bot: I haz the power, 2015-06-24 03:59:57]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-06-24 04:00:43]

Patchset 6 (id:??) landed as
https://crrev.com/7c2cbc445a81424c7df48ebe61ec4d0dcadd5dff
Cr-Commit-Position: refs/heads/master@{#335870}