Percent-encode illegal characters in Android page info popup URL

chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java

 // Copyright 2013 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.
 
 package org.chromium.chrome.browser;
 
 import android.app.Dialog;
 import android.content.Context;
 import android.content.DialogInterface;
 import android.graphics.Color;
 import android.graphics.drawable.ColorDrawable;
+import android.net.Uri;
 import android.text.Layout;
 import android.text.Spannable;
 import android.text.SpannableStringBuilder;
 import android.text.style.ForegroundColorSpan;
 import android.text.style.StyleSpan;
 import android.util.AttributeSet;
 import android.view.Gravity;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.View.OnClickListener;
@@ skipping 135 matching lines @@
             if (maxLines != mCurrentMaxLines) {
                 setMaxLines(maxLines);
                 return true;
             }
             return false;
         }
     }
 
     private static final int MAX_TABLET_DIALOG_WIDTH_DP = 400;
 
+    // This is the "reserved" character set from RFC 3986, plus the '%' character.
+    private static final String URI_RESERVED_CHARACTERS = "!*'();:@&=+$,/?#[]%";
+
+    private static final char FIRST_UNICODE_WHITESPACE = '\u2000';
+    private static final char FINAL_UNICODE_WHITESPACE = '\u200F';
+    private static final char UNICODE_NBSP = '\u00A0';
+
     private final Context mContext;
     private final Profile mProfile;
     private final WebContents mWebContents;
 
     // A pointer to the C++ object for this UI.
     private final long mNativeWebsiteSettingsPopup;
 
     // The outer container, filled with the layout from website_settings.xml.
     private final LinearLayout mContainer;
 
@@ skipping 105 matching lines @@
         mFullUrl = mWebContents.getVisibleUrl();
         try {
             mParsedUrl = new URI(mFullUrl);
             mIsInternalPage = UrlUtilities.isInternalScheme(mParsedUrl);
         } catch (URISyntaxException e) {
             mParsedUrl = null;
             mIsInternalPage = false;
         }
         mSecurityLevel = ToolbarModel.getSecurityLevelForWebContents(mWebContents);
 
-        SpannableStringBuilder urlBuilder = new SpannableStringBuilder(mFullUrl);
+        String displayUrl = encodeSuspiciousFragment(mFullUrl);
+        SpannableStringBuilder urlBuilder = new SpannableStringBuilder(displayUrl);
         OmniboxUrlEmphasizer.emphasizeUrl(urlBuilder, mContext.getResources(), mProfile,
                 mSecurityLevel, mIsInternalPage, true);
         mUrlTitle.setText(urlBuilder);
 
         // Set the URL connection message now, and the URL after layout (so it
         // can calculate its ideal height).
         mUrlConnectionMessage.setText(getUrlConnectionMessage());
     }
 
     /**
+     * Percent-encodes a URL fragment if it contains suspicious unicode whitespace characters.

[Matt Giuca, 2015/03/24 06:28:37]

nit: Unicode is a proper noun (always capitalize when using in a sentence).

[tsergeant, 2015/03/25 01:55:24]

Done.

+     * Otherwise, returns the original URL. All parts of the URL other than the fragment will
+     * already be encoded.
+     */
+    public static String encodeSuspiciousFragment(String urlStr) {

[Matt Giuca, 2015/03/24 06:28:37]

What do you mean by "fragment" here? Do you mean simply "a piece of a URL", or
the specific meaning of "fragment" which is the part after the '#'?

If it's the former, you should pick a different word for it. If it's the latter,
I don't see that reflected in the code (there is nothing here which goes after
the fragment only), in which case this should just be renamed to
encodeSuspiciousUrl.

[tsergeant, 2015/03/25 01:55:24]

The intention was that even though we're working on the entire URL, only the
fragment will ever be modified. Fixed, anyway.

[Matt Giuca, 2015/03/25 03:23:24]

You should remove the last sentence of the documentation (which says it only
affects the fragment).

+        if (isSuspiciousUrl(urlStr)) {

[Matt Giuca, 2015/03/24 06:28:37]

I'm not too keen on this behaviour, as you will be encoding non-suspicious
characters conditionally on the presence of other suspicious characters.

Using your test example:
"Düsseldorf" becomes "Düsseldorf", but
"Düsseldorf, Germany" becomes "D%C3%BCsseldorf,%20Germany".

If you don't have a reason to encode the 'ü' in the former example, you should
not encode the 'ü' in the latter either.

(At the very least, you should include both the "Düsseldorf" and "Düsseldorf,
Germany" examples in the tests, so it's clear what the behaviour is.)

If your new plan is essentially to only consider spaces to be a problem, then I
think you should just encode the space characters. That makes it hard to use
Uri.encode (since it requires a whitelist, not a blacklist), but since you are
prepared to manually iterate over the string, you could just call Uri.encode on
just the characters you identify as spaces, and then put them back together.

[tsergeant, 2015/03/25 01:55:24]

Makes sense, Done.

+            // Explicitly pass the set of reserved characters to prevent them from being escaped.
+            return Uri.encode(urlStr, URI_RESERVED_CHARACTERS);
+        }
+        return urlStr;
+    }
+
+    /**
+     * Returns true if the fragment string contains characters that could be used to inject a
+     * phishing attack into the popup. We filter based on any unicode whitespace character.
+     */
+    public static boolean isSuspiciousUrl(String urlStr) {
+        for (int i = 0; i < urlStr.length(); i++) {
+            char fragmentChar = urlStr.charAt(i);
+            if ((fragmentChar >= FIRST_UNICODE_WHITESPACE
+                        && fragmentChar <= FINAL_UNICODE_WHITESPACE)
+                    || fragmentChar == ' '
+                    || fragmentChar == UNICODE_NBSP)
+                return true;
+        }
+        return false;
+    }
+
+    /**
      * Sets the visibility of the lower area of the dialog (containing the permissions and 'Site
      * Settings' button).
      *
      * @param isVisible Whether to show or hide the dialog area.
      */
     private void setVisibilityOfLowerDialogArea(boolean isVisible) {
         mHorizontalSeparator.setVisibility(isVisible ? View.VISIBLE : View.GONE);
         mLowerDialogArea.setVisibility(isVisible ? View.VISIBLE : View.GONE);
     }
 
@@ skipping 223 matching lines @@
         new WebsiteSettingsPopup(context, profile, webContents);
     }
 
     private static native long nativeInit(WebsiteSettingsPopup popup, WebContents webContents);
 
     private native void nativeDestroy(long nativeWebsiteSettingsPopupAndroid);
 
     private native void nativeOnPermissionSettingChanged(long nativeWebsiteSettingsPopupAndroid,
             int type, int setting);
 }