Review DeviceDescriptionFetcher |
||||||||||
Issue descriptionSee https://bugs.chromium.org/p/chromium/issues/detail?id=718154&desc=2 for discussion.
,
Jun 29 2018
DeviceDescriptionFetcher::ProcessResponse seems to explicitly disallow HTTPS Application URLs:
```
// 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()) {
ReportError(net::Error::OK,
base::StringPrintf("Invalid Application-URL: %s",
app_url_header.c_str()));
return;
}
```
+mfoltz: Shouldn't we allow HTTPS, and hope that Application URLs migrate to it over time?
,
Jun 29 2018
,
Jun 29 2018
The text-scrubbing feature (for privacy when debugging?) is brittle:
```
#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, "***");
}
```
E.g. valid markup like `<foo-element >blah blah</foo-element>` would not get scrubbed.
The proper way to do this is to iterate over the XML element tree in a proper parser. That would necessitate using a utility process. I realize that would be quite arduous just to support debugging.
There's also the unlikely issue of integer overflow in `pos + element_name.length() + 2`. I mention it only for completeness.
Do we still need this code? If we don't, we might as well delete it. If we do, we should at the very least make it a little less brittle, such as by tolerating additional characters in the start tag.
It seems like
```
bool IsValidAppUrl(const GURL& app_url, const std::string& ip_address) {
return app_url.SchemeIs(url::kHttpScheme) && app_url.host() == ip_address;
}
```
applies a different constraint than the code in comment #2. If they are meant to process the same kinds of URLs, the policy should be centralized in a single shared function.
,
Jun 29 2018
Given that the XML parsing happens in a sandboxed utility process, which is the main risk area, I think I'm done for now. Passing to mfoltz to see if you can address the minor issues described above.
,
Jun 29 2018
These are all reasonable fixes. Thanks for the review.
,
Aug 2
,
Aug 2
,
Oct 2
,
Nov 7
,
Nov 7
I'm also going to fix Bug 679432 as it wasn't addressed when we deprecated and removed the extension API for this.
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731 commit dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731 Author: mark a. foltz <mfoltz@chromium.org> Date: Thu Nov 08 22:43:24 2018 [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/1322351 Reviewed-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} [modify] https://crrev.com/dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731/chrome/browser/media/router/discovery/dial/device_description_fetcher.cc [modify] https://crrev.com/dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731/chrome/browser/media/router/discovery/dial/device_description_service.cc [modify] https://crrev.com/dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731/chrome/browser/media/router/discovery/dial/dial_device_data.cc [modify] https://crrev.com/dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731/chrome/browser/media/router/discovery/dial/dial_device_data.h [modify] https://crrev.com/dd34f4a74c4e63a1c08cfdc2e976e7194a9fc731/chrome/browser/media/router/discovery/dial/dial_device_data_unittest.cc
,
Nov 16
The issues identified here should be addressed.
,
Nov 27
Thank you mfoltz! :) |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by sko...@chromium.org
, Sep 11 2017