Potential fix for code scanning alert no. 34: Use of a broken or weak cryptographic hashing algorithm on sensitive data - #34
Open
imShakil wants to merge 4 commits into
Open
Potential fix for code scanning alert no. 34: Use of a broken or weak cryptographic hashing algorithm on sensitive data#34imShakil wants to merge 4 commits into
imShakil wants to merge 4 commits into
Conversation
… cryptographic hashing algorithm on sensitive data Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…eak cryptographic hashing algorithm on sensitive data' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
imShakil
marked this pull request as ready for review
May 4, 2026 16:34
imShakil
enabled auto-merge
May 4, 2026 16:34
There was a problem hiding this comment.
Pull request overview
This PR aims to remediate code scanning alert #34 by moving master-password verification away from fast SHA-256 hashing toward a slow, salted PBKDF2-based scheme, with backward compatibility for existing stored hashes.
Changes:
- Adds PBKDF2 (hashlib.pbkdf2_hmac) password hashing and verification helpers and uses constant-time comparison (hmac.compare_digest).
- Updates
verify_master_passwordto recognize PBKDF2-formatted records and attempt legacy compatibility/migration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+314
to
+317
| wrapped = self._hash_legacy_sha256_with_pbkdf2(legacy_sha256) | ||
| with open(PASSWORD_HASH_PATH, "w") as f: | ||
| f.write(wrapped) | ||
| legacy_ok = self._verify_legacy_sha256_with_pbkdf2(password, wrapped) |
Comment on lines
+241
to
+256
| PBKDF2_ITERATIONS = 310000 | ||
| PBKDF2_SALT_BYTES = 16 | ||
|
|
||
| def _hash_password_pbkdf2(self, password: str, salt: bytes = None) -> str: | ||
| if salt is None: | ||
| salt = os.urandom(self.PBKDF2_SALT_BYTES) | ||
| dk = hashlib.pbkdf2_hmac( | ||
| "sha256", | ||
| password.encode(), | ||
| salt, | ||
| self.PBKDF2_ITERATIONS, | ||
| ) | ||
| salt_b64 = base64.b64encode(salt).decode("utf-8") | ||
| dk_b64 = base64.b64encode(dk).decode("utf-8") | ||
| return f"pbkdf2_sha256${self.PBKDF2_ITERATIONS}${salt_b64}${dk_b64}" | ||
|
|
Comment on lines
+257
to
+266
| def _verify_password_pbkdf2(self, password: str, stored: str) -> bool: | ||
| try: | ||
| scheme, iter_str, salt_b64, expected_b64 = stored.split("$", 3) | ||
| if scheme != "pbkdf2_sha256": | ||
| return False | ||
| iterations = int(iter_str) | ||
| salt = base64.b64decode(salt_b64.encode("utf-8")) | ||
| expected = base64.b64decode(expected_b64.encode("utf-8")) | ||
| actual = hashlib.pbkdf2_hmac("sha256", password.encode(), salt, iterations) | ||
| return hmac.compare_digest(actual, expected) |
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| # Legacy fallback: support old SHA-256 hashes and transparently upgrade. | ||
| legacy_ok = False | ||
|
|
||
| legacy_sha256 = hashlib.sha256(password.encode()).hexdigest() |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+259
to
+266
| scheme, iter_str, salt_b64, expected_b64 = stored.split("$", 3) | ||
| if scheme != "pbkdf2_sha256": | ||
| return False | ||
| iterations = int(iter_str) | ||
| salt = base64.b64decode(salt_b64.encode("utf-8")) | ||
| expected = base64.b64decode(expected_b64.encode("utf-8")) | ||
| actual = hashlib.pbkdf2_hmac("sha256", password.encode(), salt, iterations) | ||
| return hmac.compare_digest(actual, expected) |
Comment on lines
+270
to
+295
| def _hash_legacy_sha256_with_pbkdf2(self, legacy_sha256_hex: str, salt: bytes = None) -> str: | ||
| if salt is None: | ||
| salt = os.urandom(self.PBKDF2_SALT_BYTES) | ||
| dk = hashlib.pbkdf2_hmac( | ||
| "sha256", | ||
| legacy_sha256_hex.encode("utf-8"), | ||
| salt, | ||
| self.PBKDF2_ITERATIONS, | ||
| ) | ||
| salt_b64 = base64.b64encode(salt).decode("utf-8") | ||
| dk_b64 = base64.b64encode(dk).decode("utf-8") | ||
| return f"legacy_pbkdf2_sha256${self.PBKDF2_ITERATIONS}${salt_b64}${dk_b64}" | ||
|
|
||
| def _verify_legacy_sha256_with_pbkdf2(self, legacy_sha256_hex: str, stored: str) -> bool: | ||
| try: | ||
| scheme, iter_str, salt_b64, expected_b64 = stored.split("$", 3) | ||
| if scheme != "legacy_pbkdf2_sha256": | ||
| return False | ||
| iterations = int(iter_str) | ||
| salt = base64.b64decode(salt_b64.encode("utf-8")) | ||
| expected = base64.b64decode(expected_b64.encode("utf-8")) | ||
| actual = hashlib.pbkdf2_hmac("sha256", legacy_sha256_hex.encode("utf-8"), salt, iterations) | ||
| return hmac.compare_digest(actual, expected) | ||
| except Exception: | ||
| return False | ||
|
|
Comment on lines
+305
to
+319
| # Legacy fallback: support old SHA-256 hashes and transparently upgrade. | ||
| legacy_ok = False | ||
|
|
||
| legacy_sha256 = hashlib.sha256(password.encode()).hexdigest() | ||
|
|
||
| if stored_hash.startswith("legacy_pbkdf2_sha256$"): | ||
| legacy_ok = self._verify_legacy_sha256_with_pbkdf2(legacy_sha256, stored_hash) | ||
| else: | ||
| # One-time migration for existing raw SHA-256 legacy values. | ||
| legacy_ok = hmac.compare_digest(stored_hash, legacy_sha256) | ||
|
|
||
| if legacy_ok: | ||
| with open(PASSWORD_HASH_PATH, "w") as f: | ||
| f.write(self._hash_password_pbkdf2(password)) | ||
| return legacy_ok |
Comment on lines
+316
to
+319
| if legacy_ok: | ||
| with open(PASSWORD_HASH_PATH, "w") as f: | ||
| f.write(self._hash_password_pbkdf2(password)) | ||
| return legacy_ok |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/imShakil/pacli/security/code-scanning/34
Use a dedicated password hashing function that is intentionally expensive and includes a salt/work factor. Since we should avoid adding new dependencies and only edit shown code, the best fit is Python stdlib
hashlib.pbkdf2_hmacwith a per-password random salt and high iteration count, storing a structured value likepbkdf2_sha256$<iterations>$<salt_b64>$<hash_b64>.In
pacli/store.py, replace the SHA-256 comparison inverify_master_password(line region around 240–246) with parser+verifier logic for PBKDF2 records using constant-time comparison (hmac.compare_digest). Keep backward compatibility by supporting legacy SHA-256 entries: if the stored file is not PBKDF2 format, verify using old SHA-256 once; if it matches, transparently upgrade the stored hash to PBKDF2 format. This preserves existing functionality while remediating the weakness for all future checks. Add only stdlib imports needed (hmac) and helper methods inside the same file/class.Suggested fixes powered by Copilot Autofix. Review carefully before merging.