Ensure workload installers can be read by other users#28608
Ensure workload installers can be read by other users#28608joeloff merged 3 commits intodotnet:mainfrom
Conversation
|
@eerhardt would you mind taking a peek at this change? |
There was a problem hiding this comment.
I don't know enough to give this a proper review, but a few comments:
-
The file is moved before the permissions are set. This should be fine if it has lower or the same permissions via move, but is there slight potential for a Race condition where the permissions were not set but the file had been moved? If its the later maybe its so late we dont care.
-
O:BAG:DUD:AI(A;ID;FA;;;SY)(A;ID;FA;;;BA)(A;ID;0x1200a9;;;BU)(A;ID;0x1200a9;;;WD)is really difficult for me to digest, could there instead be something like
const string ADMIN_READ_PERMISSION = 'O:BAG:DUD:AI
const string ADMIN_WRITE_PERMISSION = '(A;ID;FA;;;SY)`
...
const string CORRECT_PERMISSIONS = ADMIN_READ_PERMISSION + ADMIN_WRITE_PERMISSION + ...;
SetSecurityDescriptorSddlForm(CORRECT_PERMISSIONS)?
Probably a pain but it would be super helpful imo.
|
|
Thanks @joeloff for following up, that resolves my concerns and it's much easier to understand now. Would appreciate if someone could give a final approval. |
|
@eerhardt Any other concerns here? |
For MSI based installs of workloads, the MSI and its manifest are extracted to a random temporary folder and then moved into the package cache under Program Data. Checking the cache for the installer happens on the client (non-elevated) side. When one user installed a workload and another user logs on and tries to update installed workloads, the check fails because the second user has no read permissions against the file in the cache.
Because the file is extracted by the client and then copied by the server, the first user permissions are attached to the file.
For example, when the file is moved, we end up with descriptor that includes the original user's SID:
O:S-1-5-21-1004336348-1177238915-682003330-512G:DUD:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;S-1-5-21-1004336348-1177238915-682003330-512)We want the owner to be the built-in administrators and restrict built-in users and everyone to only have read and execute permissions:
O:BAG:DUD:AI(A;;0x1200a9;;;WD)(A;ID;FA;;;SY)(A;ID;FA;;;BA)(A;ID;0x1200a9;;;BU)Fixes #28450