MutonUfoAI / pgina

pGina fork: Open Source Windows Authentication
http://mutonufoai.github.io/pgina
BSD 3-Clause "New" or "Revised" License
156 stars 39 forks source link

(Security) HttpAuth plugin does not escape JSON, can change users' passwords from the login screen without valid credentials #134

Open micolous opened 5 years ago

micolous commented 5 years ago

The pGinaFork HttpAuth plugin has multiple bugs:

When using HttpAuth for authentication and changing passwords, these issues can be used to change an arbitrary user's password from the login screen without valid credentials.

As they're all related, I've put them in one GitHub issue.

Issue 1: Changing passwords sends the new password twice -- both as the new password and the old password.

During normal execution, the plugin first checks that the password is correct by attempting to authenticate on a Windows level first (which might invoke the HTTP authentication plugin again), then if successful, hands off to getPwChangeResponse:

https://github.com/MutonUfoAI/pgina/blob/1922de0fe27492c09d2f188c2c7e54a4b364bbad/Plugins/HttpAuth/HttpAuth/PluginImpl.cs#L101-L110

getPwChangeResponse accepts a username, the new password, and the old password as parameters:

https://github.com/MutonUfoAI/pgina/blob/1922de0fe27492c09d2f188c2c7e54a4b364bbad/Plugins/HttpAuth/HttpAuth/Accessor.cs#L96

However, the new password is sent twice:

https://github.com/MutonUfoAI/pgina/blob/1922de0fe27492c09d2f188c2c7e54a4b364bbad/Plugins/HttpAuth/HttpAuth/Accessor.cs#L106

As a result, a "compliant" implementation (which actually checks that the old password is correct before changing it) couldn't possibly work.

I wasn't able to get the "Change Password" feature of the HttpAuth plugin working, so I couldn't actually test this. But, I think that the code's behaviour is clear enough.

Issue 2: JSON data is not escaped at all in HttpAccessor:

https://github.com/MutonUfoAI/pgina/blob/1922de0fe27492c09d2f188c2c7e54a4b364bbad/Plugins/HttpAuth/HttpAuth/Accessor.cs#L31

https://github.com/MutonUfoAI/pgina/blob/1922de0fe27492c09d2f188c2c7e54a4b364bbad/Plugins/HttpAuth/HttpAuth/Accessor.cs#L106

I was able to confirm that the Authentication handler worked this way, but I never got any requests sent for the Change Password handler. I'm unsure of why, but pGina always said my password changes were "successful". 🤷‍♂️

Issue 3: Authentication and password change API calls both use the same URL, and are not otherwise distinctive aside from the inclusion of the old field for password change calls (and exclusion for authentication calls). As a result, any flaw in HttpAuth allows sending arbitrary API calls to the endpoint.

I wasn't able to get the "Change Password" feature of the HttpAuth plugin working, so I couldn't actually test this. But, I think that the code's behaviour is clear enough.

Scenarios as a result of these issues:

  1. Because of issue 1, an installation doing authentication and password changes with the HTTP plugin (a likely setup), changing passwords is vulnerable to a race condition between checking the password and actually changing the password -- so it is not possible to build a "correct" implementation.

    If the system operators were aware of this issue, they could attempt to work-around the issue by requiring a recent successful authentication. But this is not easy or reliable:

    • there is no session token passed between requests
    • IP-based restrictions would only be effective if clients have distinct IP addresses seen by the HTTP server, and endpoints have no other code running which could make a HTTP POST request.

      That could be defeated, if an attacker discovered the HTTP authentication URL were discovered (eg: the plugin is configured with the default https://pginaloginserver/login), they could entice authorised users of the system to visit a web page that makes HTTP POST requests in the background with JavaScript.

  2. Because of issue 1, if an administrator is doing password changes with the HTTP plugin, but authentication with a different plugin, it is possible to change users' passwords without authentication.

    Secondary mechanisms can be defeated (per above), and there is no guarantee that they are actually implemented.

  3. Because of issue 2, it is not possible for a user to have any JSON-reserved characters in their username or password, such as ".

    When I try to authenticate with the username of a" and the password of a", I get this request sent to the authentication endpoint:

    {"username":"a"","password":"a""}

    This is invalid JSON, so a user with these characters in their password at best will not be able to log in.

  4. Because of issues 1, 2 and 3 combined, an installation doing authentication and password changes with the HTTP plugin (a likely setup), it is possible to change someone's password without authentication, from the login screen.

    If I wanted to change a user's password to newpassword, entering the password newpassword","old":"newpassword would result in the following request sent to the endpoint:

    {"username":"user","password":"newpassword","old":"newpassword"}

    This would result in the user's password being changed to newpassword. The request would come from an "authorised" IP address and endpoint.

Mitigation strategies

Issues 1 and 3 (and thus scenarios 1, 2 and 4) can be mitigated by:

It is not possible to mitigate Issue 2. There may be additional issues as a result of what receives this request, but this is implementation-dependent.

Environment

cc: @vencax (per #17 #19)