New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 770406 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug
Team-Security-UX


Show other hotlists

Hotlists containing this issue:
EnamelAndFriendsFixIt


Sign in to add a comment

Log DevTools console message for subresources with Symantec certs

Project Member Reported by est...@chromium.org, Sep 29 2017

Issue description

As a follow-up to  issue 763984 , we should log console message warnings for subresources loaded with Symantec certs. This requires plumbing the necessary information into Blink, unfortunately.
 
Note: we're currently implementing this for all subresources, which could be a little much and get really spammy for sites that have tons of Symantec subresources. After some # of messages, we could consider capping it and printing a generic message about "additional resources" instead of printing the URL for each one.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3694c80b663b36ecf77da4e0ea16c177f1f443f1

commit 3694c80b663b36ecf77da4e0ea16c177f1f443f1
Author: Emily Stark <estark@google.com>
Date: Fri Oct 06 05:30:21 2017

Log Symantec console warnings in Blink for main- and sub-resources

We previously added DevTools logging, triggered from the browser process,
for main resources that were loaded with Symantec certificates that are
slated for distrust in Chrome 66/70. This CL attaches the necessary
information to resource responses and passes it into Blink and does
the logging from there, so that we log on subresources as well as main
resources. (We must pass it into Blink because we can't associate a
resource load with a particular document/frame in which to do the logging
in the browser process.)

We want to log on all embedders, but customize the message for Chrome,
so once we get to Blink we call back out through ContentRendererClient
to allow chrome to override the console message.

Bug:  770406 
Change-Id: I992294feb763211542fdf2fcde1cbaf84e9fcf61
Reviewed-on: https://chromium-review.googlesource.com/693315
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506984}
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/chrome/browser/ssl/ssl_browser_tests.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/browser/ssl/ssl_manager_unittest.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/common/resource_messages.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/browser/content_browser_client.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/common/resource_response.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/common/resource_response_info.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/common/resource_response_info.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/content/renderer/render_frame_impl.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/public/platform/WebURLResponse.h
[modify] https://crrev.com/3694c80b663b36ecf77da4e0ea16c177f1f443f1/third_party/WebKit/public/web/WebFrameClient.h

Status: Fixed (was: Assigned)
Labels: -Pri-2 Pri-1
Status: Assigned (was: Fixed)
> Note: we're currently implementing this for all subresources, which could be a little much and get really spammy for sites that have tons of Symantec subresources. After some # of messages, we could consider capping it and printing a generic message about "additional resources" instead of printing the URL for each one.

More discussion here: https://chromium-review.googlesource.com/c/chromium/src/+/693315/3/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp#515

We're going to need to handle this for m63. 

The first two sites I opened in my ToT build today were crazily spammy. cnn.com has 106 symantec warnings logged and weather.com has 125. It's pretty rough. :)

Can we just log once for each certificate? Or once per origin/domain?
Cc: l...@chromium.org
and FWIW capping at 10/20 messages is also an option, though I prefer it less than the above solutions.

Comment 6 by l...@chromium.org, Oct 11 2017

Labels: ReleaseBlock-Beta
Solutions in comment #4 sound appropriate.  We've already started receiving complaints about how often this fills up a console ( https://bugs.chromium.org/p/chromium/issues/detail?id=734088#c10 ), so I'm marking this as release block beta.

estark@, have you started on the TODO?  Another quick, short-term solution is to make these messages Verbose level in the interim.

Comment 7 by est...@chromium.org, Oct 12 2017

Cc: mkwst@chromium.org
Status: Started (was: Assigned)
Planning on capping to 5 messages and limiting to main-frame for M63. Working on the CL now and mkwst should be able to review today in CEST.

Comment 8 by est...@chromium.org, Oct 12 2017

https://chromium-review.googlesource.com/c/chromium/src/+/715556 is in the CQ and makes the following changes:
- logs a message specific to the origin, not the full resource URL, and dedups by hostname to prevent multiple messages for the same origin
- caps out after 10 messages with a generic "Additional resources..." message
- logs resources in subframes with Verbose level

We'll probably want to do something better for M64, like list all the exact resources in the Security panel.
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80940ee33c0ecc800934aea9ef76a8852f30435a

