Don't unescape BiDi control characters in URL components

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 79 matching lines @@
 //   @  A  B  C  D  E  F  G  H  I  J  K  L  M  N  O
      0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 //   P  Q  R  S  T  U  V  W  X  Y  Z  [  \  ]  ^  _
      1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 1,
 //   `  a  b  c  d  e  f  g  h  i  j  k  l  m  n  o
      0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 //   p  q  r  s  t  u  v  w  x  y  z  {  |  }  ~  <NBSP>
      1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0
 };
 
+// Unescapes the escape sequence starting at index in escaped_text into unsigned
+// char value.

[Peter Kasting, 2014/02/27 04:43:04]

Nit:

// Attempts to unescape the sequence at |index| within |escaped_text|.  If
// successful, sets |value| to the unescaped value.  Returns whether unescaping
// succeeded.

[Anuj, 2014/02/27 19:38:45]

Done.

+template<typename STR>
+bool UnescapeUnsignedCharAtIndex(const STR& escaped_text,
+                                int index,

[Peter Kasting, 2014/02/27 04:43:04]

This should be a size_t.

Nit: Indenting (2 lines)

[Anuj, 2014/02/27 19:38:45]

Done.

+                                unsigned char* value) {
+  const typename STR::value_type most_sig_digit(

[Peter Kasting, 2014/02/27 04:43:04]

This function should also check whether escaped_text[index] == '%'.

You should also check whether escaped_text.size() <= (i + 2).  Doing that check
here will allow simplifying the caller some; see below.

[Anuj, 2014/02/27 19:38:45]

Done.

+      static_cast<typename STR::value_type>(escaped_text[index + 1]));
+  const typename STR::value_type least_sig_digit(
+      static_cast<typename STR::value_type>(escaped_text[index + 2]));
+  if (IsHexDigit(most_sig_digit) && IsHexDigit(least_sig_digit)) {
+    *value = HexDigitToInt(most_sig_digit) * 16 +
+        HexDigitToInt(least_sig_digit);
+    return true;
+  }
+  return false;
+}
+
 template<typename STR>
 STR UnescapeURLWithOffsetsImpl(const STR& escaped_text,
                                UnescapeRule::Type rules,
                                std::vector<size_t>* offsets_for_adjustment) {
   if (offsets_for_adjustment) {
     std::for_each(offsets_for_adjustment->begin(),
                   offsets_for_adjustment->end(),
                   base::LimitOffset<STR>(escaped_text.length()));
   }
   // Do not unescape anything, return the |escaped_text| text.
   if (rules == UnescapeRule::NONE)
     return escaped_text;
 
   // The output of the unescaping is always smaller than the input, so we can
   // reserve the input size to make sure we have enough buffer and don't have
   // to allocate in the loop below.
   STR result;
   result.reserve(escaped_text.length());
 
   // Locations of adjusted text.
   net::internal::AdjustEncodingOffset::Adjustments adjustments;
   for (size_t i = 0, max = escaped_text.size(); i < max; ++i) {
     if (static_cast<unsigned char>(escaped_text[i]) >= 128) {
       // Non ASCII character, append as is.
       result.push_back(escaped_text[i]);
       continue;
     }
 
     char current_char = static_cast<char>(escaped_text[i]);
     if (current_char == '%' && i + 2 < max) {

[Peter Kasting, 2014/02/27 04:43:04]

If you add the checks mentioned above, |max| can be eliminated entirely, and
this can become:

  unsigned char value;
  if (UnescapeUnsignedCharAtIndex(escaped_text, i, &value)) {
    ...

[Anuj, 2014/02/27 19:38:45]

Done.

-      const typename STR::value_type most_sig_digit(
-          static_cast<typename STR::value_type>(escaped_text[i + 1]));
-      const typename STR::value_type least_sig_digit(
-          static_cast<typename STR::value_type>(escaped_text[i + 2]));
-      if (IsHexDigit(most_sig_digit) && IsHexDigit(least_sig_digit)) {
-        unsigned char value = HexDigitToInt(most_sig_digit) * 16 +
-            HexDigitToInt(least_sig_digit);
+      unsigned char value;
+      if (UnescapeUnsignedCharAtIndex(escaped_text, i, &value)) {
+        // As per http://tools.ietf.org/html/rfc3987#section-4.1, BiDi control
+        // characters are disallowed. The BiDi control characters in escaped
+        // form are :
+        // kRightToLeftMark = "%E2%80%8F"
+        // kLeftToRightMark = "%E2%80%8E"
+        // kLeftToRightEmbeddingMark = "%E2%80%AA"
+        // kRightToLeftEmbeddingMark = "%E2%80%AB"
+        // kPopDirectionalFormatting = "%E2%80%AC"
+        // kLeftToRightOverride = "%E2%80%AD"
+        // kRightToLeftOverride = "%E2%80%AE"

[Peter Kasting, 2014/02/27 04:43:04]

Nit: Don't use kNames for things that are just comments.  Also, give the Unicode
character constants and names for these:

// Per http://tools.ietf.org/html/rfc3987#section-4.1, the following BiDi
// control characters are not allowed to appear unescaped in URLs:
//
// U+200E LEFT-TO-RIGHT MARK         (%E2%80%8E)
// U+200F RIGHT-TO-LEFT MARK         (%E2%80%8F)
// U+202A LEFT-TO-RIGHT EMBEDDING    (%E2%80%AA)
// U+202B RIGHT-TO-LEFT EMBEDDING    (%E2%80%AB)
// U+202C POP DIRECTIONAL FORMATTING (%E2%80%AC)
// U+202D LEFT-TO-RIGHT OVERRIDE     (%E2%80%AD)
// U+202E RIGHT-TO-LEFT OVERRIDE     (%E2%80%AE)

[Anuj, 2014/02/27 19:38:45]

Done.

+        if (value == 0xE2 && i + 8 < max) {

[Peter Kasting, 2014/02/27 04:43:04]

If you add the checks mentioned above, you can eliminate all the length checks
here.

You do need to add checks that the function call succeeded.  (Those should have
been present anyway.)

[Anuj, 2014/02/27 19:38:45]

Done.

+          // Possible BiDi control character.
+          UnescapeUnsignedCharAtIndex(escaped_text, i + 3, &value);
+          if (value == 0x80) {
+            UnescapeUnsignedCharAtIndex(escaped_text, i + 6, &value);
+            if (value == 0xAA || value == 0xAB || value == 0xAC ||
+                value == 0xAD || value == 0xAE || value == 0x8E ||
+                value == 0x8F) {

[Peter Kasting, 2014/02/27 04:43:04]

Nit: Simpler:

if ((value == 0x8E) || (value == 0x8F) ||
    ((value >= 0xAA) && (value <= 0xAE))) { ...

[Anuj, 2014/02/27 19:38:45]

Done.

+              result.append(escaped_text, i, 9);
+              i += 8;
+              continue;
+            }
+          }
+          // Restore value if BiDi control character not found.

[Peter Kasting, 2014/02/27 04:43:04]

Prefer declaring a different temp to hold the second and third bytes instead of
overwriting |value| and then having to restore it here.

[Anuj, 2014/02/27 19:38:45]

Done.

+          value = 0xE2;
+        }
         if (value >= 0x80 ||  // Unescape all high-bit characters.
             // For 7-bit characters, the lookup table tells us all valid chars.
             (kUrlUnescape[value] ||
              // ...and we allow some additional unescaping when flags are set.
              (value == ' ' && (rules & UnescapeRule::SPACES)) ||
              // Allow any of the prohibited but non-control characters when
              // we're doing "special" chars.
              (value > ' ' && (rules & UnescapeRule::URL_SPECIAL_CHARS)) ||
              // Additionally allow control characters if requested.
              (value < ' ' && (rules & UnescapeRule::CONTROL_CHARS)))) {
@@ skipping 238 matching lines @@
       return;
     }
     adjusted_offset -= 2;
   }
   offset = adjusted_offset;
 }
 
 }  // namespace internal
 
 }  // namespace net