Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083223003:
Allow network service to query remote_security_key process for its session ID (Closed)
DescriptionAllow network service to query remote_security_key process for its session ID
This change updates the remote_security_key process such that it will add an
access right to allow processes running as Local Service to query its Windows
session ID. This change is required to allow the network service to restrict
access to global resources to the Windows session being remoted.
BUG=591746
Committed: https://crrev.com/0d3f8b65e95068329ec7fb8f97d4addbb52c99f5
Cr-Commit-Position: refs/heads/master@{#401786}
Patch Set 1 #Patch Set 2 : CL cleanup #
Total comments: 15
Patch Set 3 : Addressing feedback #
Dependent Patchsets: Messages
Total messages: 14 (5 generated)
joedow@chromium.org changed reviewers: + sergeyu@chromium.org, zijiehe@chromium.org
joedow@chromium.org changed required reviewers: + sergeyu@chromium.org
This is the second of several changes needed to use the Windows Session ID to restrict access to the security key IPC channel. PTAL! Thanks! Joe
Ping!
lgtm when my comments are addressed https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_main.cc (right): https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:43: PACL new_dacl = nullptr; move this below, next to the SetEntriesInAcl() call. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:60: LocalFree(descriptor); do you need to free old_nacl here and below? https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:75: if (ERROR_SUCCESS != SetEntriesInAcl(1, &new_access, old_dacl, &new_dacl)) { nit: move ERROR_SUCCESS to the end, i.e. (SetEntriesInAcl() != ERROR_SUCCESS). That way it would be more readable and more consistent with the rest of the code.
https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_main.cc (right): https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:28: namespace { I believe we usually add a blank line after "namespace {", except for forwarding declaration. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:29: bool AddAccessRightForWellKnownSid(WELL_KNOWN_SID_TYPE type, DWORD new_right) { I saw this function has only been used with WinLocalServiceSid and PROCESS_QUERY_LIMITED_INFORMATION combination. So do we really need the parameters? A bool ApplyQueryLimitedInformationPermission() function should be enough. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:60: LocalFree(descriptor); You can use unique_ptr with a custom deleter to avoid calling LocalFree before return. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:107: return false; The return type is int, not bool.
https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_main.cc (right): https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:60: LocalFree(descriptor); On 2016/06/23 22:35:58, Hzj_jie wrote: > You can use unique_ptr with a custom deleter to avoid calling LocalFree before > return. +1, that would be the best
Thanks! https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_main.cc (right): https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:28: namespace { On 2016/06/23 22:35:58, Hzj_jie wrote: > I believe we usually add a blank line after "namespace {", except for forwarding > declaration. Done. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:29: bool AddAccessRightForWellKnownSid(WELL_KNOWN_SID_TYPE type, DWORD new_right) { On 2016/06/23 22:35:58, Hzj_jie wrote: > I saw this function has only been used with WinLocalServiceSid and > PROCESS_QUERY_LIMITED_INFORMATION combination. So do we really need the > parameters? A bool ApplyQueryLimitedInformationPermission() function should be > enough. There are two reasons: - When someone reads the call site it is obvious what is being applied (this is true with your example but it doesn't state who the permission is being granted to, ApplyQueryLimitedInformationPermissionForLocalServiceAccount() seemed too long of a name :) - I can imagine other scenarios where we would want to add a specific ACL for a well-known SID. By writing the function this way up front, it would be easy to pull this code out into a separate header/cc and use it in multiple locations. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:43: PACL new_dacl = nullptr; On 2016/06/23 22:28:41, Sergey Ulanov wrote: > move this below, next to the SetEntriesInAcl() call. Done. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:60: LocalFree(descriptor); On 2016/06/23 22:35:58, Hzj_jie wrote: > You can use unique_ptr with a custom deleter to avoid calling LocalFree before > return. Acknowledged. I looked into this and it was more than I wanted to do for this CL. My plan is to implement a ScopedLocal type (or similar approach) with the other Windows scoped ptrs and update the other areas of the codebase with it. I'll add a TODO and a bug for now. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:60: LocalFree(descriptor); On 2016/06/23 22:28:41, Sergey Ulanov wrote: > do you need to free old_nacl here and below? old_dacl is a pointer into an opaque struct (|descriptor|). I have a comment that describes why it shouldn't be freed. I think it will be more obvious that it is intentional after I add a custom deleter. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:75: if (ERROR_SUCCESS != SetEntriesInAcl(1, &new_access, old_dacl, &new_dacl)) { On 2016/06/23 22:28:41, Sergey Ulanov wrote: > nit: move ERROR_SUCCESS to the end, i.e. (SetEntriesInAcl() != ERROR_SUCCESS). > That way it would be more readable and more consistent with the rest of the > code. Done. https://codereview.chromium.org/2083223003/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_main.cc:107: return false; On 2016/06/23 22:35:58, Hzj_jie wrote: > The return type is int, not bool. Ha, good catch :)
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2083223003/#ps40001 (title: "Addressing feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083223003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow network service to query remote_security_key process for its session ID This change updates the remote_security_key process such that it will add an access right to allow processes running as Local Service to query its Windows session ID. This change is required to allow the network service to restrict access to global resources to the Windows session being remoted. BUG=591746 ========== to ========== Allow network service to query remote_security_key process for its session ID This change updates the remote_security_key process such that it will add an access right to allow processes running as Local Service to query its Windows session ID. This change is required to allow the network service to restrict access to global resources to the Windows session being remoted. BUG=591746 Committed: https://crrev.com/0d3f8b65e95068329ec7fb8f97d4addbb52c99f5 Cr-Commit-Position: refs/heads/master@{#401786} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0d3f8b65e95068329ec7fb8f97d4addbb52c99f5 Cr-Commit-Position: refs/heads/master@{#401786} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
