Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1067233002:
Added a trace option for whitewashing trace args against a known-PII-less list. (Closed)
|
Created:
5 years, 8 months ago by oystein (OOO til 10th of July) Modified:
5 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description(CLOSED: Folded into https://codereview.chromium.org/1115343002/)
Patch Set 1 : #
Total comments: 3
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Fixed NaCl build #Patch Set 5 : Just base/trace/event/* #
Total comments: 1
Patch Set 6 : Removed enable_args_whitelist #Patch Set 7 : Removed enable_args_whitelist #
Messages
Total messages: 25 (7 generated)
Patchset #1 (id:1) has been deleted
First pass at this.
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event_impl_constants.cc (right): https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl_constants.cc:29: const char* const TraceEvent::kEventArgsWhitelist[][2] = { Maybe it's just me but it feels like this list should be part of the trace config/filter?
https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event_impl_constants.cc (right): https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl_constants.cc:29: const char* const TraceEvent::kEventArgsWhitelist[][2] = { On 2015/04/09 14:46:45, Sami wrote: > Maybe it's just me but it feels like this list should be part of the trace > config/filter? The whitelist is essentially args we've manually vetted to not contain any PII, I'm not sure if this can/should vary between traces?
friendly ping!
https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event_impl_constants.cc (right): https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl_constants.cc:29: const char* const TraceEvent::kEventArgsWhitelist[][2] = { On 2015/04/09 18:00:29, Oystein wrote: > On 2015/04/09 14:46:45, Sami wrote: > > Maybe it's just me but it feels like this list should be part of the trace > > config/filter? > > The whitelist is essentially args we've manually vetted to not contain any PII, > I'm not sure if this can/should vary between traces? I was just thinking that base/ seems like an odd place to hardcode this since if effectively relies on global knowledge, but I don't feel too strongly about it.
On 2015/04/10 11:08:36, Sami wrote: > https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... > File base/trace_event/trace_event_impl_constants.cc (right): > > https://codereview.chromium.org/1067233002/diff/80001/base/trace_event/trace_... > base/trace_event/trace_event_impl_constants.cc:29: const char* const > TraceEvent::kEventArgsWhitelist[][2] = { > On 2015/04/09 18:00:29, Oystein wrote: > > On 2015/04/09 14:46:45, Sami wrote: > > > Maybe it's just me but it feels like this list should be part of the trace > > > config/filter? > > > > The whitelist is essentially args we've manually vetted to not contain any > PII, > > I'm not sure if this can/should vary between traces? > > I was just thinking that base/ seems like an odd place to hardcode this since if > effectively relies on global knowledge, but I don't feel too strongly about it. It is a good point, I'm just not sure which place would be more appropriate :/. The entries can come from all over the place (content, chrome, v8, blink...). We could place it in chrome, but the non-chrome events could make sense for other embedders too. And I think maintaining separate lists would be pretty awkward. Suggestions welcome :).
Is it okay for a compromised renderer process to be in charge of whitelist stripping? Implicit in this patch is a choice that pii-ification is done in the local processes... I'm not sure thats the right call. If this is part of appending-time operations, then I tend to think it should be a pluggable. base would have a setBlahFilter, and when you start tracing, you'd set a filter. But then when it crosses processes, you have a problelm. So architecturally, I dont like this route at all. Personally, I'd like this to be a pass that we do on the file once its been obtained, as part of uploading. That way we trust the pii-ification, and it can be a browser-side-only feature. It seems like these traces are going to be smallish so presumably we can afford a parse again to strip args?
On 2015/04/14 19:20:05, nduca wrote: > Is it okay for a compromised renderer process to be in charge of whitelist > stripping? Implicit in this patch is a choice that pii-ification is done in the > local processes... I'm not sure thats the right call. > Why is that a problem? A compromised renderer process could shove anything it wanted into the JSON anyway, and in either case it can't control the upload destination of the trace. > If this is part of appending-time operations, then I tend to think it should be > a pluggable. base would have a setBlahFilter, and when you start tracing, you'd > set a filter. But then when it crosses processes, you have a problelm. So > architecturally, I dont like this route at all. > > > Personally, I'd like this to be a pass that we do on the file once its been > obtained, as part of uploading. That way we trust the pii-ification, and it can > be a browser-side-only feature. > > It seems like these traces are going to be smallish so presumably we can afford > a parse again to strip args? Smallish post-compression. It could be rather large pre-compression given N tabs, and and a post-processing step could thus be somewhat heavy. I'd also rather we do as little browser-side parsing/handling of untrusted data as we possibly can.
Alright. I'm not particularly wild about this approach but its not something I want to argue heavily. You should go for content/ because we want this to work for clank eventually and clank doesn't use chrome. Base should have an ArgFilter predicate that you can set, then child tracing filter should set it when tracing starts.
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
v2 uploaded; no tests yet, holding off on that until I get yea/nay on the approach. On 2015/04/14 21:48:54, nduca wrote: > Alright. I'm not particularly wild about this approach but its not something I > want to argue heavily. > > You should go for content/ because we want this to work for clank eventually and > clank doesn't use chrome. > > Base should have an ArgFilter predicate that you can set, then child tracing > filter should set it when tracing starts. Like so?
On 2015/04/21 23:43:08, Oystein wrote: > v2 uploaded; no tests yet, holding off on that until I get yea/nay on the > approach. > > On 2015/04/14 21:48:54, nduca wrote: > > Alright. I'm not particularly wild about this approach but its not something I > > want to argue heavily. > > > > You should go for content/ because we want this to work for clank eventually > and > > clank doesn't use chrome. > > > > Base should have an ArgFilter predicate that you can set, then child tracing > > filter should set it when tracing starts. > > Like so? friendly ping for feedback on general approach :)
Thanks, I think I like the predicate approach better than the previously hardcoded list. Others?
i think it'd be good to have three patches: 1) add predicate to trace event 2) add content whitelist 3) hook it up to trace options
(the approach seems fine, though i think there're minor tweaks that need doing that will become clearer when we have smaller pieces in each patch)
On 2015/04/29 16:37:46, nduca wrote: > (the approach seems fine, though i think there're minor tweaks that need doing > that will become clearer when we have smaller pieces in each patch) Done; this CL is now just the base/trace_event/* portion. ptal.
(also, enable_args_whitelist -> strip_non_whitelisted_args? that's a nit and wouldn't have held an lg) https://codereview.chromium.org/1067233002/diff/200001/base/trace_event/trace... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1067233002/diff/200001/base/trace_event/trace... base/trace_event/trace_event_impl.h:396: enable_args_whitelist(false) {} Should devtools protocol support this enable_args_whitelist? i was good to lg the rest of this patch except for my indecision around whether anytime we add a trace option we should update devtools protocol to accept it. i had originally requested that the enable_Args_whitelist changew as a separate patch so that i could add pavel to the codereview to ask for his advice. i'd still kinda like to do that. we could add him to this total patch, but the "proper" thing to do codereview wise is to have a patch that is one logical thought at a time. in this case, we still have two thoughts in the patch: one about predicate, one about the new option. so thats making this hard to give good review to. help, need advice on how to proceed.
On 2015/05/01 03:09:03, nduca wrote: > (also, enable_args_whitelist -> strip_non_whitelisted_args? that's a nit and > wouldn't have held an lg) > > https://codereview.chromium.org/1067233002/diff/200001/base/trace_event/trace... > File base/trace_event/trace_event_impl.h (right): > > https://codereview.chromium.org/1067233002/diff/200001/base/trace_event/trace... > base/trace_event/trace_event_impl.h:396: enable_args_whitelist(false) {} > Should devtools protocol support this enable_args_whitelist? i was good to lg > the rest of this patch except for my indecision around whether anytime we add a > trace option we should update devtools protocol to accept it. > > i had originally requested that the enable_Args_whitelist changew as a separate > patch so that i could add pavel to the codereview to ask for his advice. > > i'd still kinda like to do that. we could add him to this total patch, but the > "proper" thing to do codereview wise is to have a patch that is one logical > thought at a time. in this case, we still have two thoughts in the patch: one > about predicate, one about the new option. so thats making this hard to give > good review to. > > help, need advice on how to proceed. Sure, no problem; I've stripped the enable_args_whitelist stuff out now.
ping :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
