New issue
Advanced search Search tips

Issue 732537 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

CertTransparencyVerifier created but never used for the OTR Profile

Project Member Reported by mmenke@chromium.org, Jun 12 2017

Issue description

Currently, we create a CTVerifier for OffTheRecordProfileIOData, but never actually use it.  Instead, we use the system URLRequestContext's CTVerifier, which is set up using the exact same parameters as the one created by ProfileIOData.
 
Source links? We shouldn't be using the SystemURLRequestContext CTVerifier
Oh, making sure I understand:

We create one in ProfileIOData ( https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_io_data.h?rcl=57cbe4e5fd1f7d7c24b404b783e0587aa97cbfd3&l=565 ) but since OTRProfileIOData is derived from Profile, we don't use it in OTR

The ProfileIOData attached one has 'user state' hanging off it (c.f. https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_io_data.cc?rcl=ad604940edf862c4379cf916eaa81d3fc8b7ee36&l=1148 ) that is used to wire up the inclusion proof fetching (which stores which certs you've visited in memory, and we were exploring persisting to disk), so we didn't want to use that same OTR mechanism.

Does OTRProfileIOData use ProfileIOData::Init()? It wasn't readily apparent from https://cs.chromium.org/chromium/src/chrome/browser/profiles/off_the_record_profile_io_data.cc?rcl=57cbe4e5fd1f7d7c24b404b783e0587aa97cbfd3&l=194

Using the SystemURLRequestContext's CTVerifier is (less) problematic, because we don't wire up the system context to any SCT state, so it's just a memory optimization, but I'd rather be inclined to think we should try to separate out the OTR properly.

Hopefully that made sense :)

Comment 4 by mmenke@chromium.org, Jun 12 2017

ProfileIOData is completely shared by the OTR profile and ProfileImplIOData.  So what's happening is we create one got OTR in ProfileIOData, but then the the OTR profile overwrites the pointer to its own (Which it doesn't delete, so it just sits there in memory, unused) with a pointer to the System URLRequestContext's verifier.

I'm not seeing any user state hanging off of ProfileIOData's CTVerifier - looks like we only hook up IOThread's (global) CTLogs and a fresh TreeStateTracker.

Comment 5 by mmenke@chromium.org, Jun 12 2017

I think we should only go with things that are globally shared (CTEnforcer, NetLog, HostResolverImpl, currently), or things that are per-URLRequestContext.  From a NetworkService perspective, things that are only shared between some URLRequestContexts are weird.
The TreeStateTracker is the user-state (of sorts). It doesn't persist to disk, and is thus outside the bounds of OTR's guarantees, but does note when/what certs a user is seeing and tries to fetch details over DNS (or will).

This would be fine to be per-URC
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 12 2017

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

commit 1a6910dc3ffb95581db2b63a7ccd9b089585d323
Author: mmenke <mmenke@chromium.org>
Date: Mon Jun 12 22:29:02 2017

Make OffTheRecordProfileIOData use its own CTVerifier.

It subclasses ProfileIOData, which always makes and sets a CTVerifier
for the MainURLRequestContext, but the OffTheRecordProfileIOData was
replacing the pointer with on to the system context's CTVerifier. Both
verifiers are created with the same arguments, so this doesn't seem to
server any useful purpose.

The CTVerifier created by ProfileIOData contains no reference to
outside data, other than CTLogs and as an STH Observers, so it should
be fine to use for Incognito mode (Also note that it is not shared
between Incognito and non-Incognito).

BUG= 732537 

Review-Url: https://codereview.chromium.org/2936643004
Cr-Commit-Position: refs/heads/master@{#478792}

[modify] https://crrev.com/1a6910dc3ffb95581db2b63a7ccd9b089585d323/chrome/browser/profiles/off_the_record_profile_io_data.cc

Comment 8 by mmenke@chromium.org, Jun 13 2017

Status: Fixed (was: Assigned)
Labels: M-61

Sign in to add a comment