Skip to content

security: fix 9 vulnerabilities (critical SHA-512 bug, PIN handling, atomic writes, CI/CD) - #2

Open
calu777 wants to merge 1 commit into
edeustua:mainfrom
calu777:main
Open

security: fix 9 vulnerabilities (critical SHA-512 bug, PIN handling, atomic writes, CI/CD)#2
calu777 wants to merge 1 commit into
edeustua:mainfrom
calu777:main

Conversation

@calu777

@calu777 calu777 commented May 20, 2026

Copy link
Copy Markdown

Summary

Security audit of the codebase identified 9 vulnerabilities ranging from a critical cryptographic bug to medium/low hardening issues. This PR fixes all of them.

  • [CRITICAL] hashes.py: sha512 branch was silently calling hash_sha384() — any signature produced with --hash-algorithm sha512 was cryptographically invalid (SHA-384 digest under SHA-512 DER OID), making it unverifiable by any conformant verifier
  • [HIGH] commands/general.py: PIN sent to card without length validation, risking card lock-out (3–5 failed attempts permanently disables the DNIe); PIN stored as immutable str/bytes with no zeroing after use
  • [MEDIUM] commands/signature.py: unbounded file read into RAM (no size limit); non-atomic output write (corrupt file on interruption)
  • [MEDIUM] commands/certificate.py: broken f-string in debug line ("Select PKI: '{r:!r}'" literal instead of f-string); raw APDU data leaked to stdout via print(); TLV tag validated after accumulating data instead of before; non-atomic output write
  • [MEDIUM] apdu.py: lc > 255 caused an unhandled OverflowError at serialization time instead of a clear ValueError at construction time
  • [LOW] i18n.py: unsupported PERUDNIE_LANG value (e.g. fr) caused a KeyError crash at import time before any error handling could run
  • [MEDIUM] release.yaml: PyPI publish used deprecated username/password secrets; replaced with OIDC Trusted Publishing via pypa/gh-action-pypi-publish (no stored credentials)

Changes

File Change
src/peru_dnie/hashes.py Fix sha512 branch to call hash_sha512()
src/peru_dnie/commands/general.py Add PIN length validation (4–16 chars); use bytearray + zeroing in finally block
src/peru_dnie/commands/signature.py Enforce 512 MB file size limit; atomic write via .tmp + os.replace()
src/peru_dnie/commands/certificate.py Fix f-string; replace print() with logging.debug(); validate TLV before accumulate; atomic write
src/peru_dnie/apdu.py Raise ValueError in __attrs_post_init__ when lc > 255
src/peru_dnie/i18n.py Guard PERUDNIE_LANG against unsupported values; add error messages for new validations
.github/workflows/release.yaml Replace twine + password secrets with pypa/gh-action-pypi-publish OIDC
CHANGELOG.md Document all findings and fixes

Note on PyPI Trusted Publishing

The release.yaml change requires a one-time setup in PyPI: add a Trusted Publisher entry for this repository pointing to the release.yaml workflow. No secrets need to be stored. See the PyPI docs for instructions.

Test plan

  • dniectl sign --hash-algorithm sha512 <file> <out> produces a valid signature verifiable with openssl dgst -sha512 -verify
  • dniectl sign --hash-algorithm sha256 <file> <out> still works correctly (regression check)
  • Entering a PIN shorter than 4 characters is rejected with a clear error before any APDU is sent
  • Passing a file larger than 512 MB to dniectl sign is rejected immediately
  • Setting PERUDNIE_LANG=fr no longer crashes; falls back to system locale or en
  • CI release workflow publishes to PyPI successfully via OIDC (requires Trusted Publisher configured on PyPI)

- hashes.py: fix sha512 branch silently calling hash_sha384 (critical —
  produced cryptographically invalid signatures with wrong DER OID)
- general.py: validate PIN length (4-16 chars) before card transmission
  to prevent wasting retry attempts and risking card lock-out; store
  encoded PIN as bytearray and zero it in a finally block
- signature.py: reject input files over 512 MB; write output atomically
  via .tmp + os.replace() to prevent corrupt files on interruption
- certificate.py: fix broken f-string in debug line; replace print()
  with logging.debug(); validate TLV tag before accumulating response
  data; write output atomically via .tmp + os.replace()
- apdu.py: raise ValueError early when lc > 255 instead of crashing
  with OverflowError at serialization time
- i18n.py: ignore unsupported PERUDNIE_LANG values instead of raising
  KeyError at import time
- release.yaml: replace deprecated PyPI username/password with OIDC
  Trusted Publishing via pypa/gh-action-pypi-publish
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.

1 participant