Changeset 249810 in webkit

Timestamp:

Sep 12, 2019 8:33:31 AM (5 years ago)

Author:

Adrian Perez de Castro

Message:

[GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers
​https://bugs.webkit.org/show_bug.cgi?id=201077

Reviewed by Carlos Garcia Campos.

Source/WebCore:

Add a function to validate whether a string contains a valid value
which can be used in a HTTP User-Agent header.

Covered by new WebCore API test HTTPParsers.ValidateUserAgentValues.

• platform/glib/UserAgentGLib.cpp:

(WebCore::standardUserAgent): Assert that the returned string is a valid User-Agent.
(WebCore::standardUserAgentForURL): Ditto.

• platform/network/HTTPParsers.cpp: Added a series of helper functions which skip over

characters of a string, which can be used to scan over the different elements of an
User-Agent value; all of them receive the position from the input string where to start
scanning, updating it to the position right after the scanned item (this follow the
convention already in use by other functions in the source file). Each of them has
been annotated with the RFC number and section which contains the definition of the
scanned item, and the corresponding BNF rules to make the code easier to follow.
(WebCore::skipWhile): Added.
(WebCore::isVisibleCharacter): Added.
(WebCore::isOctectInFieldContentCharacter): Added.
(WebCore::isCommentTextCharacter): Added.
(WebCore::isHTTPTokenCharacter): Added.
(WebCore::isValidHTTPToken): Refactored to use the new isHTTPTokenCharacter()
helper function instead of having the test inside the loop.
(WebCore::skipCharacter): Added.
(WebCore::skipQuotedPair): Added.
(WebCore::skipComment): Added.
(WebCore::skipHTTPToken): Added.
(WebCore::skipUserAgentProduct): Added.
(WebCore::isValidUserAgentHeaderValue): Added.

• platform/network/HTTPParsers.h: Add prototype for isValidUserAgentHeaderValue().

Source/WebKit:

• UIProcess/API/glib/WebKitSettings.cpp:

(webkit_settings_set_user_agent): Check the passed string using the new
WebCore::isValidUserAgentHeaderValue() function, and return early without
changing the setting if the string is not usable in the User-Agent HTTP
header.

Tools:

• TestWebKitAPI/CMakeLists.txt: Add missing HTTPParsers.cpp to be built into TestWebCore.
• TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp:

(TestWebKitAPI::TEST): Add tests for WebCore::isValidUserAgentHeaderValue().

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/glib/UserAgentGLib.cpp (modified) (2 diffs)
• Source/WebCore/platform/network/HTTPParsers.cpp (modified) (4 diffs)
• Source/WebCore/platform/network/HTTPParsers.h (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/CMakeLists.txt (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-09-12  Adrian Perez de Castro  <aperez@igalia.com>
+
+        [GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers
+        https://bugs.webkit.org/show_bug.cgi?id=201077
+
+        Reviewed by Carlos Garcia Campos.
+
+        Add a function to validate whether a string contains a valid value
+        which can be used in a HTTP User-Agent header.
+
+        Covered by new WebCore API test HTTPParsers.ValidateUserAgentValues.
+
+        * platform/glib/UserAgentGLib.cpp:
+        (WebCore::standardUserAgent): Assert that the returned string is a valid User-Agent.
+        (WebCore::standardUserAgentForURL): Ditto.
+        * platform/network/HTTPParsers.cpp: Added a series of helper functions which skip over
+        characters of a string, which can be used to scan over the different elements of an
+        User-Agent value; all of them receive the position from the input string where to start
+        scanning, updating it to the position right after the scanned item (this follow the
+        convention already in use by other functions in the source file). Each of them has
+        been annotated with the RFC number and section which contains the definition of the
+        scanned item, and the corresponding BNF rules to make the code easier to follow.
+        (WebCore::skipWhile): Added.
+        (WebCore::isVisibleCharacter): Added.
+        (WebCore::isOctectInFieldContentCharacter): Added.
+        (WebCore::isCommentTextCharacter): Added.
+        (WebCore::isHTTPTokenCharacter): Added.
+        (WebCore::isValidHTTPToken): Refactored to use the new isHTTPTokenCharacter()
+        helper function instead of having the test inside the loop.
+        (WebCore::skipCharacter): Added.
+        (WebCore::skipQuotedPair): Added.
+        (WebCore::skipComment): Added.
+        (WebCore::skipHTTPToken): Added.
+        (WebCore::skipUserAgentProduct): Added.
+        (WebCore::isValidUserAgentHeaderValue): Added.
+        * platform/network/HTTPParsers.h: Add prototype for isValidUserAgentHeaderValue().
+
 2019-09-12  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/WebCore/platform/glib/UserAgentGLib.cpp

@@ -126 +126 @@
     // wrong can cause sites to load the wrong JavaScript, CSS, or custom fonts. In some cases
     // sites won't load resources at all.
-    if (applicationName.isEmpty())
-        return standardUserAgentStatic();
 
-    String finalApplicationVersion = applicationVersion;
-    if (finalApplicationVersion.isEmpty())
-        finalApplicationVersion = versionForUAString();
-
-    return standardUserAgentStatic() + ' ' + applicationName + '/' + finalApplicationVersion;
+    String userAgent;
+    if (applicationName.isEmpty()) {
+        userAgent = standardUserAgentStatic();
+    } else {
+        String finalApplicationVersion = applicationVersion;
+        if (finalApplicationVersion.isEmpty())
+            finalApplicationVersion = versionForUAString();
+        userAgent = standardUserAgentStatic() + ' ' + applicationName + '/' + finalApplicationVersion;
+    }
+    ASSERT(isValidUserAgentHeaderValue(userAgent));
+    return userAgent;
 }
 
@@ -140 +144 @@
     auto quirks = UserAgentQuirks::quirksForURL(url);
     // The null string means we don't need a specific UA for the given URL.
-    return quirks.isEmpty() ? String() : buildUserAgentString(quirks);
+    if (quirks.isEmpty())
+        return String();
+
+    String userAgent(buildUserAgentString(quirks));
+    ASSERT(isValidUserAgentHeaderValue(userAgent));
+    return userAgent;
 }
 

trunk/Source/WebCore/platform/network/HTTPParsers.cpp

@@ -98 +98 @@
 }
 
+// True if characters which satisfy the predicate are present, incrementing
+// "pos" to the next character which does not satisfy the predicate.
+// Note: might return pos == str.length().
+static inline bool skipWhile(const String& str, unsigned& pos, const WTF::Function<bool(const UChar)>& predicate)
+{
+    const unsigned start = pos;
+    const unsigned len = str.length();
+    while (pos < len && predicate(str[pos]))
+        ++pos;
+    return pos != start;
+}
+
 // See RFC 7230, Section 3.1.2.
 bool isValidReasonPhrase(const String& value)
@@ -134 +146 @@
         || c == '<' || c == '=' || c == '>' || c == '?' || c == '@' || c == '[' || c == '\\'
         || c == ']' || c == '{' || c == '}');
