Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1132093003:
Add metrics to record TCP/UDP connections made from Flash (Closed)
|
Created:
5 years, 7 months ago by raymes Modified:
5 years, 7 months ago CC:
asvitkine+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, timwillis, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics to record TCP/UDP connections made from Flash
BUG=472256
Committed: https://crrev.com/568fbcac71e0edcddded42eb4550fd2ad7e5e141
Cr-Commit-Position: refs/heads/master@{#329895}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #Patch Set 8 : #Messages
Total messages: 46 (8 generated)
raymes@chromium.org changed reviewers: + rsleevi@chromium.org
Sorry about the other CL, I used the wrong issue.
raymes@chromium.org changed reviewers: + jww@chromium.org
https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_socket_utils.h (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_socket_utils.h:52: void RecordFlashConnectMetric(const std::string& plugin_module_name, > Is there a reason we don't record these metrics for all plugins? I was told that we're only interested in Flash? Do you want to know about the stats including NaCl, etc? https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:652: ppapi::host::ReplyMessageContext reply_context(context); > Doesn't this only go based on the plugin's URL, not the effective > origin? > > That is, if I had an HTTP page framing an HTTPS iframe, this would > report as HTTPS, right? I'm not sure what you mean here? I'm not sure what the "effective origin" is :( This is purely based on the URL where the .swf is served from. Is that the wrong URL to use? Should we be checking all URLs in the frame hierarchy?
https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_socket_utils.h (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_socket_utils.h:52: void RecordFlashConnectMetric(const std::string& plugin_module_name, On 2015/05/11 06:44:21, raymes wrote: > > Is there a reason we don't record these metrics for all plugins? > > I was told that we're only interested in Flash? Do you want to know about the > stats including NaCl, etc? From our perspective, I believe we're pretty much solely concerned about Flash. I suppose there's no reason not to collect broader metrics though. https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:652: ppapi::host::ReplyMessageContext reply_context(context); On 2015/05/11 06:44:21, raymes wrote: > > Doesn't this only go based on the plugin's URL, not the effective > > origin? > > > > That is, if I had an HTTP page framing an HTTPS iframe, this would > > report as HTTPS, right? > > I'm not sure what you mean here? I'm not sure what the "effective origin" is :( > This is purely based on the URL where the .swf is served from. Is that the wrong > URL to use? Should we be checking all URLs in the frame hierarchy? In Blink, we do have a complex hierarchy traversal to determine what's a priveleged context (see isPrivilegedContext: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) Unfortunately, I don't believe we have an equivalent calculation for the browser process. sleevi@, should that be replicated for this?
https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_socket_utils.h (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_socket_utils.h:52: void RecordFlashConnectMetric(const std::string& plugin_module_name, On 2015/05/11 06:51:14, jww (traveling. slow.) wrote: > On 2015/05/11 06:44:21, raymes wrote: > > > Is there a reason we don't record these metrics for all plugins? > > > > I was told that we're only interested in Flash? Do you want to know about the > > stats including NaCl, etc? > > From our perspective, I believe we're pretty much solely concerned about Flash. > I suppose there's no reason not to collect broader metrics though. I don't think that's true, Joel. That is, it applies broadly to all PPAPI plugins. From what I understand, it may be that only Flash is auto-whitelisted, but given that there's at least a Peppet API for sockets makes me want to make sure we are covering all plugins equally. https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:652: ppapi::host::ReplyMessageContext reply_context(context); On 2015/05/11 06:51:14, jww (traveling. slow.) wrote: > On 2015/05/11 06:44:21, raymes wrote: > > > Doesn't this only go based on the plugin's URL, not the effective > > > origin? > > > > > > That is, if I had an HTTP page framing an HTTPS iframe, this would > > > report as HTTPS, right? > > > > I'm not sure what you mean here? I'm not sure what the "effective origin" is > :( > > This is purely based on the URL where the .swf is served from. Is that the > wrong > > URL to use? Should we be checking all URLs in the frame hierarchy? Ultimately, that's what will decide API blocking behaviour, so I think we do. That is, per spec, if an HTTP page is framing an HTTPS plugin, it *should* be blocked. WebCrypto is the sole exception that only looks at the loading page (and that's less than ideal) > > In Blink, we do have a complex hierarchy traversal to determine what's a > priveleged context (see isPrivilegedContext: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) > > Unfortunately, I don't believe we have an equivalent calculation for the browser > process. sleevi@, should that be replicated for this? Well, that's not the only way to solve it. Another would be to gather these metrics on the renderer side. I'm surprised the API security checks live in Blink - that seems to be a violation of our "don't trust the renderer" security model. But gathering this data in the renderer seems also viable to providing us quantifiable data.
https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_socket_utils.h (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_socket_utils.h:52: void RecordFlashConnectMetric(const std::string& plugin_module_name, On 2015/05/11 07:07:43, Ryan Sleevi wrote: > On 2015/05/11 06:51:14, jww (traveling. slow.) wrote: > > On 2015/05/11 06:44:21, raymes wrote: > > > > Is there a reason we don't record these metrics for all plugins? > > > > > > I was told that we're only interested in Flash? Do you want to know about > the > > > stats including NaCl, etc? > > > > From our perspective, I believe we're pretty much solely concerned about > Flash. > > I suppose there's no reason not to collect broader metrics though. > > I don't think that's true, Joel. > > That is, it applies broadly to all PPAPI plugins. From what I understand, it may > be that only Flash is auto-whitelisted, but given that there's at least a Peppet > API for sockets makes me want to make sure we are covering all plugins equally. From Tim and my perspective, we really do care specifically about Flash, since it's broadly installed on virtually all machines, so that's where abuse and bypass of other restrictions are likely to happen. However, if you see value in measuring it across the board, that certainly seems fine to me. https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:652: ppapi::host::ReplyMessageContext reply_context(context); On 2015/05/11 07:07:43, Ryan Sleevi wrote: > On 2015/05/11 06:51:14, jww (traveling. slow.) wrote: > > On 2015/05/11 06:44:21, raymes wrote: > > > > Doesn't this only go based on the plugin's URL, not the effective > > > > origin? > > > > > > > > That is, if I had an HTTP page framing an HTTPS iframe, this would > > > > report as HTTPS, right? > > > > > > I'm not sure what you mean here? I'm not sure what the "effective origin" is > > :( > > > This is purely based on the URL where the .swf is served from. Is that the > > wrong > > > URL to use? Should we be checking all URLs in the frame hierarchy? > > Ultimately, that's what will decide API blocking behaviour, so I think we do. > > That is, per spec, if an HTTP page is framing an HTTPS plugin, it *should* be > blocked. WebCrypto is the sole exception that only looks at the loading page > (and that's less than ideal) > > > > > In Blink, we do have a complex hierarchy traversal to determine what's a > > priveleged context (see isPrivilegedContext: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) > > > > Unfortunately, I don't believe we have an equivalent calculation for the > browser > > process. sleevi@, should that be replicated for this? > > Well, that's not the only way to solve it. Another would be to gather these > metrics on the renderer side. > > I'm surprised the API security checks live in Blink - that seems to be a > violation of our "don't trust the renderer" security model. But gathering this > data in the renderer seems also viable to providing us quantifiable data. There are many Web-level API security checks that live in Blink (think all of CSP, mixed content, etc.). We don't trust the renderer for any system level security decisions, but we do trust the renderer for Web spec security decisions. In theory, we should and can move them out eventually, but that's an exceedingly large undertaking, and in most cases only really meaningful once site isolation exists.
https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/pepper/pepper_socket_utils.h (right): https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/pepper/pepper_socket_utils.h:52: void RecordFlashConnectMetric(const std::string& plugin_module_name, On 2015/05/11 22:58:03, jww (traveling. slow.) wrote: > On 2015/05/11 07:07:43, Ryan Sleevi wrote: > > On 2015/05/11 06:51:14, jww (traveling. slow.) wrote: > > > On 2015/05/11 06:44:21, raymes wrote: > > > > > Is there a reason we don't record these metrics for all plugins? > > > > > > > > I was told that we're only interested in Flash? Do you want to know about > > the > > > > stats including NaCl, etc? > > > > > > From our perspective, I believe we're pretty much solely concerned about > > Flash. > > > I suppose there's no reason not to collect broader metrics though. > > > > I don't think that's true, Joel. > > > > That is, it applies broadly to all PPAPI plugins. From what I understand, it > may > > be that only Flash is auto-whitelisted, but given that there's at least a > Peppet > > API for sockets makes me want to make sure we are covering all plugins > equally. > > From Tim and my perspective, we really do care specifically about Flash, since > it's broadly installed on virtually all machines, so that's where abuse and > bypass of other restrictions are likely to happen. However, if you see value in > measuring it across the board, that certainly seems fine to me. On second thought, this is definitely more general. We should definitely apply this to all of PPAPI since, as sleevi pointed out, it could be done by anything using the API.
> > From Tim and my perspective, we really do care specifically about Flash, > since it's broadly installed on virtually all machines, so that's where > abuse and bypass of other restrictions are likely to happen. However, if > you see value in measuring it across the board, that certainly seems > fine to me. Although we expect a large percentage of the use case to be Flash, it would be preferable to see PPAPI usage across the board. On second thought, this is definitely more general. We should definitely > apply this to all of PPAPI since, as sleevi pointed out, it could be > done by anything using the API. > https://codereview.chromium.org/1132093003/ +1 On Mon, May 11, 2015 at 4:25 PM, <jww@chromium.org> wrote: > > > https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/pepper/pepper_socket_utils.h (right): > > > https://codereview.chromium.org/1132093003/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/pepper/pepper_socket_utils.h:52: void > RecordFlashConnectMetric(const std::string& plugin_module_name, > On 2015/05/11 22:58:03, jww (traveling. slow.) wrote: > >> On 2015/05/11 07:07:43, Ryan Sleevi wrote: >> > On 2015/05/11 06:51:14, jww (traveling. slow.) wrote: >> > > On 2015/05/11 06:44:21, raymes wrote: >> > > > > Is there a reason we don't record these metrics for all >> > plugins? > >> > > > >> > > > I was told that we're only interested in Flash? Do you want to >> > know about > >> > the >> > > > stats including NaCl, etc? >> > > >> > > From our perspective, I believe we're pretty much solely concerned >> > about > >> > Flash. >> > > I suppose there's no reason not to collect broader metrics though. >> > >> > I don't think that's true, Joel. >> > >> > That is, it applies broadly to all PPAPI plugins. From what I >> > understand, it > >> may >> > be that only Flash is auto-whitelisted, but given that there's at >> > least a > >> Peppet >> > API for sockets makes me want to make sure we are covering all >> > plugins > >> equally. >> > > From Tim and my perspective, we really do care specifically about >> > Flash, since > >> it's broadly installed on virtually all machines, so that's where >> > abuse and > >> bypass of other restrictions are likely to happen. However, if you see >> > value in > >> measuring it across the board, that certainly seems fine to me. >> > > On second thought, this is definitely more general. We should definitely > apply this to all of PPAPI since, as sleevi pointed out, it could be > done by anything using the API. > > https://codereview.chromium.org/1132093003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've updated the patch. PTAL and let me know your thoughts. The metric still lives in the browser process. The reason is that the socket APIs don't involve the renderer process at all. Instead we send whether the plugin is living in a privileged context over to the browser. This relies on another patch to expose isPrivilegedContext in blink: https://codereview.chromium.org/1136883003
https://codereview.chromium.org/1132093003/diff/20001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/20001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:1006: } else { shouldn't this be "else if (pp_result == PP_OK)" or alternatively shouldn't you factor out the pp_result == PP_OK to an 'if' surrounding the whole thing? That is, I think you only want to record Pepper_TCPConnect_Insecure if pp_result == PP_OK. https://codereview.chromium.org/1132093003/diff/20001/content/common/pepper_r... File content/common/pepper_renderer_instance_data.cc (right): https://codereview.chromium.org/1132093003/diff/20001/content/common/pepper_r... content/common/pepper_renderer_instance_data.cc:22: is_privileged_context(privileged) { Given the name of this variable, there should probably be a comment somewhere, maybe in the header, making it clear that this is untrusted data from the renderer, but that's okay because we're only using it for metrics purposes. We want to really make sure that something actually security critical doesn't mistakenly use this to make an important decision. https://codereview.chromium.org/1132093003/diff/20001/content/renderer/pepper... File content/renderer/pepper/host_dispatcher_wrapper.cc (right): https://codereview.chromium.org/1132093003/diff/20001/content/renderer/pepper... content/renderer/pepper/host_dispatcher_wrapper.cc:93: bool is_privileged_context = I find this variable name weird given that it's the && of isPrivileged context *with* the security state of the plugin URL. That is, the *context* is the document (since ExecutionContext is a formal Blink term), and the context can be privileged without the plugin being on a secure URL. Can we come up with a better name that reflects this? https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10311: <owner>Please list the metric's owners. Add more owner tags as needed.</owner> You should fill in <owner> and <description> for each of these. https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10320: <action name="Pepper_UDPConnect_Insecure"> I don't see where the two UDP actions are ever recorded.
https://codereview.chromium.org/1132093003/diff/20001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/20001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:1006: } else { On 2015/05/12 02:47:47, jww (traveling. slow.) wrote: > shouldn't this be "else if (pp_result == PP_OK)" or alternatively shouldn't you > factor out the pp_result == PP_OK to an 'if' surrounding the whole thing? That > is, I think you only want to record Pepper_TCPConnect_Insecure if pp_result == > PP_OK. Oops, done. I actually took out the check altogether. I was in two minds about it but I've decided just to record all attempts. https://codereview.chromium.org/1132093003/diff/20001/content/common/pepper_r... File content/common/pepper_renderer_instance_data.cc (right): https://codereview.chromium.org/1132093003/diff/20001/content/common/pepper_r... content/common/pepper_renderer_instance_data.cc:22: is_privileged_context(privileged) { On 2015/05/12 02:47:48, jww (traveling. slow.) wrote: > Given the name of this variable, there should probably be a comment somewhere, > maybe in the header, making it clear that this is untrusted data from the > renderer, but that's okay because we're only using it for metrics purposes. We > want to really make sure that something actually security critical doesn't > mistakenly use this to make an important decision. Good point. I added a comment. https://codereview.chromium.org/1132093003/diff/20001/content/renderer/pepper... File content/renderer/pepper/host_dispatcher_wrapper.cc (right): https://codereview.chromium.org/1132093003/diff/20001/content/renderer/pepper... content/renderer/pepper/host_dispatcher_wrapper.cc:93: bool is_privileged_context = On 2015/05/12 02:47:48, jww (traveling. slow.) wrote: > I find this variable name weird given that it's the && of isPrivileged context > *with* the security state of the plugin URL. That is, the *context* is the > document (since ExecutionContext is a formal Blink term), and the context can be > privileged without the plugin being on a secure URL. > > Can we come up with a better name that reflects this? Done. https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10311: <owner>Please list the metric's owners. Add more owner tags as needed.</owner> On 2015/05/12 02:47:48, jww (traveling. slow.) wrote: > You should fill in <owner> and <description> for each of these. Will do - I was waiting for the other parts of the CL to be settled on first.
Thanks for the changes. lgtm, with sleevi and OWNER approval. https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10320: <action name="Pepper_UDPConnect_Insecure"> On 2015/05/12 02:47:48, jww (traveling. slow.) wrote: > I don't see where the two UDP actions are ever recorded. Scratch that. Not sure how I missed it the first time :-P
Thanks for the fast work raymes and jww! On Mon, May 11, 2015 at 10:10 PM, <jww@chromium.org> wrote: > Thanks for the changes. lgtm, with sleevi and OWNER approval. > > > > https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... > File tools/metrics/actions/actions.xml (right): > > > https://codereview.chromium.org/1132093003/diff/20001/tools/metrics/actions/a... > tools/metrics/actions/actions.xml:10320: <action > name="Pepper_UDPConnect_Insecure"> > On 2015/05/12 02:47:48, jww (traveling. slow.) wrote: > >> I don't see where the two UDP actions are ever recorded. >> > > Scratch that. Not sure how I missed it the first time :-P > > https://codereview.chromium.org/1132093003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
+site-isolation-reviews So we actually have origin information in the browser process already, though it's not yet exposed outside of content. You can go from render process ID + frame ID -> RenderFrameHost. A RenderFrameHostImpl has a FrameTreeNode, which has the origin information for that frame. nasko/creis, this CL seems like a good reason to experiment with exposing it? =)
Would that still involve porting the code from Document::IsPrivilegedContext?
On 2015/05/12 at 08:50:00, raymes wrote: > Would that still involve porting the code from Document::IsPrivilegedContext? Yes, the logic that drives isPrivilegedContext would need to be ported over.
On 2015/05/12 07:14:29, dcheng wrote: > +site-isolation-reviews > > So we actually have origin information in the browser process already, though > it's not yet exposed outside of content. You can go from render process ID + > frame ID -> RenderFrameHost. A RenderFrameHostImpl has a FrameTreeNode, which > has the origin information for that frame. > > nasko/creis, this CL seems like a good reason to experiment with exposing it? =) Exposing the origin from FrameTreeNode sounds fine to me, but the problem is that it is only known on the UI thread. It might be even more correct to just use the RenderFrameHost that sent the IPC for its current URL, as part of the check uses it. The code being modified here runs on the IO thread and doesn't have access to RenderFrameHost. If the data collection can be moved over to the UI thread, it is doable.
Just some drivebys, but overall looks good to me. Hopefully we can get this landed in time for M44. https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: auto* data = instance_map_.get(instance); nit: why the auto*? Why not let auto bind to the appropriate type? https://codereview.chromium.org/1132093003/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_browser_connection.cc (right): https://codereview.chromium.org/1132093003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_browser_connection.cc:46: bool is_privileged_context = false; Why is this?
https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: auto* data = instance_map_.get(instance); On 2015/05/12 22:56:05, Ryan Sleevi wrote: > nit: why the auto*? Why not let auto bind to the appropriate type? (I'm not raymes@ but I personally prefer auto* when appropriate, to make it clear that there's no accidental copy happening)
On 2015/05/12 23:56:20, dcheng wrote: > https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... > File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): > > https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... > content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: auto* data > = instance_map_.get(instance); > On 2015/05/12 22:56:05, Ryan Sleevi wrote: > > nit: why the auto*? Why not let auto bind to the appropriate type? > > (I'm not raymes@ but I personally prefer auto* when appropriate, to make it > clear that there's no accidental copy happening) As I mentioned to dcheng yesterday, while I agree it would make sense to ultimately move the isPrivilegedContext logic to the browser, I'm not sure this is the right time to do that. It seems like a non-trivial refactor, and this is just UMA collection. nasko, dcheng, how do you feel about separating that work out into a separate issue?
https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: auto* data = instance_map_.get(instance); On 2015/05/12 22:56:05, Ryan Sleevi wrote: > nit: why the auto*? Why not let auto bind to the appropriate type? I copied the code from other functions in here. I've changed it. https://codereview.chromium.org/1132093003/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_browser_connection.cc (right): https://codereview.chromium.org/1132093003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_browser_connection.cc:46: bool is_privileged_context = false; On 2015/05/12 22:56:06, Ryan Sleevi wrote: > Why is this? In process plugins are deprecated and the only in-process plugin that exists is the the "NaCl plugin" which will never need to know this. I've updated the comments.
https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: auto* data = instance_map_.get(instance); based on dcheng's comment, I left it as-is :P but really I don't feel strongly.
On 2015/05/13 at 00:05:23, jww wrote: > On 2015/05/12 23:56:20, dcheng wrote: > > https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right): > > > > https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... > > content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: auto* data > > = instance_map_.get(instance); > > On 2015/05/12 22:56:05, Ryan Sleevi wrote: > > > nit: why the auto*? Why not let auto bind to the appropriate type? > > > > (I'm not raymes@ but I personally prefer auto* when appropriate, to make it > > clear that there's no accidental copy happening) > > As I mentioned to dcheng yesterday, while I agree it would make sense to ultimately move the isPrivilegedContext logic to the browser, I'm not sure this is the right time to do that. It seems like a non-trivial refactor, and this is just UMA collection. nasko, dcheng, how do you feel about separating that work out into a separate issue? Since it sounds like we want this to make M44, I'm OK with the current approach. But I think isPrivilegedContext is actually one of the easier things to port, so it'd be nice if we could take a stab at this sooner rather than later =)
Good times. Anything else we're waiting on to land this change? Looks like we're good to go from my side. On Tue, May 12, 2015 at 5:13 PM, <dcheng@chromium.org> wrote: > On 2015/05/13 at 00:05:23, jww wrote: > >> On 2015/05/12 23:56:20, dcheng wrote: >> > >> > > https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... > >> > File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc >> > (right): > >> > >> > >> > > https://codereview.chromium.org/1132093003/diff/40001/content/browser/rendere... > >> > content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:146: >> auto* >> > data > >> > = instance_map_.get(instance); >> > On 2015/05/12 22:56:05, Ryan Sleevi wrote: >> > > nit: why the auto*? Why not let auto bind to the appropriate type? >> > >> > (I'm not raymes@ but I personally prefer auto* when appropriate, to >> make it >> > clear that there's no accidental copy happening) >> > > As I mentioned to dcheng yesterday, while I agree it would make sense to >> > ultimately move the isPrivilegedContext logic to the browser, I'm not sure > this > is the right time to do that. It seems like a non-trivial refactor, and > this is > just UMA collection. nasko, dcheng, how do you feel about separating that > work > out into a separate issue? > > Since it sounds like we want this to make M44, I'm OK with the current > approach. > But I think isPrivilegedContext is actually one of the easier things to > port, so > it'd be nice if we could take a stab at this sooner rather than later =) > > https://codereview.chromium.org/1132093003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > Since it sounds like we want this to make M44, I'm OK with the current > > approach. > > But I think isPrivilegedContext is actually one of the easier things to > > port, so > > it'd be nice if we could take a stab at this sooner rather than later =) Sounds good. If jww@ or some other security person is able to implement isPrivilegedContext in chrome land I'm happy to move this code to use it. Tim: I'm waiting on an lg from rsleevi and also this needs to be reviewed and landed first: https://codereview.chromium.org/1136883003/
@raymes: Thanks for the context. rsleevi@ informes me that he doesn't need to lg this and all you need is OWNER approval. Also, good to see that https://codereview.chromium.org/1136883003/ has landed. On Tue, May 12, 2015 at 5:41 PM, <raymes@chromium.org> wrote: > > Since it sounds like we want this to make M44, I'm OK with the current >> > approach. >> > But I think isPrivilegedContext is actually one of the easier things to >> > port, so >> > it'd be nice if we could take a stab at this sooner rather than later =) >> > > Sounds good. If jww@ or some other security person is able to implement > isPrivilegedContext in chrome land I'm happy to move this code to use it. > > Tim: I'm waiting on an lg from rsleevi and also this needs to be reviewed > and > landed first: https://codereview.chromium.org/1136883003/ > > https://codereview.chromium.org/1132093003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
raymes@chromium.org changed reviewers: + isherman@chromium.org, jam@chromium.org, tsepez@chromium.org
+OWNERS +isherman for tools/metrics/actions/actions.xml +jam for content/common/pepper_renderer_instance_data.* +tsepez for content/common/view_messages.h
Hmm, why did you choose to use UMA actions rather than histograms? Histograms are typically (a) more efficient, and (b) easier to use for analysis.
Messages LGTM
isherman: Thanks I didn't really know about enumerated histograms. I've updated the CL accordingly.
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1132093003/#ps100001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132093003/100001
Thanks! Metrics lgtm % suggestions: https://codereview.chromium.org/1132093003/diff/100001/content/browser/render... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/100001/content/browser/render... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:1010: pepper_socket_utils::PLUGIN_CONTEXT_SECURITY_NUM_ENTRIES); nit: Since you're currently tracking a boolean value, you could use UMA_HISTOGRAM_BOOLEAN instead, to save on some boilerplate. If you were to ever decide to add more buckets to the histogram, it would be safe to switch over to UMA_HISTOGRAM_ENUMERATION at that time. https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:26589: + Number of Pepper TCP connect bind attempts from a plugin in a secure origin. nit: I'd phrase this as something closer to "Whether a Pepper TCP connect bind attempt comes from a plugin in a secure or an insecure origin". Definitely feel free to reword; the main idea that I'm trying to convey is that this histogram measures the possible security states of a connection attempt. https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:26599: + Number of Pepper UDP socket bind attempts from a plugin in a secure origin. (Same idea applies here as well.) https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60215: + <int value="1" label="PLUGIN_CONTEXT_INSECURE"/> nit: You can use these labels if you'd like, but I'd recommend just "Secure" and "Insecure", as they're more human-reader friendly.
Thanks! 1 question below :) https://codereview.chromium.org/1132093003/diff/100001/content/browser/render... File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/1132093003/diff/100001/content/browser/render... content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:1010: pepper_socket_utils::PLUGIN_CONTEXT_SECURITY_NUM_ENTRIES); On 2015/05/14 02:26:01, Ilya Sherman wrote: > nit: Since you're currently tracking a boolean value, you could use > UMA_HISTOGRAM_BOOLEAN instead, to save on some boilerplate. If you were to ever > decide to add more buckets to the histogram, it would be safe to switch over to > UMA_HISTOGRAM_ENUMERATION at that time. Done. https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:26589: + Number of Pepper TCP connect bind attempts from a plugin in a secure origin. On 2015/05/14 02:26:01, Ilya Sherman wrote: > nit: I'd phrase this as something closer to "Whether a Pepper TCP connect bind > attempt comes from a plugin in a secure or an insecure origin". Definitely feel > free to reword; the main idea that I'm trying to convey is that this histogram > measures the possible security states of a connection attempt. Done. https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:26599: + Number of Pepper UDP socket bind attempts from a plugin in a secure origin. On 2015/05/14 02:26:01, Ilya Sherman wrote: > (Same idea applies here as well.) Done. https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60215: + <int value="1" label="PLUGIN_CONTEXT_INSECURE"/> On 2015/05/14 02:26:01, Ilya Sherman wrote: > nit: You can use these labels if you'd like, but I'd recommend just "Secure" and > "Insecure", as they're more human-reader friendly. Done. Can I keep the enum here with UMA_HISTOGRAM_BOOLEAN? Or should I change it to a boolean type?
https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60215: + <int value="1" label="PLUGIN_CONTEXT_INSECURE"/> On 2015/05/14 02:47:44, raymes wrote: > On 2015/05/14 02:26:01, Ilya Sherman wrote: > > nit: You can use these labels if you'd like, but I'd recommend just "Secure" > and > > "Insecure", as they're more human-reader friendly. > > Done. Can I keep the enum here with UMA_HISTOGRAM_BOOLEAN? Or should I change it > to a boolean type? You can keep the enum here. UMA_HISTOGRAM_BOOLEAN is really just a special case of UMA_HISTOGRAM_ENUMERATION. The "BooleanFoo" enum types in this file are just named such by convention. That said, I probably would go with "BooleanSecure" as the enum name in this case, since that's a concept that might be useful for other histograms as well. histograms.xml is just a metadata file that drives the dashboard UI, so it's easy to rename the enum type later if need be.
https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1132093003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60215: + <int value="1" label="PLUGIN_CONTEXT_INSECURE"/> Done. Ahh that makes sense! Thanks!!
On 2015/05/13 23:46:00, raymes wrote: > +OWNERS > > +isherman for > tools/metrics/actions/actions.xml > > +jam for > content/common/pepper_renderer_instance_data.* lgtm > > +tsepez for > content/common/view_messages.h
The CQ bit was checked by timwillis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org, tsepez@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1132093003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132093003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/568fbcac71e0edcddded42eb4550fd2ad7e5e141 Cr-Commit-Position: refs/heads/master@{#329895} |
