Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: src/shared/Communication.cpp

Issue 11756012: Enhanced Protected Mode support (Closed)
Patch Set: Created Sept. 15, 2013, 1 a.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« src/shared/Communication.h ('K') | « src/shared/Communication.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/shared/Communication.cpp
===================================================================
--- a/src/shared/Communication.cpp
+++ b/src/shared/Communication.cpp
@@ -1,9 +1,17 @@
#include <Windows.h>
#include <Lmcons.h>
#include <Sddl.h>
+#include <aclapi.h>
#include "Communication.h"
#include "Utils.h"
+#include <strsafe.h>
Wladimir Palant 2013/09/16 13:45:07 Please move that up, to the built-in include files
+
+#ifndef SECURITY_APP_PACKAGE_AUTHORITY
+#define SECURITY_APP_PACKAGE_AUTHORITY {0,0,0,0,0,15}
+#endif
+
+std::wstring Communication::browserSID;
namespace
{
@@ -25,6 +33,132 @@
throw std::runtime_error(AppendErrorCode("Failed to get the current user's name"));
return std::wstring(buffer.get(), length);
}
+
+ // See http://msdn.microsoft.com/en-us/library/windows/desktop/hh448493(v=vs.85).aspx
+ bool GetLogonSid (HANDLE hToken, PSID *ppsid)
Felix Dahlke 2013/09/16 16:30:12 Nit: No space after GetLogonSid?
+ {
+ DWORD dwLength = 0;
+ PTOKEN_GROUPS ptg = NULL;
+
+ // Verify the parameter passed in is not NULL.
Felix Dahlke 2013/09/16 16:30:12 I do think this is kinda obvious from the conditio
+ if (NULL == ppsid)
Felix Dahlke 2013/09/16 16:30:12 We normally don't use Yoda conditions :D But it's
+ return false;
+
+ // Get required buffer size and allocate the TOKEN_GROUPS buffer.
+ if (!GetTokenInformation(hToken, TokenLogonSid, (LPVOID) ptg, 0, &dwLength))
Felix Dahlke 2013/09/16 16:30:12 This code looks very similar to what's in AdblockP
+ {
+ if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
+ return false;
+
+ ptg = (PTOKEN_GROUPS)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);
+
+ if (ptg == NULL)
+ return false;
+ }
+
+ // Get the token group information from the access token.
+ if (!GetTokenInformation(hToken, TokenLogonSid, (LPVOID) ptg, dwLength, &dwLength) || ptg->GroupCount != 1)
+ {
+ HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+ return false;
+ }
+
+ // Found the logon SID; make a copy of it.
+ dwLength = GetLengthSid(ptg->Groups[0].Sid);
+ *ppsid = (PSID) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);
+ if (*ppsid == NULL)
+ {
+ HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+ return false;
+ }
+ if (!CopySid(dwLength, *ppsid, ptg->Groups[0].Sid))
+ {
+ HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+ HeapFree(GetProcessHeap(), 0, (LPVOID)*ppsid);
+ return false;
+ }
+
+ HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+ return true;
+ }
+
+ bool CreateObjectSecurityDescriptor(PSID pLogonSid, PSECURITY_DESCRIPTOR* ppSD)
+ {
+ BOOL bSuccess = FALSE;
Felix Dahlke 2013/09/16 16:30:12 I'd rather have these declarations closer to their
+ DWORD dwRes;
+ PSID pBrowserSID = NULL;
+ PACL pACL = NULL;
+ PSECURITY_DESCRIPTOR pSD = NULL;
+ EXPLICIT_ACCESS ea[2];
+ SID_IDENTIFIER_AUTHORITY ApplicationAuthority = SECURITY_APP_PACKAGE_AUTHORITY;
+
+ ConvertStringSidToSid(Communication::browserSID.c_str(), &pBrowserSID);
+
+ // Initialize an EXPLICIT_ACCESS structure for an ACE.
+ // The ACE will allow LogonSid generic all access
+ ZeroMemory(&ea, 2 * sizeof(EXPLICIT_ACCESS));
+ ea[0].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
+ ea[0].grfAccessMode = SET_ACCESS;
+ ea[0].grfInheritance= NO_INHERITANCE;
+ ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+ ea[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
+ ea[0].Trustee.ptstrName = (LPTSTR) pLogonSid;
+
+ // Initialize an EXPLICIT_ACCESS structure for an ACE.
+ // The ACE will give the browser SID all permissions
+ ea[1].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
+ ea[1].grfAccessMode = SET_ACCESS;
+ ea[1].grfInheritance= NO_INHERITANCE;
+ ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+ ea[1].Trustee.TrusteeType = TRUSTEE_IS_GROUP;
+ ea[1].Trustee.ptstrName = (LPTSTR) pBrowserSID;
+
+ // Create a new ACL that contains the new ACEs.
+ dwRes = SetEntriesInAcl(2, ea, NULL, &pACL);
+ if (ERROR_SUCCESS != dwRes)
+ {
+ FreeSid(pBrowserSID);
+ return false;
+ }
+
+ // Initialize a security descriptor.
+ pSD = (PSECURITY_DESCRIPTOR) LocalAlloc(LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);
+ if (NULL == pSD)
+ {
+ FreeSid(pBrowserSID);
+ LocalFree(pACL);
+ return false;;
+ }
+
+ if (!InitializeSecurityDescriptor(pSD, SECURITY_DESCRIPTOR_REVISION))
+ {
+ FreeSid(pBrowserSID);
+ LocalFree(pACL);
+ LocalFree((LPVOID)pSD);
+ return false;
+ }
+
+ // Add the ACL to the security descriptor.
+ if (!SetSecurityDescriptorDacl(pSD, TRUE, pACL, FALSE)) // not a default DACL
+ {
+ FreeSid(pBrowserSID);
+ LocalFree(pACL);
+ LocalFree((LPVOID)pSD);
+ return false;
+ }
+
+ *ppSD = pSD;
+ pSD = NULL;
+
+ if (pBrowserSID)
+ FreeSid(pBrowserSID);
+ if (pACL)
+ LocalFree(pACL);
+
+ return true;
+ }
+
+
}
const std::wstring Communication::pipeName = L"\\\\.\\pipe\\adblockplusengine_" + GetUserName();
@@ -67,7 +201,6 @@
: std::runtime_error("Pipe disconnected")
{
}
-
Communication::Pipe::Pipe(const std::wstring& pipeName, Communication::Pipe::Mode mode)
Wladimir Palant 2013/09/16 13:45:07 Nit: Please keep an empty line before this one.
{
pipe = INVALID_HANDLE_VALUE;
@@ -76,25 +209,36 @@
SECURITY_ATTRIBUTES sa;
memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
+
+ HANDLE hToken = NULL;
+ OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &hToken);
+
+ if (browserSID.empty())
+ {
+ if (IsWindowsVistaOrLater())
+ {
+ // Low mandatory label. See http://msdn.microsoft.com/en-us/library/bb625958.aspx
+ LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
+ ConvertStringSecurityDescriptorToSecurityDescriptorW(accessControlEntry, SDDL_REVISION_1, &securitydescriptor, 0);
+ sa.lpSecurityDescriptor = securitydescriptor;
+ }
- PSECURITY_DESCRIPTOR securitydescriptor;
- if (IsWindowsVistaOrLater())
- {
- // Low mandatory label. See http://msdn.microsoft.com/en-us/library/bb625958.aspx
- LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
- ConvertStringSecurityDescriptorToSecurityDescriptorW(accessControlEntry, SDDL_REVISION_1, &securitydescriptor, 0);
- sa.lpSecurityDescriptor = securitydescriptor;
- }
-
- sa.bInheritHandle = TRUE;
-
- pipe = CreateNamedPipeW (pipeName.c_str(), PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
- PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &sa);
- if (IsWindowsVistaOrLater() && securitydescriptor)
- {
- LocalFree(securitydescriptor);
- }
- }
+ sa.bInheritHandle = TRUE;
+ }
+ else // IE is in AppContainer
+ {
+ PSID pLogonSid = NULL;
+ //Allowing LogonSid and IE's appcontainer.
+ if (GetLogonSid(hToken, &pLogonSid) && CreateObjectSecurityDescriptor(pLogonSid, &securitydescriptor) )
Wladimir Palant 2013/09/16 13:45:07 Why do we need to allow the logon sid? Is it for t
Oleksandr 2013/09/17 08:27:19 Our plugin is unable to access the named pipe if w
+ {
+ sa.nLength = sizeof(SECURITY_ATTRIBUTES);
+ sa.bInheritHandle = TRUE;
Wladimir Palant 2013/09/16 13:45:07 I think that we still want to set this in any case
+ sa.lpSecurityDescriptor = securitydescriptor;
+ }
+ }
+ pipe = CreateNamedPipe(pipeName.c_str(), PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
Wladimir Palant 2013/09/16 13:45:07 Please use CreateNamedPipeW explicitly, as we had
+ PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &sa);
+ }
Wladimir Palant 2013/09/16 13:45:07 Bad indentation?
else
{
pipe = CreateFileW(pipeName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0);
@@ -121,6 +265,9 @@
Communication::Pipe::~Pipe()
{
CloseHandle(pipe);
+ if (securitydescriptor)
+ LocalFree(securitydescriptor);
+
}
Communication::InputBuffer Communication::Pipe::ReadMessage()
« src/shared/Communication.h ('K') | « src/shared/Communication.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld