Added characters that look like padlocks to URL unescaping blacklist.

net/base/escape.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/escape.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 146 matching lines @@
   if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 6, &third_byte))
     return false;
   if (second_byte == 0x80) {
     return third_byte == 0x8E ||
            third_byte == 0x8F ||
            (third_byte >= 0xAA && third_byte <= 0xAE);
   }
   return third_byte >= 0xA6 && third_byte <= 0xA9;
 }
 
+// Returns true if there is a four-byte banned char at |index|. |first_byte| is
+// the byte at |index|.
+template <typename STR>
+bool HasFourByteBannedCharAtIndex(const STR& escaped_text,
+                                  unsigned char first_byte,
+                                  size_t index) {
+  // The following characters are blacklisted for spoofability concerns.
+  // U+1F50F LOCK WITH INK PEN         (%F0%9F%94%8F)
+  // U+1F510 CLOSED LOCK WITH KEY      (%F0%9F%94%90)
+  // U+1F512 LOCK                      (%F0%9F%94%92)
+  // U+1F513 OPEN LOCK                 (%F0%9F%94%93)
+  if (first_byte != 0xF0)
+    return false;
+  unsigned char second_byte;
+  if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 3, &second_byte))

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

Nit: Combine these next two conditionals.

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

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]

Yes, I meant this.  This reads more clearly to me than two conditionals.
That's the very last "optional" comment below; I'm not convinced this would be a
good change.

[mmenke, 2015/06/17 19:50:16]

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.

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

Done. (Not a fan of this change, but don't want to bikeshed.)

+    return false;
+  if (second_byte != 0x9F)
+    return false;
+  unsigned char third_byte;
+  if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 6, &third_byte))

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

Nit: And these.

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

Done.

+    return false;
+  if (third_byte != 0x94)
+    return false;
+  unsigned char fourth_byte;
+  if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 9, &fourth_byte))

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

Nit: And maybe these (more optional).

Also optional: Combine all the conditionals.

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

Done.

+    return false;
+  return fourth_byte == 0x8F || fourth_byte == 0x90 || fourth_byte == 0x92 ||
+         fourth_byte == 0x93;
+}
+
 // Unescapes |escaped_text| according to |rules|, returning the resulting
 // string.  Fills in an |adjustments| parameter, if non-NULL, so it reflects
 // the alterations done to the string that are not one-character-to-one-
 // character.  The resulting |adjustments| will always be sorted by increasing
 // offset.
 template<typename STR>
 STR UnescapeURLWithAdjustmentsImpl(
     const STR& escaped_text,
     UnescapeRule::Type rules,
     base::OffsetAdjuster::Adjustments* adjustments) {
@@ skipping 33 matching lines @@
       // Additionally, the Unicode Technical Report (TR9) as referenced by RFC
       // 3987 above has since added some new BiDi control characters.
       // http://www.unicode.org/reports/tr9
       //
       // U+061C ARABIC LETTER MARK         (%D8%9C)
       // U+2066 LEFT-TO-RIGHT ISOLATE      (%E2%81%A6)
       // U+2067 RIGHT-TO-LEFT ISOLATE      (%E2%81%A7)
       // U+2068 FIRST STRONG ISOLATE       (%E2%81%A8)
       // U+2069 POP DIRECTIONAL ISOLATE    (%E2%81%A9)
       //
+      // We also ban certain spoofable characters that could be used to imitate

[mmenke, 2015/06/17 19:50:16]

nit:  Should avoid "we" in comments.  Just use passive voice.

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

Done.

+      // parts of the browser UI.

[mmenke, 2015/06/17 19:50:16]

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]

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.)

+      //
+      // U+1F50F LOCK WITH INK PEN         (%F0%9F%94%8F)
+      // U+1F510 CLOSED LOCK WITH KEY      (%F0%9F%94%90)
+      // U+1F512 LOCK                      (%F0%9F%94%92)
+      // U+1F513 OPEN LOCK                 (%F0%9F%94%93)
+      //
       // However, some schemes such as data: and file: need to parse the exact
       // binary data when loading the URL. For that reason, CONTROL_CHARS allows
       // unescaping BiDi control characters.
       // DO NOT use CONTROL_CHARS if the parsed URL is going to be displayed
       // in the UI.
       if (!(rules & UnescapeRule::CONTROL_CHARS)) {
         if (HasArabicLanguageMarkAtIndex(escaped_text, first_byte, i)) {
           // Keep Arabic Language Mark escaped.
           result.append(escaped_text, i, 6);
           i += 5;
           continue;
         }
         if (HasThreeByteBidiControlCharAtIndex(escaped_text, first_byte, i)) {
           // Keep BiDi control char escaped.
           result.append(escaped_text, i, 9);
           i += 8;
           continue;
         }
+        if (HasFourByteBannedCharAtIndex(escaped_text, first_byte, i)) {
+          // Keep banned char escaped.
+          result.append(escaped_text, i, 12);
+          i += 11;
+          continue;
+        }
       }
 
       if (first_byte >= 0x80 ||  // Unescape all high-bit characters.
           // For 7-bit characters, the lookup table tells us all valid chars.
           (kUrlUnescape[first_byte] ||
            // ...and we allow some additional unescaping when flags are set.
            (first_byte == ' ' && (rules & UnescapeRule::SPACES)) ||
            // Allow any of the prohibited but non-control characters when
            // we're doing "special" chars.
            (first_byte > ' ' && (rules & UnescapeRule::URL_SPECIAL_CHARS)) ||
@@ skipping 206 matching lines @@
                        1, kEscapeToChars[i].replacement);
           break;
         }
       }
     }
   }
   return text;
 }
 
 }  // namespace net