-
Notifications
You must be signed in to change notification settings - Fork 491
[CB-3681] Use something better than MD5 hash to pass local user passwords #3776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
[CB-3681] Use something better than MD5 hash to pass local user passwords #3776
Conversation
…hash-to-pass-local-user-passwords
…hash-to-pass-local-user-passwords
…hash-to-pass-local-user-passwords
…r-than-md5-hash-to-pass-local-user-passwords' into 4569-cb-3681-use-something-better-than-md5-hash-to-pass-local-user-passwords
…hash-to-pass-local-user-passwords
| return hash.toUpperCase(); | ||
| } | ||
|
|
||
| return md5(value).toUpperCase(); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
an access to oldPassword
Password from
an access to newPassword
Password from
an access to adminPassword
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, to fix the use of an insufficiently strong hash (like SHA-256 or MD5) for password hashing, we must replace them with a secure, computationally expensive, and salted password-hashing algorithm. In the JavaScript/TypeScript ecosystem, the most common and well-supported options are bcrypt, scrypt, or PBKDF2—bcrypt is especially simple and robust.
For this project, the change should be limited to only the relevant password-flows, so existing generic hashing functionality (e.g., for checksums, not password hashing) is preserved. You should create a new utility function—such as hashPassword—that uses bcrypt, and replace the problematic use of createHash with a call to the new function where passwords are being hashed (i.e., in UserInfoResource.ts, method updateLocalPassword). Update imports accordingly. Since bcrypt is an external dependency, add the import at the top of the relevant file. You do not need to change the general createHash function, unless you know it is used exclusively for password hashing (which, based on its name and structure, it is not).
Specifically:
- In
UserInfoResource.ts, define and use a secure password hashing function (bcrypt.hashSyncor the async version). - Add required
bcryptimport. - Use this new function to hash
oldPasswordandnewPasswordinupdateLocalPassword, instead of usingcreateHash. - You may need to set a salt rounds constant (recommended: 12).
-
Copy modified line R9 -
Copy modified lines R23-R29 -
Copy modified lines R247-R250
| @@ -6,6 +6,7 @@ | ||
| * you may not use this file except in compliance with the License. | ||
| */ | ||
| import { computed, makeObservable, runInAction } from 'mobx'; | ||
| import bcrypt from 'bcryptjs'; | ||
|
|
||
| import { injectable } from '@cloudbeaver/core-di'; | ||
| import { AutoRunningTask, type ISyncExecutor, type ITask, SyncExecutor, whileTask } from '@cloudbeaver/core-executor'; | ||
| @@ -19,6 +20,13 @@ | ||
| import type { IAuthCredentials } from './IAuthCredentials.js'; | ||
| import { createHash } from '@cloudbeaver/core-utils'; | ||
|
|
||
|
|
||
| // Secure password hashing function for user passwords | ||
| async function hashPassword(password: string): Promise<string> { | ||
| const SALT_ROUNDS = 12; | ||
| return await bcrypt.hash(password, SALT_ROUNDS); | ||
| } | ||
|
|
||
| export type UserLogoutInfo = AuthLogoutQuery['result']; | ||
|
|
||
| export interface ILoginOptions { | ||
| @@ -236,7 +244,10 @@ | ||
|
|
||
| async updateLocalPassword(oldPassword: string, newPassword: string): Promise<void> { | ||
| await this.performUpdate(undefined, [], async () => { | ||
| const [oldPasswordHash, newPasswordHash] = await Promise.all([createHash(oldPassword, 'sha256'), createHash(newPassword, 'sha256')]); | ||
| const [oldPasswordHash, newPasswordHash] = await Promise.all([ | ||
| hashPassword(oldPassword), | ||
| hashPassword(newPassword), | ||
| ]); | ||
|
|
||
| await this.graphQLService.sdk.authChangeLocalPassword({ | ||
| oldPassword: oldPasswordHash, |
-
Copy modified lines R40-R41
| @@ -37,7 +37,8 @@ | ||
| "@cloudbeaver/core-utils": "workspace:*", | ||
| "@dbeaver/js-helpers": "workspace:*", | ||
| "mobx": "^6", | ||
| "tslib": "^2" | ||
| "tslib": "^2", | ||
| "bcryptjs": "^3.0.2" | ||
| }, | ||
| "devDependencies": { | ||
| "@cloudbeaver/core-cli": "workspace:*", |
| Package | Version | Security advisories |
| bcryptjs (npm) | 3.0.2 | None |
…hash-to-pass-local-user-passwords
…r-than-md5-hash-to-pass-local-user-passwords' into 4569-cb-3681-use-something-better-than-md5-hash-to-pass-local-user-passwords
…hash-to-pass-local-user-passwords
| private static final Log log = Log.getLog(BruteForceUtils.class); | ||
|
|
||
| public static void checkBruteforce(SMControllerConfiguration smConfig, List<UserLoginRecord> latestLoginAttempts) | ||
| public static void checkBruteforce(SMControllerConfiguration smConfig, List<UserLoginRecord> latestLoginAttempts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotations + code style
| Map<String, Object> credentials = new LinkedHashMap<>(); | ||
| credentials.put(LocalAuthProviderConstants.CRED_USER, adminUser.getUserId()); | ||
| credentials.put(LocalAuthProviderConstants.CRED_PASSWORD, clientPassword); | ||
| credentials.put(LocalAuthProviderConstants.CRED_PASSWORD, adminPassword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove digest?
| { | ||
| adminName: "test", | ||
| adminPassword: "test", | ||
| adminPassword: "9F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why store digest but password?
|
|
||
| private void validatePasswordSha(String passwordSha, Map<String, Object> storedCredentials) throws DBException { | ||
| try { | ||
| if (!Argon2IdHasher.verify(CommonUtils.toString(storedCredentials.get(LocalAuthProviderConstants.CRED_PASSWORD)), passwordSha)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move all game with argon inside security utils. We mustn't know what algorithms are used on this level of abstraction.
| public record UserLoginRecord(SMAuthStatus smAuthStatus, LocalDateTime time) { | ||
| public record UserLoginRecord(SMAuthStatus smAuthStatus, | ||
| LocalDateTime time, | ||
| Map<String, Object> authState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that it is safe to read auth state. What changed, why do we need it now?
| public static final String CRED_PASSWORD = LocalAuthProviderConstants.CRED_PASSWORD; | ||
| public static final String AUTH_LOCAL_TYPE = "authLocalType"; | ||
| public static final String LEGACY_AUTH_LOCAL_TYPE = "legacy"; | ||
| public static final String NEW_AUTH_LOCAL_TYPE = "new"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's name it sha
|
|
||
| public static final String PROVIDER_ID = LocalAuthProviderConstants.PROVIDER_ID; | ||
| public static final String CRED_USER = LocalAuthProviderConstants.CRED_USER; | ||
| public static final String CRED_PASSWORD_MD_5 = LocalAuthProviderConstants.CRED_PASSWORD_MD5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MD5
| await authInfoService.login(provider.id, { | ||
| configurationId: configuration?.id, | ||
| credentials: { | ||
| ...state.credentials, | ||
| credentials: { | ||
| ...state.credentials.credentials, | ||
| user: state.credentials.credentials['user']?.trim(), | ||
| password: state.credentials.credentials['password']?.trim(), | ||
| }, | ||
| }, | ||
| forceSessionsLogout: state.forceSessionsLogout, | ||
| linkUser, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap this block with additional try/catch
if it's shouldReloginWithOldHash than run
await authInfoService.login(provider.id, {
configurationId: configuration?.id,
credentials: {
...state.credentials,
credentials: {
...state.credentials.credentials,
user: state.credentials.credentials['user']?.trim(),
password: state.credentials.credentials['password']?.trim(),
},
},
forceSessionsLogout: state.forceSessionsLogout,
linkUser,
});
again in catch block, if it throws - higher catch will get exception and handle it properly
| if (shouldReloginWithOldHash) { | ||
| try { | ||
| await authInfoService.login(provider.id, { | ||
| configurationId: configuration?.id, | ||
| credentials: { | ||
| ...state.credentials, | ||
| credentials: { | ||
| ...state.credentials.credentials, | ||
| user: state.credentials.credentials['user']?.trim(), | ||
| password: state.credentials.credentials['password']?.trim(), | ||
| }, | ||
| }, | ||
| forceSessionsLogout: state.forceSessionsLogout, | ||
| linkUser, | ||
| hasOldHash: true, | ||
| }); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove to upper place without try/catch as described above
…hash-to-pass-local-user-passwords
closes https://github.com/dbeaver/pro/issues/4569