Log 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().
Modified Paths
- trunk/Source/WebCore/ChangeLog
- trunk/Source/WebCore/platform/glib/UserAgentGLib.cpp
- trunk/Source/WebCore/platform/network/HTTPParsers.cpp
- trunk/Source/WebCore/platform/network/HTTPParsers.h
- trunk/Source/WebKit/ChangeLog
- trunk/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp
- trunk/Tools/ChangeLog
- trunk/Tools/TestWebKitAPI/CMakeLists.txt
- trunk/Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp
Diff
Modified: trunk/Source/WebCore/ChangeLog (249809 => 249810)
--- trunk/Source/WebCore/ChangeLog 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Source/WebCore/ChangeLog 2019-09-12 15:33:31 UTC (rev 249810)
@@ -1,3 +1,40 @@
+2019-09-12 Adrian Perez de Castro <ape...@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....@apple.com>
Harden JSC against the abuse of runtime options.
Modified: trunk/Source/WebCore/platform/glib/UserAgentGLib.cpp (249809 => 249810)
--- trunk/Source/WebCore/platform/glib/UserAgentGLib.cpp 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Source/WebCore/platform/glib/UserAgentGLib.cpp 2019-09-12 15:33:31 UTC (rev 249810)
@@ -125,14 +125,18 @@
// 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)
@@ -139,7 +143,12 @@
{
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
Modified: trunk/Source/WebCore/platform/network/HTTPParsers.cpp (249809 => 249810)
--- trunk/Source/WebCore/platform/network/HTTPParsers.cpp 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Source/WebCore/platform/network/HTTPParsers.cpp 2019-09-12 15:33:31 UTC (rev 249810)
@@ -97,6 +97,18 @@
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 @@
|| 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,6 +215,13 @@
}
// 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())
@@ -180,16 +228,124 @@
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)
{
Modified: trunk/Source/WebCore/platform/network/HTTPParsers.h (249809 => 249810)
--- trunk/Source/WebCore/platform/network/HTTPParsers.h 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Source/WebCore/platform/network/HTTPParsers.h 2019-09-12 15:33:31 UTC (rev 249810)
@@ -72,6 +72,7 @@
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&);
Modified: trunk/Source/WebKit/ChangeLog (249809 => 249810)
--- trunk/Source/WebKit/ChangeLog 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Source/WebKit/ChangeLog 2019-09-12 15:33:31 UTC (rev 249810)
@@ -1,3 +1,16 @@
+2019-09-12 Adrian Perez de Castro <ape...@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....@apple.com>
Harden JSC against the abuse of runtime options.
Modified: trunk/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp (249809 => 249810)
--- trunk/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp 2019-09-12 15:33:31 UTC (rev 249810)
@@ -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 @@
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;
Modified: trunk/Tools/ChangeLog (249809 => 249810)
--- trunk/Tools/ChangeLog 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Tools/ChangeLog 2019-09-12 15:33:31 UTC (rev 249810)
@@ -1,3 +1,14 @@
+2019-09-12 Adrian Perez de Castro <ape...@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....@apple.com>
Harden JSC against the abuse of runtime options.
Modified: trunk/Tools/TestWebKitAPI/CMakeLists.txt (249809 => 249810)
--- trunk/Tools/TestWebKitAPI/CMakeLists.txt 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Tools/TestWebKitAPI/CMakeLists.txt 2019-09-12 15:33:31 UTC (rev 249810)
@@ -130,6 +130,7 @@
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
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp (249809 => 249810)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp 2019-09-12 15:09:31 UTC (rev 249809)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp 2019-09-12 15:33:31 UTC (rev 249810)
@@ -56,4 +56,34 @@
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
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
