- Revision
- 95490
- Author
- bbu...@chromium.org
- Date
- 2011-09-19 16:02:52 -0700 (Mon, 19 Sep 2011)
Log Message
Perform HTTP method and header validation in AssociatedURLLoader for
requests coming from untrusted code (eg. Native Client in Chrome). Use
the same code as XMLHttpRequest to reduce code duplication and have
behavior identical to XHR in _javascript_. Add an 'untrustedHTTP' option
to WebURLLoaderOptions, which AssociatedURLLoader can use to determine
if it should check the request method and headers.
https://bugs.webkit.org/show_bug.cgi?id=67655
Reviewed by Darin Fisher.
* public/WebURLLoaderOptions.h:
(WebKit::WebURLLoaderOptions::WebURLLoaderOptions):
* src/AssociatedURLLoader.cpp:
(WebKit::AssociatedURLLoader::ClientAdapter::setDelayedError):
(WebKit::AssociatedURLLoader::loadAsynchronously):
* tests/AssociatedURLLoaderTest.cpp:
(WebKit::AssociatedURLLoaderTest::CheckMethodFails):
(WebKit::AssociatedURLLoaderTest::CheckHeaderFails):
(WebKit::AssociatedURLLoaderTest::CheckFails):
(WebKit::TEST_F):
Modified Paths
Diff
Modified: trunk/Source/WebKit/chromium/ChangeLog (95489 => 95490)
--- trunk/Source/WebKit/chromium/ChangeLog 2011-09-19 22:57:30 UTC (rev 95489)
+++ trunk/Source/WebKit/chromium/ChangeLog 2011-09-19 23:02:52 UTC (rev 95490)
@@ -1,3 +1,26 @@
+2011-09-19 Bill Budge <bbu...@chromium.org>
+
+ Perform HTTP method and header validation in AssociatedURLLoader for
+ requests coming from untrusted code (eg. Native Client in Chrome). Use
+ the same code as XMLHttpRequest to reduce code duplication and have
+ behavior identical to XHR in _javascript_. Add an 'untrustedHTTP' option
+ to WebURLLoaderOptions, which AssociatedURLLoader can use to determine
+ if it should check the request method and headers.
+ https://bugs.webkit.org/show_bug.cgi?id=67655
+
+ Reviewed by Darin Fisher.
+
+ * public/WebURLLoaderOptions.h:
+ (WebKit::WebURLLoaderOptions::WebURLLoaderOptions):
+ * src/AssociatedURLLoader.cpp:
+ (WebKit::AssociatedURLLoader::ClientAdapter::setDelayedError):
+ (WebKit::AssociatedURLLoader::loadAsynchronously):
+ * tests/AssociatedURLLoaderTest.cpp:
+ (WebKit::AssociatedURLLoaderTest::CheckMethodFails):
+ (WebKit::AssociatedURLLoaderTest::CheckHeaderFails):
+ (WebKit::AssociatedURLLoaderTest::CheckFails):
+ (WebKit::TEST_F):
+
2011-09-19 Adam Barth <aba...@webkit.org>
Rename ENABLE(OPENTYPE_SANITIZER) to USE(OPENTYPE_SANITIZER)
Modified: trunk/Source/WebKit/chromium/public/WebURLLoaderOptions.h (95489 => 95490)
--- trunk/Source/WebKit/chromium/public/WebURLLoaderOptions.h 2011-09-19 22:57:30 UTC (rev 95489)
+++ trunk/Source/WebKit/chromium/public/WebURLLoaderOptions.h 2011-09-19 23:02:52 UTC (rev 95490)
@@ -41,8 +41,15 @@
CrossOriginRequestPolicyAllow
};
- WebURLLoaderOptions() : sniffContent(false), allowCredentials(false), forcePreflight(false), crossOriginRequestPolicy(CrossOriginRequestPolicyDeny) { }
+ WebURLLoaderOptions()
+ : untrustedHTTP(false)
+ , sniffContent(false)
+ , allowCredentials(false)
+ , forcePreflight(false)
+ , crossOriginRequestPolicy(CrossOriginRequestPolicyDeny)
+ { }
+ bool untrustedHTTP; // Whether to validate the method and headers as if this was an XMLHttpRequest.
bool sniffContent; // Whether to sniff content.
bool allowCredentials; // Whether to send HTTP credentials and cookies with the request.
bool forcePreflight; // If policy is to use access control, whether to force a preflight for GET, HEAD, and POST requests.
Modified: trunk/Source/WebKit/chromium/src/AssociatedURLLoader.cpp (95489 => 95490)
--- trunk/Source/WebKit/chromium/src/AssociatedURLLoader.cpp 2011-09-19 22:57:30 UTC (rev 95489)
+++ trunk/Source/WebKit/chromium/src/AssociatedURLLoader.cpp 2011-09-19 23:02:52 UTC (rev 95490)
@@ -33,11 +33,13 @@
#include "DocumentThreadableLoader.h"
#include "DocumentThreadableLoaderClient.h"
+#include "HTTPValidation.h"
#include "SubresourceLoader.h"
#include "Timer.h"
#include "WebApplicationCacheHost.h"
#include "WebDataSource.h"
#include "WebFrameImpl.h"
+#include "WebHTTPHeaderVisitor.h"
#include "WebKit.h"
#include "WebKitPlatformSupport.h"
#include "WebURLError.h"
@@ -45,13 +47,34 @@
#include "WebURLRequest.h"
#include "WrappedResourceRequest.h"
#include "WrappedResourceResponse.h"
+#include "XMLHttpRequest.h"
using namespace WebCore;
-using namespace WebKit;
using namespace WTF;
namespace WebKit {
+namespace {
+
+class SafeHTTPHeaderValidator : public WebHTTPHeaderVisitor {
+ WTF_MAKE_NONCOPYABLE(SafeHTTPHeaderValidator);
+public:
+ SafeHTTPHeaderValidator() : m_isSafe(true) { }
+
+ void visitHeader(const WebString& name, const WebString& value);
+ bool isSafe() const { return m_isSafe; }
+
+private:
+ bool m_isSafe;
+};
+
+void SafeHTTPHeaderValidator::visitHeader(const WebString& name, const WebString& value)
+{
+ m_isSafe = m_isSafe && isValidHTTPToken(name) && XMLHttpRequest::isAllowedHTTPHeader(name) && isValidHTTPHeaderValue(value);
+}
+
+}
+
// This class bridges the interface differences between WebCore and WebKit loader clients.
// It forwards its ThreadableLoaderClient notifications to a WebURLLoaderClient.
class AssociatedURLLoader::ClientAdapter : public DocumentThreadableLoaderClient {
@@ -71,6 +94,9 @@
virtual bool isDocumentThreadableLoaderClient() { return true; }
+ // Sets an error to be reported back to the client, asychronously.
+ void setDelayedError(const ResourceError&);
+
// Enables forwarding of error notifications to the WebURLLoaderClient. These must be
// deferred until after the call to AssociatedURLLoader::loadAsynchronously() completes.
void enableErrorNotifications();
@@ -177,6 +203,11 @@
notifyError(&m_errorTimer);
}
+void AssociatedURLLoader::ClientAdapter::setDelayedError(const ResourceError& error)
+{
+ didFail(error);
+}
+
void AssociatedURLLoader::ClientAdapter::enableErrorNotifications()
{
m_enableErrorNotifications = true;
@@ -225,18 +256,37 @@
m_client = client;
ASSERT(m_client);
- ThreadableLoaderOptions options;
- options.sendLoadCallbacks = SendCallbacks; // Always send callbacks.
- options.sniffContent = m_options.sniffContent ? SniffContent : DoNotSniffContent;
- options.allowCredentials = m_options.allowCredentials ? AllowStoredCredentials : DoNotAllowStoredCredentials;
- options.preflightPolicy = m_options.forcePreflight ? ForcePreflight : ConsiderPreflight;
- options.crossOriginRequestPolicy = static_cast<WebCore::CrossOriginRequestPolicy>(m_options.crossOriginRequestPolicy);
- options.shouldBufferData = DoNotBufferData;
+ bool allowLoad = true;
+ WebURLRequest newRequest(request);
+ if (m_options.untrustedHTTP) {
+ WebString method = newRequest.httpMethod();
+ allowLoad = isValidHTTPToken(method) && XMLHttpRequest::isAllowedHTTPMethod(method);
+ if (allowLoad) {
+ newRequest.setHTTPMethod(XMLHttpRequest::uppercaseKnownHTTPMethod(method));
+ SafeHTTPHeaderValidator validator;
+ newRequest.visitHTTPHeaderFields(&validator);
+ allowLoad = validator.isSafe();
+ }
+ }
- const ResourceRequest& webcoreRequest = request.toResourceRequest();
- Document* webcoreDocument = m_frameImpl->frame()->document();
m_clientAdapter = ClientAdapter::create(this, m_client, request.downloadToFile());
- m_loader = DocumentThreadableLoader::create(webcoreDocument, m_clientAdapter.get(), webcoreRequest, options);
+
+ if (allowLoad) {
+ ThreadableLoaderOptions options;
+ options.sendLoadCallbacks = SendCallbacks; // Always send callbacks.
+ options.sniffContent = m_options.sniffContent ? SniffContent : DoNotSniffContent;
+ options.allowCredentials = m_options.allowCredentials ? AllowStoredCredentials : DoNotAllowStoredCredentials;
+ options.preflightPolicy = m_options.forcePreflight ? ForcePreflight : ConsiderPreflight;
+ options.crossOriginRequestPolicy = static_cast<WebCore::CrossOriginRequestPolicy>(m_options.crossOriginRequestPolicy);
+ options.shouldBufferData = DoNotBufferData;
+
+ const ResourceRequest& webcoreRequest = newRequest.toResourceRequest();
+ Document* webcoreDocument = m_frameImpl->frame()->document();
+ m_loader = DocumentThreadableLoader::create(webcoreDocument, m_clientAdapter.get(), webcoreRequest, options);
+ } else {
+ // FIXME: return meaningful error codes.
+ m_clientAdapter->setDelayedError(ResourceError());
+ }
m_clientAdapter->enableErrorNotifications();
}
Modified: trunk/Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp (95489 => 95490)
--- trunk/Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp 2011-09-19 22:57:30 UTC (rev 95489)
+++ trunk/Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp 2011-09-19 23:02:52 UTC (rev 95490)
@@ -174,6 +174,46 @@
webkit_support::QuitMessageLoop();
}
+ void CheckMethodFails(const char* unsafeMethod)
+ {
+ WebURLRequest request;
+ request.initialize();
+ request.setURL(GURL("http://www.test.com/success.html"));
+ request.setHTTPMethod(WebString::fromUTF8(unsafeMethod));
+ WebURLLoaderOptions options;
+ options.untrustedHTTP = true;
+ CheckFails(request, options);
+ }
+
+ void CheckHeaderFails(const char* headerField)
+ {
+ CheckHeaderFails(headerField, "foo");
+ }
+
+ void CheckHeaderFails(const char* headerField, const char* headerValue)
+ {
+ WebURLRequest request;
+ request.initialize();
+ request.setURL(GURL("http://www.test.com/success.html"));
+ request.setHTTPHeaderField(WebString::fromUTF8(headerField), WebString::fromUTF8(headerValue));
+ WebURLLoaderOptions options;
+ options.untrustedHTTP = true;
+ CheckFails(request, options);
+ }
+
+ void CheckFails(const WebURLRequest& request, WebURLLoaderOptions options = WebURLLoaderOptions())
+ {
+ m_expectedLoader = createAssociatedURLLoader(options);
+ EXPECT_TRUE(m_expectedLoader);
+ m_didFail = false;
+ m_expectedLoader->loadAsynchronously(request, this);
+ // Failure should not be reported synchronously.
+ EXPECT_FALSE(m_didFail);
+ // Allow the loader to return the error.
+ webkit_support::RunMessageLoop();
+ EXPECT_TRUE(m_didFail);
+ }
+
protected:
WebString m_frameFilePath;
TestWebFrameClient m_webFrameClient;
@@ -223,15 +263,7 @@
WebURLRequest request;
request.initialize();
request.setURL(url);
-
- m_expectedLoader = createAssociatedURLLoader();
- EXPECT_TRUE(m_expectedLoader);
- m_expectedLoader->loadAsynchronously(request, this);
- // Failure should not be reported synchronously.
- EXPECT_FALSE(m_didFail);
- // Allow the loader to return the error.
- webkit_support::RunMessageLoop();
- EXPECT_TRUE(m_didFail);
+ CheckFails(request);
}
// Test a successful cross-origin load.
@@ -259,4 +291,57 @@
EXPECT_TRUE(m_didFinishLoading);
}
+// Test that untrusted loads can't use a forbidden method.
+TEST_F(AssociatedURLLoaderTest, UntrustedCheckMethods)
+{
+ // Check non-token method fails.
+ CheckMethodFails("GET()");
+ CheckMethodFails("POST\x0d\x0ax-csrf-token:\x20test1234");
+
+ // Forbidden methods should fail regardless of casing.
+ CheckMethodFails("CoNneCt");
+ CheckMethodFails("TrAcK");
+ CheckMethodFails("TrAcE");
}
+
+// Test that untrusted loads can't use a forbidden header field.
+TEST_F(AssociatedURLLoaderTest, UntrustedCheckHeaders)
+{
+ // Check non-token header fails.
+ CheckHeaderFails("foo()");
+
+ // Check forbidden headers fail.
+ CheckHeaderFails("accept-charset");
+ CheckHeaderFails("accept-encoding");
+ CheckHeaderFails("connection");
+ CheckHeaderFails("content-length");
+ CheckHeaderFails("cookie");
+ CheckHeaderFails("cookie2");
+ CheckHeaderFails("content-transfer-encoding");
+ CheckHeaderFails("date");
+ CheckHeaderFails("expect");
+ CheckHeaderFails("host");
+ CheckHeaderFails("keep-alive");
+ CheckHeaderFails("origin");
+ CheckHeaderFails("referer");
+ CheckHeaderFails("te");
+ CheckHeaderFails("trailer");
+ CheckHeaderFails("transfer-encoding");
+ CheckHeaderFails("upgrade");
+ CheckHeaderFails("user-agent");
+ CheckHeaderFails("via");
+
+ CheckHeaderFails("proxy-");
+ CheckHeaderFails("proxy-foo");
+ CheckHeaderFails("sec-");
+ CheckHeaderFails("sec-foo");
+
+ // Check that validation is case-insensitive.
+ CheckHeaderFails("AcCePt-ChArSeT");
+ CheckHeaderFails("ProXy-FoO");
+
+ // Check invalid header values.
+ CheckHeaderFails("foo", "bar\x0d\x0ax-csrf-token:\x20test1234");
+}
+
+}
