New issue
Advanced search Search tips

Issue 749700 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

Review DeviceDescriptionFetcher

Project Member Reported by palmer@chromium.org, Jul 27 2017

Issue description

Comment 1 by sko...@chromium.org, Sep 11 2017

Labels: -M-62

Comment 2 by palmer@chromium.org, Jun 29 2018

Cc: mfo...@chromium.org
Labels: -OS-All OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Status: Started (was: Assigned)
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?

Comment 3 by palmer@chromium.org, Jun 29 2018

Labels: -Type-Bug Type-Task

Comment 4 by palmer@chromium.org, 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.


Comment 5 by palmer@chromium.org, Jun 29 2018

Cc: palmer@chromium.org
Labels: -Type-Task Type-Bug
Owner: mfo...@chromium.org
Status: Assigned (was: Started)
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.

Comment 6 by mfo...@chromium.org, Jun 29 2018

Labels: Target-70
These are all reasonable fixes.  Thanks for the review.

Cc: powerb@chromium.org
Cc: -zhaobin@chromium.org
Labels: -Target-70 Target-71
Labels: -Target-71 Target-72
Status: Started (was: Assigned)
I'm also going to fix Bug 679432 as it wasn't addressed when we deprecated and removed the extension API for this.
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
The issues identified here should be addressed.
Thank you mfoltz! :)

Sign in to add a comment