louislam / uptime-kuma

A fancy self-hosted monitoring tool
https://uptime.kuma.pet
MIT License
55.87k stars 5.03k forks source link

Password hash being hashed for use as a JWT payload may lead to offline cracking #4919

Open Skelmis opened 2 months ago

Skelmis commented 2 months ago

DO NOT PROVIDE ANY DETAILS HERE. Please privately report to https://github.com/louislam/uptime-kuma/security/advisories/new.

Why need this issue? It is because GitHub Advisory do not send a notification to @louislam, it is a workaround to do so.

Your GitHub Advisory URL: https://github.com/louislam/uptime-kuma/security/advisories/GHSA-w2qm-fphj-p942

github-actions[bot] commented 1 week ago

We are clearing up our old help-issues and your issue has been open for 60 days with no activity. If no comment is made and the stale label is not removed, this issue will be closed in 7 days.

CommanderStorm commented 1 week ago

Details

While testing a local installation of Uptime Kuma, we observed that the JSON Web Token (JWT) used for authentication within the platform contained a SHAKE256 hash of the users BCRYPT hash within the JWT payload.

This can be seen in the file /server/model/user.js with the following code:

    /**
     * Create a new JWT for a user
     * @param {User} user The User to create a JsonWebToken for
     * @param {string} jwtSecret The key used to sign the JsonWebToken
     * @returns {string} the JsonWebToken as a string
     */
    static createJWT(user, jwtSecret) {
        return jwt.sign({
            username: user.username,
            h: shake256(user.password, SHAKE256_LENGTH),
        }, jwtSecret);
    }

Even though the user password is already hashed using BCRYPT and this creates a further hash using SHAKE256, this still represents a security flaw as it unnecessarily discloses inherently secret information.

In the event that a JWT is obtained, a malicious attacker may be able to use this field to crack the user password offline before using this password to pivot into the account on the website.

PoC

  1. Open the file server/model/user.js
  2. Navigate to the method createJWT
  3. Observe the signed JWT payload, and note the h: value which contains shake256(user.password, SHAKE256_LENGTH). This is the SHAKE256 of the users BCRYPT hash.

This can also be seen in the code block below, featuring the relevant piece of code.

    /**
     * Create a new JWT for a user
     * @param {User} user The User to create a JsonWebToken for
     * @param {string} jwtSecret The key used to sign the JsonWebToken
     * @returns {string} the JsonWebToken as a string
     */
    static createJWT(user, jwtSecret) {
        return jwt.sign({
            username: user.username,
            h: shake256(user.password, SHAKE256_LENGTH),
        }, jwtSecret);
    }

Impact

A malicious user may be able to use the JWT in order to reverse a users password for further exploitation. As passwords are often re-used between services, this may pose a higher risk to end users.

Likelihood

While testing Uptime Kuma, we did not find any ways to leak a users JWT, however, end users may still be phished into revealing the contents of the JWT used to authenticate to the website. Further to this, as the JWT is stored in local storage a malicious attacker may be able to exploit this and use an XSS vector (If discovered) to obtain an end user's credentials.

It is worth noting however, that as the underlying hashing mechanism is BCRYPT the likelihood of an attacker being able to disclosure a users password is very low.

Mitigations

It would appear that this hash value is used to check if a user's password has changed between login based on the hash value.

We would propose the following assuming these assumptions are correct:

This would remove an attacker's ability to retrieve password hashes while maintaining the existing use case.

CommanderStorm commented 1 week ago

Essentially, sha3 is secure. We use sha3.

=> We are using a currently quite uncrackable (as far as I read into this) hash, but are including that hash in the JWT. @Skelmis argues that a secret should be addend to the JWT instead.

I don't think this issue has enough impact to warrant immediate response (the reason it went stale).