+}
+
+// See RFC 7230, Section 3.2.6.
+static inline bool isVisibleCharacter(const UChar c)
+{
+    // VCHAR = %x21-7E
+    return (c >= 0x21 && c <= 0x7E);
+}
+
+// See RFC 7230, Section 3.2.6.
+static inline bool isOctectInFieldContentCharacter(const UChar c)
+{
+    // obs-text = %x80-FF
+    return (c >= 0x80 && c <= 0xFF);
+}
+
+// See RFC 7230, Section 3.2.6.
+static bool isCommentTextCharacter(const UChar c)
+{
+    // ctext = HTAB / SP
+    //       / %x21-27 ; '!'-'''
+    //       / %x2A-5B ; '*'-'['
+    //       / %x5D-7E ; ']'-'~'
+    //       / obs-text
+    return (c == '\t' || c == ' '
+        || (c >= 0x21 && c <= 0x27)
+        || (c >= 0x2A && c <= 0x5B)
+        || (c >= 0x5D && c <= 0x7E)
+        || isOctectInFieldContentCharacter(c));
 }
 
@@ -175 +216 @@
 
 // See RFC 7230, Section 3.2.6.
+static inline bool isHTTPTokenCharacter(const UChar c)
+{
+    // Any VCHAR, except delimiters
+    return c > 0x20 && c < 0x7F && !isDelimiterCharacter(c);
+}
+
+// See RFC 7230, Section 3.2.6.
 bool isValidHTTPToken(const String& value)
 {
@@ -181 +229 @@
     auto valueStringView = StringView(value);
     for (UChar c : valueStringView.codeUnits()) {
-        if (c <= 0x20 || c >= 0x7F
-            || c == '(' || c == ')' || c == '<' || c == '>' || c == '@'
-            || c == ',' || c == ';' || c == ':' || c == '\\' || c == '"'
-            || c == '/' || c == '[' || c == ']' || c == '?' || c == '='
-            || c == '{' || c == '}')
-        return false;
+        if (!isHTTPTokenCharacter(c))
+            return false;
     }
     return true;
+}
+
+// True if the character at the given position satisifies a predicate, incrementing "pos" by one.
+// Note: Might return pos == str.length()
+static inline bool skipCharacter(const String& value, unsigned& pos, WTF::Function<bool(const UChar)>&& predicate)
+{
+    if (pos < value.length() && predicate(value[pos])) {
+        ++pos;
+        return true;
+    }
+    return false;
+}
+
+// True if the "expected" character is at the given position, incrementing "pos" by one.
+// Note: Might return pos == str.length()
+static inline bool skipCharacter(const String& value, unsigned& pos, const UChar expected)
+{
+    return skipCharacter(value, pos, [expected](const UChar c) {
+        return c == expected;
+    });
+}
+
+// True if a quoted pair is present, incrementing "pos" to the position after the quoted pair.
+// Note: Might return pos == str.length()
+// See RFC 7230, Section 3.2.6.
+static constexpr auto QuotedPairStartCharacter = '\\';
+static bool skipQuotedPair(const String& value, unsigned& pos)
+{
+    // quoted-pair = "\" ( HTAB / SP / VCHAR / obs-text )
+    if (!skipCharacter(value, pos, QuotedPairStartCharacter))
+        return false;
+
+    return skipCharacter(value, pos, '\t')
+        || skipCharacter(value, pos, ' ')
+        || skipCharacter(value, pos, isVisibleCharacter)
+        || skipCharacter(value, pos, isOctectInFieldContentCharacter);
+}
+
+// True if a comment is present, incrementing "pos" to the position after the comment.
+// Note: Might return pos == str.length()
+// See RFC 7230, Section 3.2.6.
+static constexpr auto CommentStartCharacter = '(';
+static constexpr auto CommentEndCharacter = ')';
+static bool skipComment(const String& value, unsigned& pos)
+{
+    // comment = "(" *( ctext / quoted-pair / comment ) ")"
+    // ctext   = HTAB / SP / %x21-27 / %x2A-5B / %x5D-7E / obs-text
+    if (!skipCharacter(value, pos, CommentStartCharacter))
+        return false;
+
+    const unsigned end = value.length();
+    while (pos < end && value[pos] != CommentEndCharacter) {
+        switch (value[pos]) {
+        case CommentStartCharacter:
+            if (!skipComment(value, pos))
+                return false;
+            break;
+        case QuotedPairStartCharacter:
+            if (!skipQuotedPair(value, pos))
+                return false;
+            break;
+        default:
+            if (!skipWhile(value, pos, isCommentTextCharacter))
+                return false;
+        }
+    }
+    return skipCharacter(value, pos, CommentEndCharacter);
+}
+
+// True if an HTTP header token is present, incrementing "pos" to the position after it.
+// Note: Might return pos == str.length()
+// See RFC 7230, Section 3.2.6.
+static bool skipHTTPToken(const String& value, unsigned& pos)
+{
+    return skipWhile(value, pos, isHTTPTokenCharacter);
+}
+
+// True if a product specifier (as in an User-Agent header) is present, incrementing "pos" to the position after it.
+// Note: Might return pos == str.length()
+// See RFC 7231, Section 5.5.3.
+static bool skipUserAgentProduct(const String& value, unsigned& pos)
+{
+    // product         = token ["/" product-version]
+    // product-version = token
+    if (!skipHTTPToken(value, pos))
+        return false;
+    if (skipCharacter(value, pos, '/'))
+        return skipHTTPToken(value, pos);
+    return true;
+}
+
+// See RFC 7231, Section 5.5.3
+bool isValidUserAgentHeaderValue(const String& value)
+{
+    // User-Agent = product *( RWS ( product / comment ) )
+    unsigned pos = 0;
+    if (!skipUserAgentProduct(value, pos))
+        return false;
+
+    while (pos < value.length()) {
+        if (!skipWhiteSpace(value, pos))
+            return false;
+        if (value[pos] == CommentStartCharacter) {
+            if (!skipComment(value, pos))
+                return false;
+        } else {
+            if (!skipUserAgentProduct(value, pos))
+                return false;
+        }
+    }
+
+    return pos == value.length();
 }
 

trunk/Source/WebCore/platform/network/HTTPParsers.h

@@ -73 +73 @@
 bool isValidAcceptHeaderValue(const String&);
 bool isValidLanguageHeaderValue(const String&);
+WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&);
 bool isValidHTTPToken(const String&);
 Optional<WallTime> parseHTTPDate(const String&);

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-09-12  Adrian Perez de Castro  <aperez@igalia.com>
+
+        [GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers
+        https://bugs.webkit.org/show_bug.cgi?id=201077
+
+        Reviewed by Carlos Garcia Campos.
+
+        * UIProcess/API/glib/WebKitSettings.cpp:
+        (webkit_settings_set_user_agent): Check the passed string using the new
+        WebCore::isValidUserAgentHeaderValue() function, and return early without
+        changing the setting if the string is not usable in the User-Agent HTTP
+        header.
+
 2019-09-12  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp

@@ -36 +36 @@
 #include "WebPageProxy.h"
 #include "WebPreferences.h"
+#include <WebCore/HTTPParsers.h>
 #include <WebCore/PlatformScreen.h>
 #include <WebCore/TextEncodingRegistry.h>
@@ -3044 +3045 @@
 
     WebKitSettingsPrivate* priv = settings->priv;
-    CString newUserAgent = (!userAgent || !strlen(userAgent)) ? WebCore::standardUserAgent("").utf8() : userAgent;
+
+    String userAgentString;
+    if (userAgent && *userAgent) {
+        userAgentString = String::fromUTF8(userAgent);
+        g_return_if_fail(WebCore::isValidUserAgentHeaderValue(userAgentString));
+    } else
+        userAgentString = WebCore::standardUserAgent("");
+
+    CString newUserAgent = userAgentString.utf8();
     if (newUserAgent == priv->userAgent)
         return;

trunk/Tools/ChangeLog

@@ +1 @@
+2019-09-12  Adrian Perez de Castro  <aperez@igalia.com>
+
+        [GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers
+        https://bugs.webkit.org/show_bug.cgi?id=201077
+
+        Reviewed by Carlos Garcia Campos.
+
+        * TestWebKitAPI/CMakeLists.txt: Add missing HTTPParsers.cpp to be built into TestWebCore.
+        * TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp:
+        (TestWebKitAPI::TEST): Add tests for WebCore::isValidUserAgentHeaderValue().
+
 2019-09-12  Mark Lam  <mark.lam@apple.com>
 

trunk/Tools/TestWebKitAPI/CMakeLists.txt

@@ -131 +131 @@
         Tests/WebCore/GridPosition.cpp
         Tests/WebCore/HTMLParserIdioms.cpp
+        Tests/WebCore/HTTPParsers.cpp
         Tests/WebCore/IntPoint.cpp
         Tests/WebCore/IntRect.cpp

trunk/Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp

@@ -57 +57 @@
 }
 
+TEST(HTTPParsers, ValidateUserAgentValues)
+{
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari WebKit"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari/10.0"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari WebKit/163"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari/10.0 WebKit"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari/10.0 WebKit/163"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari/10.0 WebKit/163 (Mozilla; like Gecko)"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari (comment (nested comment))"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari () (<- Empty comment)"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("Safari (left paren \\( as quoted pair)"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("!#$%&'*+-.^_`|~ (non-alphanumeric token characters)"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("0123456789 (numeric token characters)"));
+    EXPECT_TRUE(isValidUserAgentHeaderValue("a (single character token)"));
+
+    EXPECT_FALSE(isValidUserAgentHeaderValue(" "));
+    EXPECT_FALSE(isValidUserAgentHeaderValue(" Safari (leading whitespace)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("Safari (trailing whitespace) "));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("\nSafari (leading newline)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("Safari (trailing newline)\n"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("Safari/ (no version token after slash)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("Safari (unterminated comment"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("Safari unopened commanent)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("\x1B (contains control character)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("Safari/\n10.0 (embeded newline)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("WPE\\ WebKit (quoted pair in token)"));
+    EXPECT_FALSE(isValidUserAgentHeaderValue("/123 (missing product token)"));
+}
+
 } // namespace TestWebKitAPI