[Security Risk] Dangerous flow when JWT is incorrect or missing

[lukasz-wronski]

Problem description

If JWT token in an authentication link is missing, is incorrect or its signature is incorrect then false is passed to the verify method. This flow seems not to be expected as even the library documentation on the passport page here https://www.passportjs.org/packages/passport-magic-login/ does not seem to handle this situation properly assuming payload.destination always exists.

Possible security implication

When false is passed then payload.destination is equal to undefined. If this is not handled and payload.destination is then passed to an ORM or ODM while looking for the user in a database this can lead to the situation in which undefined is passed directly to the where clause of the database query. In this case most ORMs like TypeORM will assume that the query should not have a where clause at all and will return first user from the database. This leads to login without access to user's email and bypassing whole application security.

Proof of concept

When application is using following verify method

verify: (payload, callback) => {
    const user = userRepository.findOne({
        where: { email: payload.destination },
    });

    if(user)
        callback(null, user)
    else 
        callback("user not found")
}

and the callback parameter is empty or contains malformed JWT token, the application will login user as a first user in the database.

Proposed mitigation

1. Throw an error and break the application flow if JWT token provided with the link is incorrect or empty
2. Mark in the documentation that payload parameter in the verify method should be checked if equal to false

[mxstbr]

Thank you for the careful read of the source; I agree that this isn't an optimal implication of the API design. (even though I wish that JS-based ORMs handled this properly 😬)

Do you think something like if (!payload.destination) throw new Error('Please provide a destination') would solve this potential hazard?

[lukasz-wronski]

Hey @mxstbr, thanks for quick reaction. In fact this is discovery was made with a real system I was pentesting. Developers were confident the links are verified correctly and I've managed to login as a random user just by messing the token up.

Regarding the code change. In my opinion it would be best to catch it as soon as possible adding this exception right inside the decodeToken function. In example like this:

export const decodeToken = (secret: string, token?: string) => {
  try {
    return jwt.verify(token, secret) as JwtPayload;
  } catch (err) {
    throw new Error('JWT incorrect or missing');
  }
};

Please note that if (typeof token !== 'string') you have now is redundant as jsonwebtoken library already checks this in the line below:

https://github.com/auth0/node-jsonwebtoken/blob/74d5719bd03993fcf71e3b176621f133eb6138c0/verify.js#L56

I think it's safe to just put anything into jwt.verify and rely on @auth0 to have all these checks implemented correctly.

If you want to make it even simplier you can make decodeToken to directly call jwt.verify and do no try..catch wrapping. Then your users will get more detailed error about the problem with the JWT provided. Your choice, both options seem to be right.

Let me know if I can assist you further.

[mxstbr]

Published the second idea in v1.0.10. I appreciate the detailed writeup and suggestions @lukasz-wronski!

[lukasz-wronski]

@mxstbr One thing that I can see you've left the if (typeof token !== 'string') and it's still working the same way as before in case I'm not adding ?token in query string. Undefined is not a string so it returns false. It still needs to be fixed.

[mxstbr]

Thanks for double-checking my work, I had missed that nuance. Fixed in v1.0.11!