Issue 5792731695677440: Fix named pipe security on Windows 7 (Closed)
|
Created:
May 13, 2014, 12:21 p.m. by Oleksandr Modified:
July 18, 2014, 2:09 p.m. Visibility:
Public. |
DescriptionFix named pipe security on Windows 7
Patch Set 1 #
Total comments: 10
Patch Set 2 : Refactor to eliminate the corrupt ACL bug. Address comments. #
Total comments: 15
Patch Set 3 : Use custom free function. Make sure sure SACL is released correctly as well. #
Total comments: 12
Patch Set 4 : Nits addressed #Patch Set 5 : Hide the spelling fail #MessagesTotal messages: 21
http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:105: std::tr1::shared_ptr<ACL> sharedAcl(static_cast<ACL*>(acl), LocalFree); // Just to simplify cleanup If this is just to simplify cleanup, why not use unique_ptr? https://stackoverflow.com/a/9893418/785541 http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:180: Nit: don't add an empty line here. http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:221: DWORD err = GetLastError(); Why create a variable and not use it then? http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:222: throw std::runtime_error("Client failed to connect: error " + GetLastError()); Does this actually work? IMHO this adds an int to char* rather than doing string concatenation, cast to str::string first? http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Utils.cpp:34: return osvi.dwMajorVersion >= 6 && osvi.dwMinorVersion >= 2; So if dwMajorVersion is 7 and dwMinorVersion is 0 then it is older than Windows 8?
On 2014/05/15 07:36:18, Wladimir Palant wrote: > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > File src/shared/Communication.cpp (right): > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > src/shared/Communication.cpp:105: std::tr1::shared_ptr<ACL> > sharedAcl(static_cast<ACL*>(acl), LocalFree); // Just to simplify cleanup > If this is just to simplify cleanup, why not use unique_ptr? > https://stackoverflow.com/a/9893418/785541 > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > src/shared/Communication.cpp:180: > Nit: don't add an empty line here. > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > src/shared/Communication.cpp:221: DWORD err = GetLastError(); > Why create a variable and not use it then? > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > src/shared/Communication.cpp:222: throw std::runtime_error("Client failed to > connect: error " + GetLastError()); > Does this actually work? IMHO this adds an int to char* rather than doing string > concatenation, cast to str::string first? > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > File src/shared/Utils.cpp (right): > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > src/shared/Utils.cpp:34: return osvi.dwMajorVersion >= 6 && osvi.dwMinorVersion > >= 2; > So if dwMajorVersion is 7 and dwMinorVersion is 0 then it is older than Windows > 8? Can you address Wladimir's comments Oleksandr? Then I'll look over it too.
http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:58: std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptor(PSID logonSid) We can use unique_ptr here. VS2012 supports it. It's admittedly a more extensive modification, since we'd need to change the callers also, but as long as we're touching code with auto_ptr, we can take the opportunity to eliminate an occurrence of this de-facto-deprecated class. http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:76: std::tr1::shared_ptr<SID> sharedAllAppContainersSid; This declaration doesn't need to be separate, but can be combined with its single use below. http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:93: sharedAllAppContainersSid.reset(static_cast<SID*>(allAppContainersSid), FreeSid); // Just to simplify cleanup We can use a constructor here instead of reset(). We can also use unique_ptr, which can specify a deleter as a constructor argument. http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Utils.cpp:28: bool IsWindows8OrLater() How about combining the duplicated code to get the version number into a single function that returns std::unique_ptr<OSVERSIONINFOEX>?
http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... src/shared/Communication.cpp:58: std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptor(PSID logonSid) On 2014/06/25 16:13:00, Eric wrote: > We can use unique_ptr here. VS2012 supports it. > > It's admittedly a more extensive modification, since we'd need to change the > callers also, but as long as we're touching code with auto_ptr, we can take the > opportunity to eliminate an occurrence of this de-facto-deprecated class. We really shouldn't mix switching from TR1 shared pointers to C++11 shared pointers in here, it'll make this change impossible to understand.
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:101: // acl has to be released after this I don't really understand, it has to be released after SetEntries? Well it's not. http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:105: // This only references the acl, not copies it I think this is not necessary, the official docs say: "The DACL is referenced by, not copied into, the security descriptor." http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:132: // Releases the DACL structure in the provided security descriptor I think the function name communicates this quite well :) http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:195: Whitespace change? http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:216: ReleaseDacl(securityAttributes.lpSecurityDescriptor); Wouldn't it'd be cleaner to put this in a custom free function for sharedSecurityDescriptor? It's scope ends right about here anyway. And that way we'd be exception safe. And we wouldn't need the "isWindowsVistaOrLater" check either. Note that we'd still need to manually free the security descriptor in that function. http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Utils.cpp:19: std::unique_ptr<OSVERSIONINFOEX> GetWindowsVersion() This seems like a refactoring, can we put it in a separate review?
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:101: // acl has to be released after this 1. This allocates a new ACL, so it has to be released. 2. That new ACL should not be released before we actually use it in CreateNamedPipe. 3. We release it after CreateNamedPipe by ReleaseDacl. On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > I don't really understand, it has to be released after SetEntries? Well it's > not. http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:105: // This only references the acl, not copies it I don't really understand your point here. Could you rephrase, please? On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > I think this is not necessary, the official docs say: "The DACL is referenced > by, not copied into, the security descriptor." http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Utils.cpp:19: std::unique_ptr<OSVERSIONINFOEX> GetWindowsVersion() It's addressing comments in this review. Shall we really separate it? On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > This seems like a refactoring, can we put it in a separate review?
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:101: // acl has to be released after this On 2014/07/03 13:51:49, Oleksandr wrote: > 1. This allocates a new ACL, so it has to be released. > 2. That new ACL should not be released before we actually use it in > CreateNamedPipe. > 3. We release it after CreateNamedPipe by ReleaseDacl. > > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > I don't really understand, it has to be released after SetEntries? Well it's > > not. > The comment is on top of "SetEntries", so I presumed it's commenting on that line, confused me a bit. How about a comment above the |PACL acl = 0;| line like: "Will be released later"? http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:105: // This only references the acl, not copies it On 2014/07/03 13:51:49, Oleksandr wrote: > I don't really understand your point here. Could you rephrase, please? > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > I think this is not necessary, the official docs say: "The DACL is referenced > > by, not copied into, the security descriptor." > My point is that the Microsoft documentation says exactly this, if it's clearly documented I don't see why we need a comment. You know I'm wary of comments :) They tend to get out of sync with the code, so we generally try to minimise comments. http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Utils.cpp:19: std::unique_ptr<OSVERSIONINFOEX> GetWindowsVersion() On 2014/07/03 13:51:49, Oleksandr wrote: > It's addressing comments in this review. Shall we really separate it? > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > This seems like a refactoring, can we put it in a separate review? > Which comment? To switch to std::unique_ptr? That has nothing to do with the issue at hand here, we shouldn't mix refactorings and bug fixes.
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Utils.cpp:19: std::unique_ptr<OSVERSIONINFOEX> GetWindowsVersion() http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... and http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... actually On 2014/07/03 13:56:33, Felix H. Dahlke wrote: > On 2014/07/03 13:51:49, Oleksandr wrote: > > It's addressing comments in this review. Shall we really separate it? > > > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > > This seems like a refactoring, can we put it in a separate review? > > > > Which comment? To switch to std::unique_ptr? That has nothing to do with the > issue at hand here, we shouldn't mix refactorings and bug fixes.
On 2014/07/03 14:00:42, Oleksandr wrote: > http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... > File src/shared/Utils.cpp (right): > > http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... > src/shared/Utils.cpp:19: std::unique_ptr<OSVERSIONINFOEX> GetWindowsVersion() > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > and > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/... > actually > On 2014/07/03 13:56:33, Felix H. Dahlke wrote: > > On 2014/07/03 13:51:49, Oleksandr wrote: > > > It's addressing comments in this review. Shall we really separate it? > > > > > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > > > This seems like a refactoring, can we put it in a separate review? > > > > > > > Which comment? To switch to std::unique_ptr? That has nothing to do with the > > issue at hand here, we shouldn't mix refactorings and bug fixes. Ah I see, didn't see semantic differences. Alright then :)
Here is yet one thing, sacl from dummy security descriptor is also removed too early, because dummy security descriptor is removed. For now it's not clear when any ACL is bound to the security descriptor or not. The Windows API here may be could cause even more insecure implementation, than it's intended for...
Comments addressed http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:105: // This only references the acl, not copies it This comment is quite important, IMHO. It's pretty non intuitive how ACL is used here, and one can make the same mistake again. Intuitively you would expect SetSecurityDescriptorDacl makes a copy of ACL for itself. On 2014/07/03 13:56:33, Felix H. Dahlke wrote: > On 2014/07/03 13:51:49, Oleksandr wrote: > > I don't really understand your point here. Could you rephrase, please? > > > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > > I think this is not necessary, the official docs say: "The DACL is > referenced > > > by, not copied into, the security descriptor." > > > > My point is that the Microsoft documentation says exactly this, if it's clearly > documented I don't see why we need a comment. You know I'm wary of comments :) > They tend to get out of sync with the code, so we generally try to minimise > comments.
I personally don't like the idea that the caller has to call cleaning functions.
I would prefer to create something like
struct SecurityDescriptor : private noncopyable
{
SECURITY_DESCRIPTOR* value; // nullptr if some error
std::error_code errorCode;
static /*unique*/SecurityDescriptorPtr Create(PSID logonSid);
private:
SecurityDescriptor();
std::array<uint8_t, SECURITY_DESCRIPTOR_MIN_LENGTH> m_valueMemory;
SmartType<PACL> m_aclMemory;
/// SACL is bound to this security descriptor, so we should keep it, while
value is used.
SmartType<PSECURITY_DESCRIPTOR> m_dummySecurityDescriptorLowMemory;
};
where SmartType<T> properly cleans the resources. At least the idea is to free
them automatically when they are not needed.
Also I don't like `AppendErrorCode` (I know that it's better, because without
the fix it is likely to crash with access violation)
- GetLastError() should be called before anything else. At least here we are
creating some strings which involves memory allocation, which can modify the
result of GetLastError()
- I would like to use std::error_code and regarding it we have to have custom
error_category (C++11) for GetLastError(). I will omit the comment about its
impl, because it's not important here.
You do have a point, and the solution you propose does look more elegant,
however it will be a really specialized class, which will hardly be used
anywhere else due to that. So the win in implementing it will be minimal, while
it will require quite a bit of time to test every aspect of it again. In this
situation I think a custom free function is almost as good and we should stick
to that, for now at least. We might create a P4 issue to refactor this in
future, if you insist.
Same goes for AppendErrorCode. It is outside the scope of this review, really,
and I'd vote for creating a P4 issue for this as well.
On 2014/07/15 15:30:50, sergei wrote:
> I personally don't like the idea that the caller has to call cleaning
functions.
> I would prefer to create something like
> struct SecurityDescriptor : private noncopyable
> {
> SECURITY_DESCRIPTOR* value; // nullptr if some error
> std::error_code errorCode;
> static /*unique*/SecurityDescriptorPtr Create(PSID logonSid);
> private:
> SecurityDescriptor();
> std::array<uint8_t, SECURITY_DESCRIPTOR_MIN_LENGTH> m_valueMemory;
> SmartType<PACL> m_aclMemory;
> /// SACL is bound to this security descriptor, so we should keep it, while
> value is used.
> SmartType<PSECURITY_DESCRIPTOR> m_dummySecurityDescriptorLowMemory;
> };
> where SmartType<T> properly cleans the resources. At least the idea is to free
> them automatically when they are not needed.
>
> Also I don't like `AppendErrorCode` (I know that it's better, because without
> the fix it is likely to crash with access violation)
> - GetLastError() should be called before anything else. At least here we are
> creating some strings which involves memory allocation, which can modify the
> result of GetLastError()
> - I would like to use std::error_code and regarding it we have to have custom
> error_category (C++11) for GetLastError(). I will omit the comment about its
> impl, because it's not important here.
On 2014/07/16 12:23:52, Oleksandr wrote: > You do have a point, and the solution you propose does look more elegant, > however it will be a really specialized class, which will hardly be used > anywhere else due to that. So the win in implementing it will be minimal, while > it will require quite a bit of time to test every aspect of it again. In this > situation I think a custom free function is almost as good and we should stick > to that, for now at least. We might create a P4 issue to refactor this in > future, if you insist. > > Same goes for AppendErrorCode. It is outside the scope of this review, really, > and I'd vote for creating a P4 issue for this as well. Seconded.
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/... src/shared/Communication.cpp:105: // This only references the acl, not copies it On 2014/07/15 14:16:55, Oleksandr wrote: > This comment is quite important, IMHO. It's pretty non intuitive how ACL is used > here, and one can make the same mistake again. Intuitively you would expect > SetSecurityDescriptorDacl makes a copy of ACL for itself. > > On 2014/07/03 13:56:33, Felix H. Dahlke wrote: > > On 2014/07/03 13:51:49, Oleksandr wrote: > > > I don't really understand your point here. Could you rephrase, please? > > > > > > On 2014/07/03 13:38:07, Felix H. Dahlke wrote: > > > > I think this is not necessary, the official docs say: "The DACL is > > referenced > > > > by, not copied into, the security descriptor." > > > > > > > My point is that the Microsoft documentation says exactly this, if it's > clearly > > documented I don't see why we need a comment. You know I'm wary of comments :) > > They tend to get out of sync with the code, so we generally try to minimise > > comments. > Alright. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:111: // Create a dummy security descriptor with low integrirty preset and refference its SACL in ours s/refference/reference/ http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:117: DWORD sdSize, saclSize, daclSize, ownerSize, primaryGroupSize; Shouldn't we initialise all the sizes to 0 here? Should be garbage currently. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:119: if ((saclSize == 0) || (sdSize == 0)) Redundant parentheses around both comparisons. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:121: DWORD err = GetLastError(); Unused variable. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:125: PACL sacl = (PACL) new byte[saclSize]; I'd actually find malloc more intuitive here. byte is not a standard type. Also, C++-style cast please :) Same below. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:131: if ((res == 0) || (absoluteDummySecurityDescriptorLow.get() == 0) || (saclSize == 0)) Redundant parentheses here as well. Also, !res would be more intuitive here IMO. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:133: DWORD err = GetLastError(); Also unused. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:201: aclDefaulted = false; Should also be that Windows FALSE macro for consistency. http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:206: delete acl; You need to use delete[], since it was allocated using new[]. Same below.
http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:111: // Create a dummy security descriptor with low integrirty preset and refference its SACL in ours Um. Refference is meant to be a verb here. On 2014/07/16 14:40:03, Felix H. Dahlke wrote: > s/refference/reference/
Just one remaining :) http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:111: // Create a dummy security descriptor with low integrirty preset and refference its SACL in ours On 2014/07/17 17:32:48, Oleksandr wrote: > Um. Refference is meant to be a verb here. > On 2014/07/16 14:40:03, Felix H. Dahlke wrote: > > s/refference/reference/ > But it's still one f too many! :)
http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/... src/shared/Communication.cpp:111: // Create a dummy security descriptor with low integrirty preset and refference its SACL in ours Oh. Totally missread your correction. On 2014/07/18 12:40:57, Felix H. Dahlke wrote: > On 2014/07/17 17:32:48, Oleksandr wrote: > > Um. Refference is meant to be a verb here. > > On 2014/07/16 14:40:03, Felix H. Dahlke wrote: > > > s/refference/reference/ > > > > But it's still one f too many! :)
LGTM!!11!! 
|