commit 80940ee33c0ecc800934aea9ef76a8852f30435a
Author: Emily Stark <estark@google.com>
Date: Thu Oct 12 19:46:08 2017

Cap Symantec warning messages to prevent log spam

Tweak the Symantec warning message to be about an origin rather than a specific
resource URL so that we don't log more than once per hostname. Also cap to 10
messages, and log subframe resources at the Verbose level rather than Warning.

TBR=jochen@chromium.org

Bug:  770406 
Change-Id: I0ecb8bcec0ee2b6d4b83105151654b1091e2299c
Reviewed-on: https://chromium-review.googlesource.com/715556
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Paul Irish <paulirish@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508386}
[modify] https://crrev.com/80940ee33c0ecc800934aea9ef76a8852f30435a/chrome/browser/ssl/ssl_browser_tests.cc
[modify] https://crrev.com/80940ee33c0ecc800934aea9ef76a8852f30435a/chrome/renderer/chrome_content_renderer_client.cc
[add] https://crrev.com/80940ee33c0ecc800934aea9ef76a8852f30435a/chrome/test/data/ssl/page_with_many_subresources.html
[modify] https://crrev.com/80940ee33c0ecc800934aea9ef76a8852f30435a/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/80940ee33c0ecc800934aea9ef76a8852f30435a/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/80940ee33c0ecc800934aea9ef76a8852f30435a/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp

Comment 10 by l...@chromium.org, Oct 13 2017

Labels: -ReleaseBlock-Beta
Removing rb-beta for DevTools, now that capping messages has landed.  Thank you estark@!
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/90efb744873acd59517c78a1ebe300668c7079a3

commit 90efb744873acd59517c78a1ebe300668c7079a3
Author: Emily Stark <estark@google.com>
Date: Fri Oct 13 16:44:06 2017

Fix origin serialization in Symantec console message

The message was using scheme+host+port but not properly accounting for
default/absent ports, so origins were printing as, e.g.,
"https://weather.com:". Oops. Now the message actually constructs an origin and
gets its string form and doesn't try to serialize an origin itself.

TBR=jochen@chromium.org

Bug:  770406 
Change-Id: I3d6e3a27957935d14f3acd7206a62e04a041dc0c
Reviewed-on: https://chromium-review.googlesource.com/718837
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508724}
[modify] https://crrev.com/90efb744873acd59517c78a1ebe300668c7079a3/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/90efb744873acd59517c78a1ebe300668c7079a3/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp

Labels: Merge-Request-63
Requesting a merge for the commit in comment 11. It's a small typo fix and I've verified the fix on canary.
Labels: Merge-Approved-63
Labels: -Merge-Request-63
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 18 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b9948e3ab13b07ce2414af9c60d7262a106d3db

commit 6b9948e3ab13b07ce2414af9c60d7262a106d3db
Author: Emily Stark <estark@google.com>
Date: Wed Oct 18 06:29:02 2017

Fix origin serialization in Symantec console message

The message was using scheme+host+port but not properly accounting for
default/absent ports, so origins were printing as, e.g.,
"https://weather.com:". Oops. Now the message actually constructs an origin and
gets its string form and doesn't try to serialize an origin itself.

TBR=jochen@chromium.org

Bug:  770406 
Change-Id: I3d6e3a27957935d14f3acd7206a62e04a041dc0c
Reviewed-on: https://chromium-review.googlesource.com/718837
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#508724}(cherry picked from commit 90efb744873acd59517c78a1ebe300668c7079a3)
Reviewed-on: https://chromium-review.googlesource.com/725259
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#48}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/6b9948e3ab13b07ce2414af9c60d7262a106d3db/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/6b9948e3ab13b07ce2414af9c60d7262a106d3db/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp

how about highlighting these requests in the network panel, with a new toolbar filter option "warnings"? 
@zac, not a bad idea. but it'll require some fairly significant eng work to pull off. Can you file a new issue for this? 
It'll be interesting to see the rollout of these warnings. :)

Comment 18 Deleted

Labels: Hotlist-EnamelAndFriendsFixIt
Status: Fixed (was: Started)

Sign in to add a comment