Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Quality: Disable drag & drop when running as admin #15795

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Jul 13, 2024

Summary

The service 'IWindowsSecurityService' gets the current process's token and checks if the token belongs to Administrators group.

  • Create IWindowsSecurityService
  • Replace ElevationHelpers with the service above
  • Cache the result in WindowContext

Resolved / Related Issues

Steps used to test these changes

  1. Open Files app as admin
  2. Confirm drag and drop is disabled in TabBar and content views.
  3. Check it also when UAC is disabled.
Open details that I learnt

Why drag and drop doesn’t work?

This is because of UIPI (User Interface Privilege Isolation) using MIC (Mandatory Integrity Control), which blocks a process that has lower IL (integrity level) from interacting with a process that has higher IL. This was introduced in Vista with UAC introduction.

It is not caused by UIPI, btw, that UWP cannot access system resource and user data without user’s consent (it’s by AppContainer itself).

IL get higher from top to bottom

  1. Untrusted (Chrome, IE)
  2. AppContainer/LowBox (UWP)
  3. Low
  4. Medium (default)
  5. Medium Plus
  6. High/Elevated (elevated)
  7. System (system services)
  8. Protected Process
  9. Installer (Windows)

UAC settings

Behavior

Level Modification
by a process
Modification
by user
On what session Remarks
Highest Yes Yes Secure Desktop (dimmed) Default in Vista
2nd Yes No Secure Desktop (dimmed) Default in 7 onwards
3rd Yes No Normal Desktop (not dimmed) Not recommended
Lowest No No N/A Not recommended
  • By process means: Installation of a software manifest of which calls for elevation, and invocation via RunAs verb
  • By user means: modification via Windows Settings app and certain Control Panel applets

Registry values to be modified

Level ConsentPrompt
BehaviorAdmin
ConsentPrompt
BehaviorUser
EnableLUA PromptOn
SecureDesktop
Highest 2 (AAC UAC) 3 (OTS UAC) 1 1
2nd 5 3 1 1
3rd 5 3 1 0
Lowest 0 3 0 0
  • AAC (Admin Approval Consent) UAC displays only Yes or No since logon user is in Administrators group.
  • OTS (Over The Shoulder) UAC displays text boxes to enter credential of an admin identity.

Disabling UAC

When you disabled UAC through UAC Settings dialog (UIPI will be disabled as well), the UAC prompt won’t be shown.

However, it seems that DataExchangeHost.exe doesn’t accept dropping onto a window of a process that has higher IL (Windows OS Bug?) even though UAC is disabled. A contributor of Windows Terminal made a workaround for it.
Also, I should denote that we might be able to use DragQueryFile to workaround UIPI, while I’m still not sure.

How UAC works (if it helps)?

On startup, the kernel executes explorer.exe with a token below:

Standard User (not in Administrators group)

  • Only restricted token will be generated after logon.
  • ShellExecute calls appinfo.exe (AIS) and then it calls consent.exe (UAC prompt) via its hosted inner service. When a valid credential of an administrator entered, AIS calls CreateProcessAsUser with that administrative identity; otherwise, it returns ERROR_ACCESS_DENIED.
  • The token generated by this routine is called filtered token (not privileged token)

Administrator User (in Administrators group)

  • Both privileged and restricted token will be generated.
  • When Yes clicked, AIS calls CreateProcessAsUser with the logon user ‘s privileged token; otherwise, it returns ERROR_ACCESS_DENIED.

BUILTIN/Administrator (the real administrator)

  • Only privileged token will be generated
  • AIS doesn’t call consent.exe. But this behavior can be altered from group policy.

I've been described so far, for when user hasn't changed anything through Windows Registry or Group Policy. Some behavior can be changed otherwise.

@0x5bfa 0x5bfa changed the title Code Quality: Fixed where checking if running as admin called a wrong API Code Quality: Fixed an issue where checking if running as admin called a wrong API Jul 13, 2024
@yaira2
Copy link
Member

yaira2 commented Jul 14, 2024

Can you cache the result?

@yaira2
Copy link
Member

yaira2 commented Jul 14, 2024

For #13394

@0x5bfa
Copy link
Member Author

0x5bfa commented Jul 14, 2024

Yeah do you think would it be better in WindowContext?

@yaira2
Copy link
Member

yaira2 commented Jul 14, 2024

I would keep the code in the service, but we can cache it in the context.

@0x5bfa

This comment was marked as resolved.

@yaira2
Copy link
Member

yaira2 commented Jul 15, 2024

I would prefer to do the same as Terminal.

@yaira2
Copy link
Member

yaira2 commented Jul 15, 2024

FYI @ahmed605

@ahmed605
Copy link
Contributor

IL get higher from top to bottom

this order is kinda wrong, AC has higher IL than Untrusted, and it's actually Low IL but with lower TrustLevel than usual Low IL apps, and there's also LPAC (Less Privileged AppContainer) which is almost on the same level as Untrusted but a little bit higher, it's used by Chromium, Firefox, and Microsoft Edge Legacy

@0x5bfa
Copy link
Member Author

0x5bfa commented Jul 16, 2024

I was really uncertain in that point. I was even not sure this is IL. But a docs i referred (not official) wrote as AppContainer < Untrusted. Thank you for the correction.

What about that function above to workaround this blocking?

@ahmed605
Copy link
Contributor

ahmed605 commented Jul 16, 2024

What about that function above to workaround this blocking?

I'm a bit worried about the security concerns, you can still inject through only the HWNDs btw

@0x5bfa
Copy link
Member Author

0x5bfa commented Jul 17, 2024

I see, thank you for letting me know it.
I'll just check token type and disable drag and drop accordingly.

@yaira2 yaira2 force-pushed the main branch 2 times, most recently from ff65c93 to 8097ba8 Compare July 17, 2024 15:22
@0x5bfa 0x5bfa changed the title Code Quality: Fixed an issue where checking if running as admin called a wrong API Code Quality: Disable drag & drop when running as admin or UAC is disabled Jul 20, 2024
@yaira2
Copy link
Member

yaira2 commented Jul 21, 2024

Is this ready for review?

@0x5bfa 0x5bfa marked this pull request as ready for review July 21, 2024 07:24
@yaira2
Copy link
Member

yaira2 commented Jul 21, 2024

Do we need to adjust the text in the "running as admin" prompt?

@0x5bfa 0x5bfa changed the title Code Quality: Disable drag & drop when running as admin or UAC is disabled Code Quality: Disable drag & drop when running as admin Jul 21, 2024
@0x5bfa 0x5bfa force-pushed the 5bfa/Fix-AdminCheck branch 3 times, most recently from 3c9fb67 to 8894dff Compare July 22, 2024 14:18
@0x5bfa
Copy link
Member Author

0x5bfa commented Jul 22, 2024

All good.

@yaira2
Copy link
Member

yaira2 commented Jul 22, 2024

@0x5bfa I rebased the branch from main, can you confirm that everything is in order?

@0x5bfa
Copy link
Member Author

0x5bfa commented Jul 22, 2024

Yes, except that test failing

Fixed by squashing all

@Josh65-2201 Josh65-2201 mentioned this pull request Jul 23, 2024
@yaira2 yaira2 merged commit ff7528c into files-community:main Jul 24, 2024
6 checks passed
@0x5bfa 0x5bfa deleted the 5bfa/Fix-AdminCheck branch July 24, 2024 05:00
@0x5bfa 0x5bfa added the ready to merge Pull requests that are approved and ready to merge label Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: App crashes when dragging a file while app is running with a high integrity level
3 participants