search

pseymour / MakeMeAdmin

Make Me Admin is a simple, open-source application for Windows that allows standard user accounts to be elevated to administrator-level, on a temporary basis.
https://makemeadmin.com/
GNU General Public License v3.0
391 stars 85 forks source link

Security Questions #25

Closed jojastahl closed 3 years ago

jojastahl commented 3 years ago

Hi,

I came accross this when looking for a solution to the local admin problem. I was curios about how this project is secure, so that somebody cannot get admin rights who is not allowed to.

I quickly looked at the codebase and have these questions:

  1. IAdminGroup has a bool UserIsAuthorized(string[] allowedSidsList, string[] deniedSidsList) method, isn't it insecure if the caller specifies the conditions which should be checked?
  2. What is the EncryptedSettings for? It is protected by ProtectedData-API using Machine scope. As far as I understand the documentation, any account locally on the machine can encrypt and decrypt that data. So the encryption is useless?
  3. I was thinking about adding MFA for the admin right request. Is something like this planned for the future?

Please do not take my questions as criticism. Seems to be a simple way to grant temporary local admin rights. Was just looking if the systems could be compromised by an attacker.

pseymour commented 3 years ago
  1. You could pass in different arrays and trick UserIsAuthorized() into saying that the user is allowed to gain admin rights. However, checking to see if the user is authorized does not do anything dangerous. The dangerous function, AddUserToAdministratorsGroup(), does not accept any arguments. It uses the identity of the user who called it and the authorization lists from the application settings. You do not get a chance to pass in an alternate set of allow/deny lists. This function performs an additional check of the user's authority to gain admin rights, using the application settings. So, even if you trick the add user function into being called, you can't trick it into performing an authorization check with a different list of identities.
  2. The encrypted settings is just the list of users that the service has added to the admins group, along with some expiration data. It is really just encryped to prevent casual browsing of the list. Indeed, if someone uses Make Me Admin to gain admin rights, they could just delete this file; that's a risk you run in trusting people with admin rights. I am considering some changes in how this file is used, by the way, in no small part because it can also become corrupted, particularly if the machine blue screens.
  3. MFA has definitely been requested, particularly with Duo, and especially in the academic world. I just haven't gotten to it yet. (As a side note, Duo does have a way to force MFA whenever UAC kicks in. https://guide.duo.com/rdp#winlogon-uac)

And I do take questions as criticism, but I think criticism gets a bad reputation, unfairly. Criticism can also be a positive thing. This application does a dangerous thing, which then allows users to do a wide array of dangerous things. The main reason for making this project open-source was to invite examination and criticism, in order to improve. Making the software better benefits everyone, including myself.

So please continue to examine, question and critique.

ayotec2015 commented 1 year ago

Hey, This application is really nice and simple. Just wondering if there is any update on the MFA, would be perfect.

pseymour commented 1 year ago

An update to what MFA?

nebunamirax commented 8 months ago

An update to what MFA?

Hi, I would say Microsoft Authenticator ?