[Media Router] Address security review for DialDeviceDescriptionFetcher.

This addresses comments by palmer@ in Bug 749700.  Specifically:
1. Remove XML scrubbing as it's fragile and only used for debug logging.
2. Allow http: and https: for DIAL application URLs.
3. Consolidate validation logic for DIAL application URLs.

In addition some IWYU and other minor cleanups.

Bug: 749700
Change-Id: Iddde911f2c2848708fcd2854acf98e26e0be8eb4
Reviewed-on: https://chromium-review.googlesource.com/c/1322351Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Brandon Tolsch <btolsch@chromium.org>
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606627}

chrome/browser/media/router/discovery/dial/device_description_fetcher.cc

@@ -4,8 +4,11 @@
 
 #include "chrome/browser/media/router/discovery/dial/device_description_fetcher.h"
 
+#include <utility>
+
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/media/router/discovery/dial/dial_device_data.h"
+#include "net/base/ip_address.h"
 #include "net/http/http_response_headers.h"
 
 constexpr char kApplicationUrlHeaderName[] = "Application-URL";
@@ -62,9 +65,12 @@ void DeviceDescriptionFetcher::ProcessResponse(const std::string& response) {
   // Section 5.4 of the DIAL spec implies that the Application URL should not
   // have path, query or fragment...unsure if that can be enforced.
   GURL app_url(app_url_header);
-  if (!app_url.is_valid() || !app_url.SchemeIs("http") ||
-      !app_url.HostIsIPAddress() ||
-      app_url.host() != device_description_url_.host()) {
+
+  // TODO(crbug.com/679432): Get the device IP from the SSDP response.
+  net::IPAddress device_ip;
+  if (!device_ip.AssignFromIPLiteral(
+          device_description_url_.HostNoBracketsPiece()) ||
+      !DialDeviceData::IsValidDialAppUrl(app_url, device_ip)) {
     ReportError(net::Error::OK,
                 base::StringPrintf("Invalid Application-URL: %s",
                                    app_url_header.c_str()));

chrome/browser/media/router/discovery/dial/device_description_service.cc

@@ -4,12 +4,15 @@
 
 #include "chrome/browser/media/router/discovery/dial/device_description_service.h"
 
+#include <map>
+#include <utility>
+#include <vector>
+
 #if DCHECK_IS_ON()
 #include <sstream>
 #endif
 
 #include "base/stl_util.h"
-#include "base/strings/string_util.h"
 #include "chrome/browser/media/router/data_decoder_util.h"
 #include "chrome/browser/media/router/discovery/dial/device_description_fetcher.h"
 #include "chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.h"
@@ -33,30 +36,6 @@ constexpr int kCacheCleanUpTimeoutMins = 30;
 constexpr int kCacheMaxEntries = 256;
 
 #if DCHECK_IS_ON()
-// Replaces "<element_name>content</element_name>" with
-// "<element_name>***</element_name>"
-void Scrub(const std::string& element_name, std::string* xml_text) {
-  size_t pos = xml_text->find("<" + element_name + ">");
-  size_t end_pos = xml_text->find("</" + element_name + ">");
-
-  if (pos == std::string::npos || end_pos == std::string::npos)
-    return;
-
-  size_t start_pos = pos + element_name.length() + 2;
-  if (end_pos > start_pos)
-    xml_text->replace(start_pos, end_pos - start_pos, "***");
-}
-
-// Removes unique identifiers from the device description.
-// |xml_text|: The device description XML.
-// Returns original XML text if <UDN> or <serialNumber> field does not exist.
-std::string ScrubDeviceDescriptionXml(const std::string& xml_text) {
-  std::string scrubbed_xml(xml_text);
-  Scrub("UDN", &scrubbed_xml);
-  Scrub("serialNumber", &scrubbed_xml);
-  return scrubbed_xml;
-}
-
 std::string CachedDeviceDescriptionToString(
     const media_router::DeviceDescriptionService::CacheEntry& cached_data) {
   std::stringstream ss;
@@ -72,15 +51,11 @@ std::string CachedDeviceDescriptionToString(
 }
 #endif
 
-bool IsValidAppUrl(const GURL& app_url, const std::string& ip_address) {
-  return app_url.SchemeIs(url::kHttpScheme) && app_url.host() == ip_address;
-}
-
 // Checks mandatory fields. Returns ParsingError::kNone if device description is
 // valid; Otherwise returns specific error type.
 ParsingError ValidateParsedDeviceDescription(
-    const std::string& expected_ip_address,
-    const media_router::ParsedDialDeviceDescription& description_data) {
+    const GURL& device_description_url,
+    const ParsedDialDeviceDescription& description_data) {
   if (description_data.unique_id.empty()) {
     return ParsingError::kMissingUniqueId;
   }
@@ -90,7 +65,12 @@ ParsingError ValidateParsedDeviceDescription(
   if (!description_data.app_url.is_valid()) {
     return ParsingError::kMissingAppUrl;
   }
-  if (!IsValidAppUrl(description_data.app_url, expected_ip_address)) {
+
+  // TODO(crbug.com/679432): Get the device IP from the SSDP response.
+  net::IPAddress device_ip;
+  if (!device_ip.AssignFromIPLiteral(
+          device_description_url.HostNoBracketsPiece()) ||
+      !DialDeviceData::IsValidDialAppUrl(description_data.app_url, device_ip)) {
     return ParsingError::kInvalidAppUrl;
   }
   return ParsingError::kNone;
@@ -242,7 +222,7 @@ void DeviceDescriptionService::OnParsedDeviceDescription(
   }
 
   ParsingError error = ValidateParsedDeviceDescription(
-      device_data.device_description_url().host(), device_description);
+      device_data.device_description_url(), device_description);
 
   if (error != ParsingError::kNone) {
     DLOG(WARNING) << "Device description failed to validate. "
@@ -265,12 +245,11 @@ void DeviceDescriptionService::OnParsedDeviceDescription(
   cached_description_data.description_data = device_description;
 
 #if DCHECK_IS_ON()
-  DVLOG(2) << "Got device description for " << device_data.label()
+  DVLOG(2) << "Caching device description for " << device_data.label()
            << "... device description was: "
            << CachedDeviceDescriptionToString(cached_description_data);
 #endif
 
-  DVLOG(2) << "Caching device description for " << device_data.label();
   description_cache_.insert(
       std::make_pair(device_data.label(), cached_description_data));
 
@@ -283,12 +262,6 @@ void DeviceDescriptionService::OnDeviceDescriptionFetchComplete(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   ParseDeviceDescription(device_data, description_data);
-
-#if DCHECK_IS_ON()
-  DVLOG(2) << "Device description: "
-           << ScrubDeviceDescriptionXml(description_data.device_description);
-#endif
-
   device_description_fetcher_map_.erase(device_data.label());
 }
 

chrome/browser/media/router/discovery/dial/dial_device_data.cc

@@ -44,6 +44,18 @@ bool DialDeviceData::IsDeviceDescriptionUrl(const GURL& url) {
   return !address.IsPubliclyRoutable();
 }
 
+// static
+bool DialDeviceData::IsValidDialAppUrl(
+    const GURL& url,
+    const net::IPAddress& expected_ip_address) {
+  if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())
+    return false;
+
+  net::IPAddress host_ip;
+  return host_ip.AssignFromIPLiteral(url.HostNoBracketsPiece()) &&
+         host_ip.IsValid() && host_ip == expected_ip_address;
+}
+
 bool DialDeviceData::UpdateFrom(const DialDeviceData& new_data) {
   DCHECK(new_data.device_id() == device_id_);
   DCHECK(new_data.label().empty());

chrome/browser/media/router/discovery/dial/dial_device_data.h

@@ -12,6 +12,10 @@
 #include "base/values.h"
 #include "url/gurl.h"
 
+namespace net {
+class IPAddress;
+}
+
 namespace media_router {
 
 // Dial device information that is used within the DialService and Registry on
@@ -60,6 +64,11 @@ class DialDeviceData {
   // Validates that the URL is valid for the device description.
   static bool IsDeviceDescriptionUrl(const GURL& url);
 
+  // Returns true if |app_url| is a valid DIAL Application URL with a hostname
+  // matching |expected_ip_address|.
+  static bool IsValidDialAppUrl(const GURL& url,
+                                const net::IPAddress& expected_ip_address);
+
  private:
   // Hardware identifier from the DIAL response.  Not exposed to API clients.
   std::string device_id_;
@@ -81,6 +90,7 @@ class DialDeviceData {
   int config_id_;
 };
 
+// TODO(mfoltz): Do we need this as well as ParsedDialDeviceDescriptionData?
 struct DialDeviceDescriptionData {
  public:
   DialDeviceDescriptionData() = default;

chrome/browser/media/router/discovery/dial/dial_device_data_unittest.cc

@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/media/router/discovery/dial/dial_device_data.h"
+
+#include "net/base/ip_address.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media_router {
@@ -84,4 +86,36 @@ TEST(DialDeviceDataTest, TestIsDeviceDescriptionUrl) {
       DialDeviceData::IsDeviceDescriptionUrl(GURL("file://path/to/file")));
 }
 
+TEST(DialDeviceDataTest, TestIsValidDialAppUrl) {
+  net::IPAddress ipv4_address_1;
+  ASSERT_TRUE(ipv4_address_1.AssignFromIPLiteral("192.168.1.1"));
+  net::IPAddress ipv4_address_2;
+  ASSERT_TRUE(ipv4_address_2.AssignFromIPLiteral("192.168.1.2"));
+  net::IPAddress ipv4_address_bad_octets;
+  ASSERT_FALSE(ipv4_address_bad_octets.AssignFromIPLiteral("400.500.12.999"));
+  net::IPAddress ipv4_address_hex;
+  ASSERT_TRUE(ipv4_address_hex.AssignFromIPLiteral("222.173.190.239"));
+  net::IPAddress ipv6_address;
+  ASSERT_TRUE(ipv6_address.AssignFromIPLiteral(
+      "2401:fa00:480:8600:c4ee:4adc:6dd2:8e78"));
+
+  EXPECT_TRUE(DialDeviceData::IsValidDialAppUrl(
+      GURL("http://192.168.1.1:1234/apps"), ipv4_address_1));
+  EXPECT_TRUE(DialDeviceData::IsValidDialAppUrl(
+      GURL("https://192.168.1.2:4321/apps"), ipv4_address_2));
+  EXPECT_TRUE(DialDeviceData::IsValidDialAppUrl(
+      GURL("http://0xDEADBEEF:1234/apps"), ipv4_address_hex));
+  EXPECT_TRUE(DialDeviceData::IsValidDialAppUrl(
+      GURL("http://[2401:fa00:480:8600:c4ee:4adc:6dd2:8e78]:1234/apps"),
+      ipv6_address));
+
+  // AppUrl host does not match.
+  EXPECT_FALSE(DialDeviceData::IsValidDialAppUrl(
+      GURL("http://192.168.0.1:1234/apps"), ipv4_address_1));
+  EXPECT_FALSE(DialDeviceData::IsValidDialAppUrl(
+      GURL("http://400.500.12.999:1234/apps"), ipv4_address_bad_octets));
+  EXPECT_FALSE(DialDeviceData::IsValidDialAppUrl(
+      GURL("http://192.168.0.2.com:1234/apps"), ipv4_address_1));
+}
+
 }  // namespace media_router