[GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers
authoraperez@igalia.com <aperez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Sep 2019 15:33:31 +0000 (15:33 +0000)
committeraperez@igalia.com <aperez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Sep 2019 15:33:31 +0000 (15:33 +0000)
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().

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249810 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/glib/UserAgentGLib.cpp
Source/WebCore/platform/network/HTTPParsers.cpp
Source/WebCore/platform/network/HTTPParsers.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/CMakeLists.txt
Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp

index 7a4dc10..806752f 100644 (file)
@@ -1,3 +1,40 @@
+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>
 
         Harden JSC against the abuse of runtime options.
index b5ba083..4a1bc65 100644 (file)
@@ -125,21 +125,30 @@ String standardUserAgent(const String& applicationName, const String& applicatio
     // browsers that are "Safari" but not running on OS X are the Safari iOS browser. Getting this
     // 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;
 }
 
 String standardUserAgentForURL(const URL& url)
 {
     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;
 }
 
 } // namespace WebCore
index 108e516..75a41be 100644 (file)
@@ -97,6 +97,18 @@ static inline bool skipValue(const String& str, unsigned& pos)
     return pos != start;
 }
 
+// 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)
 {
@@ -135,6 +147,35 @@ static bool isDelimiterCharacter(const UChar 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));
+}
+
 // See RFC 7231, Section 5.3.2.
 bool isValidAcceptHeaderValue(const String& value)
 {
@@ -174,22 +215,137 @@ bool isValidLanguageHeaderValue(const String& value)
 }
 
 // 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)
 {
     if (value.isEmpty())
         return false;
     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 == '}')
+        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();
+}
+
 static const size_t maxInputSampleSize = 128;
 static String trimInputSample(const char* p, size_t length)
 {
index cc6ce10..82c3a15 100644 (file)
@@ -72,6 +72,7 @@ bool isValidReasonPhrase(const String&);
 bool isValidHTTPHeaderValue(const String&);
 bool isValidAcceptHeaderValue(const String&);
 bool isValidLanguageHeaderValue(const String&);
+WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&);
 bool isValidHTTPToken(const String&);
 Optional<WallTime> parseHTTPDate(const String&);
 String filenameFromHTTPContentDisposition(const String&);
index 9df1549..be2c74c 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Harden JSC against the abuse of runtime options.
index c3b65c4..6189747 100644 (file)
@@ -35,6 +35,7 @@
 #include "WebKitSettingsPrivate.h"
 #include "WebPageProxy.h"
 #include "WebPreferences.h"
+#include <WebCore/HTTPParsers.h>
 #include <WebCore/PlatformScreen.h>
 #include <WebCore/TextEncodingRegistry.h>
 #include <WebCore/UserAgent.h>
@@ -3043,7 +3044,15 @@ void webkit_settings_set_user_agent(WebKitSettings* settings, const char* userAg
     g_return_if_fail(WEBKIT_IS_SETTINGS(settings));
 
     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;
 
index a2cf5a4..a33900b 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         Harden JSC against the abuse of runtime options.
index cbeca3a..ac5cb07 100644 (file)
@@ -130,6 +130,7 @@ if (ENABLE_WEBCORE)
         Tests/WebCore/FloatSize.cpp
         Tests/WebCore/GridPosition.cpp
         Tests/WebCore/HTMLParserIdioms.cpp
+        Tests/WebCore/HTTPParsers.cpp
         Tests/WebCore/IntPoint.cpp
         Tests/WebCore/IntRect.cpp
         Tests/WebCore/IntSize.cpp
index 0166a4d..ee4391f 100644 (file)
@@ -56,4 +56,34 @@ TEST(HTTPParsers, ParseCrossOriginResourcePolicyHeader)
     EXPECT_TRUE(parseCrossOriginResourcePolicyHeader("") == CrossOriginResourcePolicy::Invalid);
 }
 
+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