codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3528736057
########## superset/security/manager.py: ########## @@ -118,6 +121,15 @@ logger = logging.getLogger(__name__) +# Fallback werkzeug-format hash used to balance timing on failed logins when +# ``AUTH_DB_FAKE_PASSWORD_HASH_CHECK`` is not provided by the Flask-AppBuilder +# config defaults. Mirrors FAB's pbkdf2 default so ``check_password_hash`` does +# comparable work whether or not the target user exists. +DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK = ( + "pbkdf2:sha256:150000$Z3t6fmj2$22da622d94a1f8118" # noqa: S105 + "c0976a03d2f18f680bfff877c9a965db9eedc51bc0be87c" +) Review Comment: **Suggestion:** The fallback fake hash is hardcoded to PBKDF2, so failed-login timing no longer matches real verification when users are stored with `argon2` (or non-PBKDF2 hashes). This creates a measurable username-enumeration side channel because non-existent users always take PBKDF2 cost while existing users take the configured hash cost. Use a fake hash that matches the configured AUTH_DB hash algorithm (and update it when the configured algorithm changes) so missing-user and existing-user failures do comparable work. [security] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx - β Login endpoint leaks AUTH_DB user existence via timing. - β οΈ Weakens resistance to username enumeration across AUTH_DB logins. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Configure AUTH_DB to use argon2 by setting ``AUTH_DB_CONFIG["password_hash_algorithm"] = "argon2"``, which `get_auth_db_password_hash_algorithm` reads in `superset/utils/auth_db_password.py:144-166` to drive hashing via `hash_auth_db_password` in `superset/utils/auth_db_password_hash.py:50-59`. 2. Create or migrate an AUTH_DB user so their `User.password` field contains an argon2 hash produced by `hash_auth_db_password` (argon2 branch at `superset/utils/auth_db_password_hash.py:56-59`), ensuring login verification uses argon2 for that account. 3. Send login requests to the `/login` route exposed by `SupersetAuthView` in `superset/views/auth.py:33-40`; Flask-AppBuilderβs `AuthView` (external dependency) uses the configured `SupersetSecurityManager.auth_user_db` method (`superset/security/manager.py:29-60`) to authenticate AUTH_DB users. 4. For a non-existent or inactive username, `auth_user_db` follows the `user is None or (not user.is_active)` branch at `superset/security/manager.py:42-54`, invoking `check_password_hash` against the PBKDF2 constant `DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK` defined at `superset/security/manager.py:128-131`; for an existing argon2 user with a wrong password, it calls `verify_auth_db_password` at `superset/security/manager.py:55`, which executes the argon2 verifier path in `superset/utils/auth_db_password_hash.py:89-93`. The difference between PBKDF2 work for missing users and argon2 work for existing users produces measurably different response times for failed logins, allowing an attacker to infer whether a given username exists. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1db56d634c1b4c8bb4101cca1a3d6ac0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1db56d634c1b4c8bb4101cca1a3d6ac0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/security/manager.py **Line:** 128:131 **Comment:** *Security: The fallback fake hash is hardcoded to PBKDF2, so failed-login timing no longer matches real verification when users are stored with `argon2` (or non-PBKDF2 hashes). This creates a measurable username-enumeration side channel because non-existent users always take PBKDF2 cost while existing users take the configured hash cost. Use a fake hash that matches the configured AUTH_DB hash algorithm (and update it when the configured algorithm changes) so missing-user and existing-user failures do comparable work. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=b6ad35f63e5d57b8a55ec8cd0fd83c7eed8e9dddee98dadf7e00dc4a787dc88e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=b6ad35f63e5d57b8a55ec8cd0fd83c7eed8e9dddee98dadf7e00dc4a787dc88e&reaction=dislike'>π</a> ########## superset/utils/auth_db_password_hash.py: ########## @@ -0,0 +1,98 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Hash and verify AUTH_DB passwords using bcrypt or argon2.""" + +from __future__ import annotations + +import re +from re import Pattern + +import bcrypt +from argon2 import PasswordHasher +from argon2.exceptions import Argon2Error +from werkzeug.security import check_password_hash + +from superset.utils.auth_db_password import ( + BCRYPT_MAX_PASSWORD_BYTES, + get_auth_db_password_hash_algorithm, +) + +_BCRYPT_HASH_RE: Pattern[str] = re.compile(r"^\$2[aby]\$\d{2}\$") +_ARGON2_HASH_PREFIX: str = "$argon2" + +_argon2_hasher: PasswordHasher = PasswordHasher() + + +def is_bcrypt_password_hash(password_hash: str) -> bool: + """Return True when ``password_hash`` uses the bcrypt wire format.""" + return bool(_BCRYPT_HASH_RE.match(password_hash)) + + +def is_argon2_password_hash(password_hash: str) -> bool: + """Return True when ``password_hash`` uses the argon2 wire format.""" + return password_hash.startswith(_ARGON2_HASH_PREFIX) + + +def hash_auth_db_password(password: str, algorithm: str | None = None) -> str: + """ + Hash a plaintext password for AUTH_DB storage. + + Uses ``AUTH_DB_CONFIG["password_hash_algorithm"]`` when ``algorithm`` is omitted. + """ + resolved: str = algorithm or get_auth_db_password_hash_algorithm() + if resolved == "argon2": + return _argon2_hasher.hash(password) + if resolved == "bcrypt": + encoded: bytes = password.encode("utf-8") + if len(encoded) > BCRYPT_MAX_PASSWORD_BYTES: + raise ValueError( + f"Password exceeds bcrypt's {BCRYPT_MAX_PASSWORD_BYTES}-byte limit." + ) + return bcrypt.hashpw(encoded, bcrypt.gensalt()).decode("utf-8") Review Comment: **Suggestion:** The bcrypt hashing branch encodes raw input with UTF-8 without handling encoding failures, so malformed Unicode passwords can raise `UnicodeEncodeError` and break password-change requests with a 500. Catch `UnicodeEncodeError` here and convert it to a controlled validation failure. [type error] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx - β Password change API may 500 on malformed password. - β οΈ Future bcrypt utility callers brittle to encoding issues. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. In a Python shell, import `hash_auth_db_password` from `superset/utils/auth_db_password_hash.py:50-66` and ensure `AUTH_DB_CONFIG["password_hash_algorithm"]` is left at the default `"bcrypt"` (resolved via `get_auth_db_password_hash_algorithm` at `superset/utils/auth_db_password.py:144-166`). 2. Construct an invalid Unicode `str` such as `bad_password = "\udcff"` and call `hash_auth_db_password(bad_password)` to exercise the bcrypt branch. 3. Execution reaches the bcrypt block at `superset/utils/auth_db_password_hash.py:59-65`; `bad_password.encode("utf-8")` on line 60 raises `UnicodeEncodeError` before any controlled `ValueError` or hash is produced, and this exception is not handled inside `hash_auth_db_password`. 4. In the password-change API handler at `superset/views/users/api.py:378`, `hash_auth_db_password(body["new_password"])` is called inside a `try` that only catches `marshmallow.ValidationError`, so the same malformed input passed through `_load_password_change_body` and `validate_auth_db_password` results in an unhandled exception and HTTP 500 for `PUT /api/v1/me/password` instead of a structured validation failure. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1cce00d9f73847798a9bd13385879507&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1cce00d9f73847798a9bd13385879507&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/auth_db_password_hash.py **Line:** 59:65 **Comment:** *Type Error: The bcrypt hashing branch encodes raw input with UTF-8 without handling encoding failures, so malformed Unicode passwords can raise `UnicodeEncodeError` and break password-change requests with a 500. Catch `UnicodeEncodeError` here and convert it to a controlled validation failure. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=b07406b7f30e04248397580e171eb895aad976ebe753983c7c56bba8c4d3fadb&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=b07406b7f30e04248397580e171eb895aad976ebe753983c7c56bba8c4d3fadb&reaction=dislike'>π</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
