Skip to content

Conversation

@tknie
Copy link
Owner

@tknie tknie commented Jan 7, 2026

Potential fix for https://github.com/tknie/services/security/code-scanning/17

In general, to fix this issue, password hashing must use a dedicated password hashing / key derivation function that is intentionally slow and tunable (e.g., bcrypt, scrypt, Argon2, or PBKDF2), not a fast hash like SHA-512. The function should also incorporate a per-password random salt. This makes brute-force and precomputed attacks much more expensive.

The best change here, without altering the rest of the authentication flow, is to modify GenerateHash so that for password hashing it uses a strong KDF like PBKDF2 with many iterations, while still returning a hex-encoded string so the callPasswordFileUserAuthenticate comparison keeps working. Since we must only change shown code in auth/passwdfile.go and should prefer well-known standard libraries, we can switch the SHA-related cases (SHA256, SHA512, and potentially SHA) to use crypto/pbkdf2 with an appropriate underlying hash (e.g., SHA-256 or SHA-512) and a fixed-cost iteration count. Because we cannot see how passwords are stored and there is no salt in the file format, we should (a) improve the cost characteristics (PBKDF2) while (b) leaving the actual string output format intact. A fully correct design would add per-user salts and storage changes, but that would likely break compatibility, which we must avoid given the limited context.

Concretely:

  • Add imports in auth/passwdfile.go:
    • crypto/pbkdf2
    • crypto/rand
    • encoding/hex
    • We can also reuse crypto/sha256 or crypto/sha512 as the PRF for PBKDF2 (already imported).
  • Extend GenerateHash:
    • Keep MD5 and SHA (SHA-1) branches as-is (for compatibility), but preferably discourage their use via comments.
    • For SHA256 and SHA512 cases, instead of directly hashing []byte(password) once, derive a key using PBKDF2:
      • Use a fixed, derived salt per algorithm if we absolutely cannot change the storage format (still not ideal but improves brute-force resistance via iteration cost). A better but breaking change would be to include random salt and store salt:hash.
      • Use a reasonably high iteration count (e.g., 100_000) and key length (e.g., 32 bytes).
      • Return the derived key as a hex string (hex.EncodeToString), so comparisons stay string-based.
  • Optionally, add a new algorithm identifier (e.g., PBKDF2) and encourage using it, but to avoid changing behavior, we can first just upgrade existing SHA256/SHA512 cases.

We must ensure that only the code shown is changed. All edits will be in GenerateHash and the import block.


Suggested fixes powered by Copilot Autofix. Review carefully before merging.

tknie and others added 2 commits January 7, 2026 20:40
… cryptographic hashing algorithm on sensitive data

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@tknie tknie marked this pull request as ready for review January 7, 2026 20:47
@tknie tknie merged commit 75d4be8 into main Jan 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants