Fix issues with security tokens (Enhanced Protected Mode, Protected Mode etc)

This fixes the bug IE hanging when navigated to bing.com.

The bug was caused by the fact that IE jumps out of AppContainer when navigating to bing.com. That caused us problems when communicating to AdblockPlusEngine.

Key decisions implemented here:

1. Always run AdblockPlusEngine in low integrity
2. Give access to AdblockPlusEngine to all app containers. (IE and all Modern UI apps). Giving access to specific AppContainer seems to mark the process as part of that AppContainer, which in turn blocks all requests from out-of-appcontainer.
3. Mark the named pipe as low integrity resource explicitly for cases when engine was launched with higher integrity.

[Felix Dahlke, 2013-12-10 16:46:36]

IMO, in general it's not a big deal if we allow other processes to communicate
with the engine. Other stuff installed on the system can most likely mess with
ABP anyway if they want to. And crashing on bing.com and other pages is not
really an option.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:52: else
This happens if the engine is first started from bing.com, right?

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:58: PSID pIntegritySid = 0;
Less Hungarian please :D

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:59: BOOL res =
ConvertStringSidToSid(L"S-1-16-4096", &pIntegritySid);
Since the return value is ignored, there's no need to store it.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:62: TOKEN_MANDATORY_LABEL tml = {0};
Let's go with {} for consistency's sake.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:67: res = SetTokenInformation(newToken,
TokenIntegrityLevel, &tml, sizeof(TOKEN_MANDATORY_LABEL) +
GetLengthSid(pIntegritySid));
As above, no need to store the return value.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:71: BOOL createProcRes = 0;
Why redeclare the variable here? This means that when createProcRes is checked
further below, it's always 0.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Communication.cpp:76: std::auto_ptr<SECURITY_DESCRIPTOR>
CreateSecurityDescriptorLowAndAppContainers(PSID logonSid)
I don't think we need to put the specifics of the descriptor we create into the
function name :)

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Communication.cpp:89: bool isWindows8 = IsWindows8();
Agreed, would be better to check for app container support. I'd definitely
suggest to rename isWindows8 to isAppContainerSupported or something.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Communication.cpp:93: SID_IDENTIFIER_AUTHORITY ApplicationAuthority =
SECURITY_APP_PACKAGE_AUTHORITY;
Variables should start with a lower case letter.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Communication.cpp:95: // Create a well-known SID for the all
appcontainers group.
I think it'd make sense to move this comment in front of the two declarations
above.

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Communication.cpp:100: // with engine when IE is launched with
different security settings.
with "the" engine

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Communication.cpp:130: PSECURITY_DESCRIPTOR
pDummySecurityDescriptorLow;
Also quite hungarian :)

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
File src/shared/Utils.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/shared/Utils.cpp:28: bool IsWindows8()
Shouldn't this be "isWindows8OrLater"?

[Oleksandr, 2014-03-04 10:40:05]

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:52: else
yes. bing.com (or any other website from the exceptions list) or just not in
Enhanced Protected Mode.

On 2013/12/10 16:46:36, Felix H. Dahlke wrote:
> This happens if the engine is first started from http://bing.com, right?

[Felix Dahlke, 2014-06-24 15:30:38]

Ah, got this one back from the dead, sorry for ignoring it after you addressed
the comments!

Just some small stuff remaining here.

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
src/shared/Communication.cpp:70: // TODO: Would be better to detect if
AppContainers are supported instead of checking the Windows version
You can also slay the TODO comment here now!

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
src/shared/Communication.cpp:117: PACL sacl;
Hm, don't we leak sacl now that you removed sharedSacl?

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
File src/shared/Utils.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
src/shared/Utils.cpp:40: if (res == FALSE)
I'd prefer !res here, but may just be me.

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
src/shared/Utils.cpp:44: else
From the Mozilla coding style which ours is based on: "Don't put an else right
after a return. Delete the else, it's unnecessary and increases indentation
level."

[Eric, 2014-06-25 17:43:55]

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
src/shared/Communication.cpp:117: PACL sacl;
On 2014/06/24 15:30:39, Felix H. Dahlke wrote:
> Hm, don't we leak sacl now [...]

Evidently not. The security descriptor structure has internal structure not
exposed in the SECURITY_DESCRIPTOR declaration. My reading of the MS docs (which
are notoriously incomplete about allocation issues) is that the function
GetSecurityDescriptorSacl() does not allocate, but rather returns a pointer to
something that's already present in its first argument.

SECURITY_DESCRIPTOR structure
http://msdn.microsoft.com/en-us/library/windows/desktop/aa379561%28v=vs.85%29...

GetSecurityDescriptorSacl function
http://msdn.microsoft.com/en-us/library/windows/desktop/aa446653%28v=vs.85%29...

[Felix Dahlke, 2014-06-27 14:35:56]

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
File src/shared/Communication.cpp (right):

http://codereview.adblockplus.org/6308231068516352/diff/5998746462584832/src/...
src/shared/Communication.cpp:117: PACL sacl;
On 2014/06/25 17:43:55, Eric wrote:
> On 2014/06/24 15:30:39, Felix H. Dahlke wrote:
> > Hm, don't we leak sacl now [...]
> 
> Evidently not. The security descriptor structure has internal structure not
> exposed in the SECURITY_DESCRIPTOR declaration. My reading of the MS docs
(which
> are notoriously incomplete about allocation issues) is that the function
> GetSecurityDescriptorSacl() does not allocate, but rather returns a pointer to
> something that's already present in its first argument.
> 
> SECURITY_DESCRIPTOR structure
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa379561%2528v=vs.85%...
> 
> GetSecurityDescriptorSacl function
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa446653%2528v=vs.85%...

Yeah, I think you're right Eric. Seems like Oleksandr interpreted it the same
way too :)

[Oleksandr, 2014-07-02 09:08:02]

This codereview is already outdated. As a lot of the changes had to be reverted
and there's another review for these changes. Closing this.