SharkCagey / SharkCage

1 stars 3 forks source link

ACL Setup #61

Closed DonatJR closed 6 years ago

DonatJR commented 6 years ago

PR for #13 After the changes in #21 were not possible this should be done with the earlier changes to the branch.

Only open issue is denying the administrator group access to the new desktop as processes can then no longer be started:

SecuritySetup.cpp:137:
explicit_access_admin.grfAccessMode = SET_ACCESS; // FIXME set to DENY_ACCESS -> currently process creation on new desktop is not possible anymore after doing this

I guess this has to do with the token manipulation (@bencikpeter), do you want to work directly on this PR or do you prefer us merging this before you start?

bencikpeter commented 6 years ago

I´d say merge it, there are multiple more issues I see in SecuritySetup and related functionality and it will require work out of scope of this PR

This specific thing is IMO not related directly to token manipulation.... I think you are just locking yourself out. If you think about it, denying access to administrator group will also deny access to you. What I see as a potential solution is:

This however might require a rewrite of TokenLibrary since it uses the token of calling process as a template... and I doubt it is possible to start a process such as CageManager with a service token.

Other way to do so, is for a CageManager to sample it´s own token, add a desired group in it and restart itself with a new token. This should accomplish the same result without shuffling functionality around, but CageManager must be able to detect it´s state (if already a member of temp group or not), restart itself and receonnect to the messaging channel with the CageService

I vote for a second solution... opinions?

DonatJR commented 6 years ago

It is not however related directly to token manipulation.... I think you are just locking yourself out. If you think about it, denying access to administrator group will also deny access to you.

Well yes, of course that is the problem. :p We just figured the plan was (from the original code) to start processes from CageManager in the context of a new group with the manipulated token and restrict the desktop to this new group. Snippet from the original code:

// EXPLICIT_ACCESS for created group
ea[0].grfAccessPermissions = GENERIC_ALL;
ea[0].grfAccessMode = SET_ACCESS;
ea[0].grfInheritance = NO_INHERITANCE;
ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
ea[0].Trustee.TrusteeType = TRUSTEE_IS_GROUP;
ea[0].Trustee.ptstrName = (LPTSTR)groupSid;
// EXPLICIT_ACCESS with second ACE for admin group
ea[1].grfAccessPermissions = GENERIC_ALL;
ea[1].grfAccessMode = SET_ACCESS; //DENY_ACCES
ea[1].grfInheritance = NO_INHERITANCE;
ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID;
ea[1].Trustee.TrusteeType = TRUSTEE_IS_GROUP;
ea[1].Trustee.ptstrName = (LPTSTR)sid_admin;

[...]

//PETER´S ACCESS TOKEN THINGS

//Create the process.
CreateProcess(NULL, &vec[0], NULL, NULL, TRUE, 0, NULL, NULL, &info, &processInfo);

create desktop and restrict access to the desktop to only members of the temporary group (this should work around your previous problem of not being able to create desktops from service)

The problem with creating desktops on the service is not the groups having access to it but rather the window station it belongs to. Creating it from the service means the desktop belongs to an invisible window station and SwitchDesktop fails for such desktops.

[...] and I doubt it is possible to start a process such as CageManager with a service token.

The duplicated token of the service is already being used to create the CageManager process, see here. I'm not sure why this should stop working with your proposed changes?

Other way to do so, is for a CageManager to sample it´s own token, add a desired group in it and restart itself with a new token. This should accomplish the same result without shuffling functionality around, but CageManager must be able to detect it´s state (if already a member of temp group or not), restart itself and receonnect to the messaging channel with the CageService

Are we absolutely sure it is necessary for the CageManager to have a token with the new group (see first comment, we thought having the new processes have this token should suffice)? If yes, I have to think about your proposed solution some more, but the first gut feeling I had while reading was that this introduces a lot of potential new errors and maybe even security implications... :/

bencikpeter commented 6 years ago

The problem with creating desktops on the service is not the groups having access to it but rather the window station it belongs to Creating it from the service means the desktop belongs to an invisible window station and SwitchDesktopfails for such desktops.

What I am trying to say with that is: You may shift the token modification to service, but have to create desktop from manager... and if you start manager with modified token, you dont need modification (and thus modification privileges) in manager anymore and can just duplicate token.

The duplicated token of the service is already being used to create the CageManagerprocess, see here. I'm not sure why this should stop working with your proposed changes?

Sorry, my bad ... you are right of course 😇

Are we absolutely sure it is necessary for the CageManager to have a token with the new group (see first comment, we thought having the new processes have this token should suffice)? If yes, I have to think about your proposed solution some more, but the first gut feeling I had while reading was that this introduces a lot of potential new errors and maybe even security implications... :/

(Side note: does it even bear any benefits to protect desktop against administrators group? If malware woud have administrative rights, it could just enumerate all groups on the system and join them. I see the shark cage as a defence over user-space malware... once malware is kernel-space, isn´t that game over?)

DonatJR commented 6 years ago
  • Isn´t CageManager a part of TCB? If so, why would it create any security implications if it was a part of the group? (If it is not TCB, than security implications are already there... it can modify tokens, therefore it can add itself (or anyone else) to the group at any time)

I was rather thinking of switching the binary in between the first and second execution, but as mentioned it was just a quick thought that popped into my head. Not really worth discussing any further ;)

  • What CageManager definitely needs, is to have an access right to start a process on the desktop and the child process need an access right to run on that desktop. If CageManager is completely locked out of said desktop I believe it means it cannot start processes on it, regardless the access rights of those processes.

You are absolutetly right, I don't know why I didn't consider this (was probably convinced by the existing code that it had to be DENY_ALL). Sorry... 😅 I will change it so the administrator group can execute processes on the desktop :)

(Side note: does it even bear any benefits to protect desktop against administrators group? If malware woud have administrative rights, it could just enumerate all groups on the system and join them. I see the shark cage as a defence over user-space malware... once malware is kernel-space, isn´t that game over?)

You are right, if the administrator group is compromised we no longer have any guarantees / everything could be compromised.

bencikpeter commented 6 years ago

I was rather thinking of switching the binary in between the first and second execution, but as mentioned it was just a quick thought that popped into my head. Not really worth discussing any further ;)

About that.... computing a hash or signing and verifing the binary before launching might help... but it seems kinda overkill (?)

DonatJR commented 6 years ago

About that.... computing a hash or signing and verifing the binary before launching might help... but it seems kinda overkill (?)

We need to include functionality for checking signatures / hashes anyway for the processes not belonging to the project ;)

SailReal commented 6 years ago

This branch could still be deleted or?