jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.48k stars 4.02k forks source link

Security: /authenticate endpoint returns information about users in the database #21731

Closed Shmarkus closed 1 year ago

Shmarkus commented 1 year ago
Overview of the feature request

Obfuscate the login response for accounts that do not exist in the database (Jhipster 7.9.3)

Motivation for or Use Case

Currently, when logging in with non-existing user, the API response is:

{
  "type" : "https://www.jhipster.tech/problem/problem-with-message",
  "title" : "Unauthorized",
  "status" : 401,
  "detail" : "User thisisanonexistinguser was not found in the database",
  "path" : "/api/authenticate",
  "message" : "error.http.401"
}

This creates an attack vector for the malicious user who can use this to enumerate users stored in the database.

Proposed solution

The solution would be to respond with a generic "invalid username or password" message.

Related issues or PR

Didn't find similar issues

atomfrede commented 1 year ago

Indeed this creates an attack vector. Does the message change when using an existing user with invalid password?

Shmarkus commented 1 year ago

Yes, when using the correct user the response is:

{
  "type":"https://www.jhipster.tech/problem/problem-with-message",
  "title":"Unauthorized",
  "status":401,
  "detail":"Invalid Credentials",
  "path":"/api/authenticate",
  "message":"error.http.401"
}
atomfrede commented 1 year ago

That's bad (I think it used to be different). Are you using the dev profile? Maybe we have excluded the detail only in production profile.

EDIT: Sorry I think I mixed up details and stacktraces inside problems

mraible commented 1 year ago

I added a bug bounty to get this one fixed.

atomfrede commented 1 year ago

Will work on it. First checking if we have something similar in main/sb3, afterwards fixing the 7.x issue. Guess a bugfix release for 7.x would be good

atomfrede commented 1 year ago

@mshima I remember something has changed with regards to local development setup, but can't find the issue/PR. Is our contribution documentation up2date? I am doing npm link and npm link generator-jhipster as usual. Afterwards I get an error running jhipster (maybe it has nothing to do with jhipster, but node. Tried node 16 and 18). Tried both jhipster and npx jhipster

WARNING! Since JHipster v8, the jhipster command will not use the locally installed generator-jhipster.
    If you want to execute the locally installed generator-jhipster, run: npx jhipster
ERROR! ERROR! Named export 'isReservedTableName' not found. The requested module '../../../jdl/jhipster/reserved-keywords.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '../../../jdl/jhipster/reserved-keywords.js';
const { isReservedTableName } = pkg;

file:///Users/frederik.hahne/git/github/jhipster/generator-jhipster/dist/generators/server/support/prepare-entity.mjs:21
import { isReservedTableName } from '../../../jdl/jhipster/reserved-keywords.js';
         ^^^^^^^^^^^^^^^^^^^
SyntaxError: Named export 'isReservedTableName' not found. The requested module '../../../jdl/jhipster/reserved-keywords.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '../../../jdl/jhipster/reserved-keywords.js';
const { isReservedTableName } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)
atomfrede commented 1 year ago

For reference, I checked the 7.9.3 sample app and get following as return value in all cases (not existing user, wrong password). @Shmarkus Are you using oauth?

{
  "type" : "https://www.jhipster.tech/problem/problem-with-message",
  "title" : "Unauthorized",
  "status" : 401,
  "detail" : "Bad credentials",
  "path" : "/api/authenticate",
  "message" : "error.http.401"
}
Shmarkus commented 1 year ago

I'm using local database for users. (I'll double check the error message with another app just to be sure)

atomfrede commented 1 year ago

Did you generate a fresh application with the latest main branch? From just checking the code it seems the problem might only be related to the 8.x development version. oauth was just a quick idea, but that should not be the case as the whole checking is delegated to the oauth provider

mshima commented 1 year ago

@atomfrede npm ci to recreate node_modules. An I create a bash alias from jhipster to the generator-jhipster/bin/jhipster.mjs (absolute path) to avoid recompiling every time.

atomfrede commented 1 year ago

Any recommendation regarding node and npm version? Already deleted node_modules. Will try again later.

mshima commented 1 year ago

Any recommendation regarding node and npm version? Already deleted node_modules. Will try again later.

Node 16 or 18.13.0 see https://github.com/jhipster/generator-jhipster/issues/21380 We had problems on testing upgrade generator at npm 9, but was fixed yesterday and today’s patch fixed upgrade test using node 18.13.0.

atomfrede commented 1 year ago

I think I have tried both. Will keep you updated after the os update.

atomfrede commented 1 year ago

