Commit b604dd16 authored by mark a. foltz's avatar mark a. foltz Committed by Commit Bot

[DIAL MRP] Adds an Origin: header to DIAL requests.

This adds a fake Origin header to DIAL HTTP requests to mitigate
security issues.

Bug: 1055608
Change-Id: I72148bd81ef1804735feff9ec9fcfc0270c1500b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2391761Reviewed-by: default avatarTakumi Fujimoto <takumif@chromium.org>
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804570}
parent a79912e5
......@@ -5,9 +5,12 @@
#include "chrome/browser/media/router/discovery/dial/dial_url_fetcher.h"
#include "base/bind.h"
#include "base/strings/strcat.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "components/version_info/version_info.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
......@@ -71,6 +74,16 @@ void BindURLLoaderFactoryReceiverOnUIThread(
factory->Clone(std::move(receiver));
}
std::string GetFakeOriginForDialLaunch() {
// Syntax: package:Google-Chrome.87.Mac-OS-X
std::string product_name(version_info::GetProductName());
base::ReplaceChars(product_name, " ", "-", &product_name);
std::string os_type(version_info::GetOSType());
base::ReplaceChars(os_type, " ", "-", &os_type);
return base::StrCat({"package:", product_name, ".",
version_info::GetMajorVersionNumber(), ".", os_type});
}
} // namespace
DialURLFetcher::DialURLFetcher(DialURLFetcher::SuccessCallback success_cb,
......@@ -112,6 +125,10 @@ void DialURLFetcher::Start(const GURL& url,
auto request = std::make_unique<network::ResourceRequest>();
request->url = url;
request->method = method;
// As a security mitigation, DIAL launch requests now require a fake origin
// which cannot be spoofed by the drive-by Web. Rather than attempt to
// coerce this fake origin into a url::Origin, set the header directly.
request->headers.SetHeader("Origin", GetFakeOriginForDialLaunch());
method_ = method;
// net::LOAD_BYPASS_PROXY: Proxies almost certainly hurt more cases than they
......@@ -119,6 +136,9 @@ void DialURLFetcher::Start(const GURL& url,
// net::LOAD_DISABLE_CACHE: The request should not touch the cache.
request->load_flags = net::LOAD_BYPASS_PROXY | net::LOAD_DISABLE_CACHE;
request->credentials_mode = network::mojom::CredentialsMode::kOmit;
if (saved_request_for_test_) {
*saved_request_for_test_ = *request;
}
loader_ = network::SimpleURLLoader::Create(std::move(request),
kDialUrlFetcherTrafficAnnotation);
......
......@@ -21,12 +21,13 @@ struct RedirectInfo;
}
namespace network {
struct ResourceRequest;
class SimpleURLLoader;
} // namespace network
namespace media_router {
// Used to make a single HTTP GET request with |url| to fetch a response
// Used to make a single HTTP request with |url| to fetch a response
// from a DIAL device. If successful, |success_cb| is invoked with the result;
// otherwise, |error_cb| is invoked with an error reason.
// This class is not sequence safe.
......@@ -59,6 +60,13 @@ class DialURLFetcher {
// of completion.
const network::mojom::URLResponseHead* GetResponseHead() const;
// If a non-nullptr |request| is passed, a copy of the resource request will
// be stored in it when the request is started. |request| must outlive the
// call to Get(), Delete() or Post().
void SetSavedRequestForTest(network::ResourceRequest* request) {
saved_request_for_test_ = request;
}
private:
friend class TestDialURLFetcher;
......@@ -93,6 +101,7 @@ class DialURLFetcher {
// The HTTP method that was started on the fetcher (e.g., "GET").
std::string method_;
network::ResourceRequest* saved_request_for_test_ = nullptr;
SEQUENCE_CHECKER(sequence_checker_);
DISALLOW_COPY_AND_ASSIGN(DialURLFetcher);
......
......@@ -9,11 +9,13 @@
#include "base/callback.h"
#include "base/macros.h"
#include "base/run_loop.h"
#include "base/strings/string_util.h"
#include "base/test/task_environment.h"
#include "chrome/browser/media/router/discovery/dial/dial_url_fetcher.h"
#include "chrome/browser/media/router/test/test_helper.h"
#include "chrome/test/base/testing_profile.h"
#include "content/public/test/browser_task_environment.h"
#include "net/base/load_flags.h"
#include "net/http/http_response_headers.h"
#include "net/http/http_status_code.h"
#include "services/network/test/test_url_loader_factory.h"
......@@ -35,6 +37,7 @@ class DialURLFetcherTest : public testing::Test {
base::BindOnce(&DialURLFetcherTest::OnSuccess, base::Unretained(this)),
base::BindOnce(&DialURLFetcherTest::OnError, base::Unretained(this)),
&loader_factory_);
fetcher_->SetSavedRequestForTest(&request_);
fetcher_->Get(url_);
base::RunLoop().RunUntilIdle();
}
......@@ -47,6 +50,7 @@ class DialURLFetcherTest : public testing::Test {
network::TestURLLoaderFactory loader_factory_;
const GURL url_;
std::unique_ptr<TestDialURLFetcher> fetcher_;
network::ResourceRequest request_;
private:
DISALLOW_COPY_AND_ASSIGN(DialURLFetcherTest);
......@@ -60,6 +64,16 @@ TEST_F(DialURLFetcherTest, FetchSuccessful) {
loader_factory_.AddResponse(url_, network::mojom::URLResponseHead::New(),
body, status);
StartGetRequest();
// Verify the request parameters.
EXPECT_EQ(request_.url, url_);
EXPECT_EQ(request_.method, "GET");
std::string origin_header;
EXPECT_TRUE(request_.headers.GetHeader("Origin", &origin_header));
EXPECT_TRUE(base::StartsWith(origin_header, "package:"));
EXPECT_TRUE(request_.load_flags & net::LOAD_BYPASS_PROXY);
EXPECT_TRUE(request_.load_flags & net::LOAD_DISABLE_CACHE);
EXPECT_EQ(request_.credentials_mode, network::mojom::CredentialsMode::kOmit);
}
TEST_F(DialURLFetcherTest, FetchFailsOnMissingAppInfo) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment