102 - Desktop access rights modified to comply with paper

[bencikpeter]

Access rights are now identical to those described in paper (table 3)

@DonatJR could you have a look at StartedInCage() function in CageChooser? It will not work with new access rights and C# is not my friend... so I guess I need your help 😅 And maybe we could fix #96 at the same time (since this might not be the last rewrite, if rights ever change)

[DonatJR]

@bencikpeter @SailReal

Alright, some points to discuss / think about:

• in order to being able to test the new StartedInCage method I had to think about how to solve #104, turns out displaying the UAC control is not the issue. The CreateProcess... family does not even try to display an elevation prompt but instead fails with ERROR_ELEVATION_REQUIRED.
  • Solution 1: Use ShellExecute(Ex) as it handles automatic elevation but I think cannot use a token to start a process
  • Solution 2: Check for this error and display our own elevation prompt, this is currently rudimentarily implemented (thoughts?)
• the access rights check itself works, but sometimes getting the sid in {EXPLICIT_ACCESS-struct}.Trustee.ptstrName fails (nullptr, first occurence, second occurence). This seems to happen fairly randomly and I have currently no idea why this is happening (maybe one of you has an idea)
  • first I thought it might have something to do with the system process used to create our token as this process could be different each time, but I verified the same process was used with the problem happening and with it working
  • I also checked the Trustee struct on the C++ side after creating the new desktop with them, and here all values seem to be okay all the time

[SailReal]

If the {EXPLICIT_ACCESS-struct}.Trustee.ptstrName isn't null the current solution works like a charm 😍. But as you discussed @DonatJR, randomly it is null but in my opinion, it is the right way to go. Will now have a look into the code in more detail, maybe I'll find out what's causing it.

Later we should enhance the dialog with more content that the user knows, it is more or less the same dialog like the UAC prompt with the same impact.

[SailReal]

Hmm, thought maybe one of these functions already fails, unfortunately not 🤨

[DonatJR]

Found the problem and will push a fix later today

[DonatJR]

Alright, this should fix the previous issue. The problem was that the ACL parameter retrieved by GetSecurityInfo apparently shares some data with the security_descriptor (last param of GetSecurityInfo). When freeing the security_descriptor (was previously done right after the call to GetSecurityInfo) param the ACL param got corrupted / invalid.

Should have read the documentation more carefully:

If the ppsidOwner, ppsidGroup, ppDacl, and ppSacl parameters are non-NULL, and the SecurityInfo parameter specifies that they be retrieved from the object, those parameters will point to the corresponding parameters in the security descriptor returned in ppSecurityDescriptor.

🙈

After testing the release-mode check should be put back in.

[SailReal]

I have started the configurator a hundred times, with success and the explanation sounds reasonable. Thanks for the awesome fix 😍!!!

Will now look over the code again...

[SailReal]

@DonatJR should I put the release-mode check back in?

[DonatJR]

@bencikpeter the PR should now be ready to merge, I removed the WIP from the title. feel free to proceed ;)