I can reproduce it now. It is only the case when using reactive, so I guess maybe for jh 7.x too. I also found another issue with not activated users which also exposed that such a user exists, which should not be the case I would say.

atomfrede commented 1 year ago

I will provide a fix later. When using non reactive there exists a default exception handler for authentication exceptions, which seems not to be the case for reactive. We have everything in place to handle these exceptions in a common way.

Shmarkus commented 1 year ago

Yes, I have the reactive gateway (thought it's the only option, but it seems you can generate gateway using the "old way" as well)

atomfrede commented 1 year ago

I have also found an issue with the user registration. Instead of "accepting" every request we differentiate between username already used and email already used. With that information an attacker could gather information about existing users. I would prefer to have just "accept" any request saying please check your email, such that all cases look the same from the outside. What you say @mshima @mraible ?

mraible commented 1 year ago

@atomfrede I agree that we shouldn't give the end user any additional information before they authenticate successfully.

BrayanMnz commented 1 year ago

Hi @atomfrede are you working on this or are you happy to hand it over? btw, could you provide the steps that you did to replicate it as you mentioned here https://github.com/jhipster/generator-jhipster/issues/21731#issuecomment-1500430600

atomfrede commented 1 year ago

Still working on it. Hope to get back to it this week(end).

Shmarkus commented 1 year ago

any updates on this issue?

atomfrede commented 1 year ago

Need to prepare the PR

Shmarkus commented 1 year ago

just checking whether things are going as planned?

atomfrede commented 1 year ago

Will do it now :D I have some time, children are sleeping already

networkinss commented 1 year ago

This is not reflected in the frontend. If it gets 200 or 201, it does not recognize an error. Even in the processError() function in the register.component.ts it checks for the status 400, but gets a 201 if a user exists already. Result is a success message for the user (despite the error), who wonders why he/she does not get a confirmation mail and cannot login.

atomfrede commented 1 year ago

Thats on purpose. An attacker should not be able to gain information about existing users. So trying to create a user with success or with error behave the same when looking at API responses.

networkinss commented 1 year ago

I checked the response from the backend. If an error occurs, it will show up in the response body with an error message. A hacker will see the error message in the response body. A user will not. As a result the user is confused and thinks the application does not work properly (which is actually right). And the hacker is not confused.

networkinss commented 1 year ago

Such a security measure may be appropriate in an environment where the user name is unique, such as an internal application in a company. Makes absolutely sense. But in an application which can be accessed from anywhere, it can be a desaster. Imagine a user takes his forename: Thomas as username. The first can register. A thousand other user with name Thomas cannot. They all get a fake message and they cannot register because they have no idea what is wrong. Image they post that in Reddit. It would be the death of the webservice.

So from my point of view, this could be only optional (opt-in configuration). If it is hardcoded, it can kill an applications reputation.

atomfrede commented 1 year ago

If there is still an error message in the response that's wrong and not intended. Which configuration are you using?

networkinss commented 1 year ago

The error message in the response is correct. It says that the user exists already. It just is not processed by the frontend, because the statuscode shows 201 instead of 400.

atomfrede commented 1 year ago

Well that's not what its supposed to do. There should no message of course

networkinss commented 1 year ago

This is simply not a valid security measure at registration of a new user. Absolutley no webservice is hiding usernames during registration. It is not possible.

You can do that only at login. Here a standard message like "username or password is wrong" makes sense.

But not at registering.

atomfrede commented 1 year ago

Let me check. For login we have changed it definitely maybe I remember it wrong for registration as the error message is still there.

vishal423 commented 11 months ago

I just realized this fix is breaking tests in the Svelte bluepirnt during upgrade to jhipster8 activity. I don't agree to hide error code when the username already exists in the database. In absence of an error, how do we expect end users to know that their registration was not successful? If the expectation is that they should get email, then, in absence of that they would never know what's wrong in the registration process (could be network issues). If we look at public sites that allow user registration, then, all of those do return appropriate error message in such scenario.

image

image

image

networkinss commented 11 months ago

Absolutely agree.

atomfrede commented 11 months ago

Sorry for the late reply. But yes obviously for registration it is too much. I will change it.

On Sun, Oct 22, 2023, 06:21 Oliver Glas @.***> wrote:

Absolutely agree.

— Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/21731#issuecomment-1773989575, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRVCKW7LCBBD6WHRUPBZ3YASNNNAVCNFSM6AAAAAAWVCAEFKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZTHE4DSNJXGU . You are receiving this because you were mentioned.Message ID: @.***>