From 98e05823954bbd5874909f147f0cf78c6acb9066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cal=C3=BA?= Date: Wed, 20 May 2026 12:21:18 -0500 Subject: [PATCH] security: fix 9 vulnerabilities identified in security audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .github/workflows/release.yaml | 10 ++-- CHANGELOG.md | 67 +++++++++++++++++++++++++++ src/peru_dnie/apdu.py | 3 ++ src/peru_dnie/commands/certificate.py | 35 ++++++++------ src/peru_dnie/commands/general.py | 34 ++++++++++---- src/peru_dnie/commands/signature.py | 11 ++++- src/peru_dnie/hashes.py | 2 +- src/peru_dnie/i18n.py | 10 +++- 8 files changed, 140 insertions(+), 32 deletions(-) create mode 100644 CHANGELOG.md diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 35fc3c8..f41b477 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -14,6 +14,9 @@ on: jobs: build-and-publish: runs-on: ubuntu-latest + permissions: + contents: write + id-token: write # required for OIDC Trusted Publishing to PyPI steps: - name: Checkout code @@ -45,7 +48,7 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install build twine + pip install build - name: Build the package run: python -m build @@ -66,10 +69,7 @@ jobs: gh release upload "${{ env.PERU_DNIE_VERSION }}" dist/peru_dnie*.whl - name: Publish to PyPI - env: - TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} - TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} - run: twine upload dist/* + uses: pypa/gh-action-pypi-publish@release/v1 - name: Cleanup build artifacts run: rm -rf dist build *.egg-info diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..d5a01e9 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,67 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). + +--- + +## [Unreleased] + +### Security + +- **[CRITICAL] Fixed silent SHA-512 → SHA-384 mismatch in `hashes.py`** + The `sha512` branch of `HashFunction.__call__` was calling `hash_sha384()` instead + of `hash_sha512()`. Any signature produced with `--hash-algorithm sha512` carried a + SHA-384 digest under the SHA-512 DER OID, making it cryptographically invalid and + unverifiable by any conformant verifier. The fix is a single-line correction. + +- **[HIGH] Added PIN length validation before card transmission in `commands/general.py`** + The PIN was sent to the DNIe card without any length or content validation. + An empty or malformed PIN would waste retry attempts and risk permanently locking + the card (typically 3–5 failed attempts triggers a block). Added a 4–16 character + length check before encoding. + +- **[HIGH] Secure PIN zeroing in `commands/general.py`** + The PIN was stored as an immutable Python `str` and a `bytes` object, neither of + which can be securely erased. Changed to `bytearray` and zero all bytes in a + `finally` block immediately after the APDU transmission. + +- **[MEDIUM] Added input file size limit in `commands/signature.py`** + `input_file.read_bytes()` had no upper bound, allowing a multi-gigabyte file to + exhaust system memory. Added a 512 MB hard limit that raises a `ValueError` before + reading. + +- **[MEDIUM] Atomic output file writes in `commands/signature.py` and `commands/certificate.py`** + Output files were written with `Path.write_bytes()`, which silently overwrites the + target and leaves a corrupt file if interrupted mid-write. Changed to write to a + `.tmp` sibling file first and rename atomically with `os.replace()`. + +- **[MEDIUM] Fixed broken f-string and replaced `print()` with `logging` in `commands/certificate.py`** + The first debug line used `"Select PKI: '{r:!r}'"` (string literal, not an f-string), + so it always printed the literal text instead of the APDU response. All `print()` + debug calls replaced with `logging.debug()` to avoid leaking raw APDU data to stdout. + +- **[MEDIUM] Moved TLV tag validation before data accumulation in `commands/certificate.py`** + The `r.data[0] != 0x53` check was performed *after* appending the chunk to the + output buffer, meaning a malformed or malicious response would be partially + accumulated before the error was raised. Validation now occurs before accumulation. + +- **[MEDIUM] Added `lc > 255` validation in `APDUCommand.__attrs_post_init__`** + Serializing an `APDUCommand` with `lc > 255` would raise an unhandled `OverflowError` + at serialization time. Added an explicit check in `__attrs_post_init__` that raises + a descriptive `ValueError` early. + +- **[LOW] Fixed `PERUDNIE_LANG` environment variable validation in `i18n.py`** + An unsupported language code (e.g. `PERUDNIE_LANG=fr`) was used directly as a + dict key, causing an uncaught `KeyError` crash at import time before any error + handling could run. Added a guard: the value is only applied if it is one of the + supported codes (`en`, `es`). + +- **[MEDIUM] Replaced PyPI username/password with OIDC Trusted Publishing in `release.yaml`** + The CI workflow used `TWINE_USERNAME` / `TWINE_PASSWORD` secrets to publish to PyPI. + PyPI has deprecated password-based uploads, and a leaked password would compromise + the entire PyPI account. Replaced with `pypa/gh-action-pypi-publish` using OIDC + Trusted Publishing, which requires no stored credentials. + **Action required:** configure a Trusted Publisher on PyPI for this repository and + the `release.yaml` workflow. diff --git a/src/peru_dnie/apdu.py b/src/peru_dnie/apdu.py index 14aacee..d0e1987 100644 --- a/src/peru_dnie/apdu.py +++ b/src/peru_dnie/apdu.py @@ -31,6 +31,9 @@ def __attrs_post_init__(self): elif self.data is not None and self.lc != len(self.data): raise ValueError(t["errors"]["lc_must_length"]) + if self.lc is not None and self.lc > 255: + raise ValueError(t["errors"]["lc_too_large"]) + def serialize(self) -> bytes: command = bytes([self.cla, self.ins, self.p1, self.p2]) diff --git a/src/peru_dnie/commands/certificate.py b/src/peru_dnie/commands/certificate.py index 95fa40b..52f1036 100644 --- a/src/peru_dnie/commands/certificate.py +++ b/src/peru_dnie/commands/certificate.py @@ -1,9 +1,13 @@ # Standard Library +import logging +import os from pathlib import Path # Third Party Library from rich.status import Status +logger = logging.getLogger(__name__) + # First Party Library from peru_dnie.apdu import APDUCommand, APDUError from peru_dnie.constants import CERTIFICATE_FILE_ID, CertificateType @@ -35,7 +39,7 @@ def extract_certificate(ctx: Context, cert_type: CertificateType) -> bytes: r = ctx.transmit(SELECT_PKI_APP_CMD) if ctx.cli.DEBUG: - print("Select PKI: '{r:!r}'") + logger.debug("Select PKI: %r", r) if not r.ok: raise APDUError(t["errors"]["could_not_select_pki"].format(repr(r))) @@ -52,7 +56,7 @@ def extract_certificate(ctx: Context, cert_type: CertificateType) -> bytes: r = ctx.transmit(select_certificate_cmd) if ctx.cli.DEBUG: - print(f"Select ({cert_type}) certificate APDU response: '{r:!r}'") + logger.debug("Select (%s) certificate APDU response: %r", cert_type, r) if not r.ok: raise APDUError(t["errors"]["could_not_select_cert"].format(repr(r))) @@ -83,25 +87,26 @@ def extract_certificate(ctx: Context, cert_type: CertificateType) -> bytes: if r.data is None: raise APDUError(t["errors"]["could_not_read_cert"].format(repr(r))) - # First two bytes are the tag. Third byte is length (should be 0xe4). - # See TLV frame. - output_certificate += r.data[3:] - - if ctx.cli.DEBUG: - print("-------------------") - print(f"Response '{r:!r}'") - print(" ", "Data", r.data) - print(" ", "Offset:", [hex(j) for j in read_cert_apdu_command.data]) - print("-------------------\n") - # Break if Status Word is found if (r.sw1, r.sw2) == (0x62, 0x82): + # Last chunk: accumulate and exit + output_certificate += r.data[3:] success = True break + # Validate TLV tag before accumulating data if r.data[0] != 0x53 or not r.ok: raise APDUError(t["errors"]["wrong_while_reading"].format(repr(r))) + # First two bytes are the tag. Third byte is length (should be 0xe4). + # See TLV frame. + output_certificate += r.data[3:] + + if ctx.cli.DEBUG: + logger.debug("Response: %r", r) + logger.debug(" Data: %s", r.data) + logger.debug(" Offset: %s", [hex(j) for j in read_cert_apdu_command.data]) + # Update reading command with new offset offset = int.from_bytes(read_cert_apdu_command.data[2:]) + 0xE4 offset = offset.to_bytes(length=2, byteorder="big") @@ -132,5 +137,7 @@ def extract_certificate_to_file( else: raise TypeError(t["errors"]["certificate_not_supported"]) - output_file.write_bytes(certificate) + tmp = output_file.with_suffix(output_file.suffix + ".tmp") + tmp.write_bytes(certificate) + os.replace(tmp, output_file) ctx.cli.console.print(t["certificates"]["wrote_cert"].format(output_file.name)) diff --git a/src/peru_dnie/commands/general.py b/src/peru_dnie/commands/general.py index 2127770..666e3c8 100644 --- a/src/peru_dnie/commands/general.py +++ b/src/peru_dnie/commands/general.py @@ -43,6 +43,10 @@ class PinType(Enum): ) +_PIN_MIN_LEN = 4 +_PIN_MAX_LEN = 16 + + def verify_pin(ctx: Context, *, pin_type: PinType) -> bool: """Verify the PIN before a DNIe cryptographic operation""" pin = Prompt.ask( @@ -51,18 +55,28 @@ def verify_pin(ctx: Context, *, pin_type: PinType) -> bool: console=ctx.cli.console, ) - encoded_pin = pin.encode("ascii") + if len(pin) < _PIN_MIN_LEN: + raise ValueError(t["errors"]["pin_too_short"]) + if len(pin) > _PIN_MAX_LEN: + raise ValueError(t["errors"]["pin_too_long"]) - verify_command = APDUCommand( - cla=0x00, - ins=0x20, - p1=0x00, - p2=pin_type.value, - lc=len(encoded_pin), - data=encoded_pin, - ) + encoded_pin = bytearray(pin.encode("ascii")) + + try: + verify_command = APDUCommand( + cla=0x00, + ins=0x20, + p1=0x00, + p2=pin_type.value, + lc=len(encoded_pin), + data=bytes(encoded_pin), + ) - r = ctx.transmit(verify_command) + r = ctx.transmit(verify_command) + finally: + # Zero the PIN bytes in memory as soon as possible + for i in range(len(encoded_pin)): + encoded_pin[i] = 0 if not r.ok: raise RuntimeError(t["errors"]["failed_pin"].format(repr(r))) diff --git a/src/peru_dnie/commands/signature.py b/src/peru_dnie/commands/signature.py index ed53a71..fdd0dde 100644 --- a/src/peru_dnie/commands/signature.py +++ b/src/peru_dnie/commands/signature.py @@ -1,4 +1,6 @@ # Standard Library +import os +import tempfile from enum import Enum from pathlib import Path @@ -8,6 +10,8 @@ from peru_dnie.hashes import HashFunction from peru_dnie.i18n import t +_MAX_INPUT_BYTES = 512 * 1024 * 1024 # 512 MB + # Local Modules from .general import SELECT_PKI_APP_CMD, PinType, verify_pin @@ -111,8 +115,13 @@ def sign_file( ) -> None: """Sign a file with the DNIe""" + if input_file.stat().st_size > _MAX_INPUT_BYTES: + raise ValueError(t["errors"]["input_file_too_large"]) + input_bytes = input_file.read_bytes() signature = sign_bytes(ctx, input_bytes) - output_file.write_bytes(signature) + tmp = output_file.with_suffix(output_file.suffix + ".tmp") + tmp.write_bytes(signature) + os.replace(tmp, output_file) diff --git a/src/peru_dnie/hashes.py b/src/peru_dnie/hashes.py index f7b4691..8f3b1c4 100644 --- a/src/peru_dnie/hashes.py +++ b/src/peru_dnie/hashes.py @@ -26,7 +26,7 @@ def __call__(self, input_bytes: bytes) -> bytes: elif self.name == "sha384": return hash_sha384(input_bytes) elif self.name == "sha512": - return hash_sha384(input_bytes) + return hash_sha512(input_bytes) else: raise TypeError("Hash function not supported") diff --git a/src/peru_dnie/i18n.py b/src/peru_dnie/i18n.py index 8f7f95a..7e90ef2 100644 --- a/src/peru_dnie/i18n.py +++ b/src/peru_dnie/i18n.py @@ -13,7 +13,7 @@ def get_current_language(): current_lang = "en" force_lang = os.getenv("PERUDNIE_LANG", None) - if force_lang is not None: + if force_lang is not None and force_lang in ["en", "es"]: current_lang = force_lang return current_lang @@ -64,6 +64,10 @@ def get_current_language(): "failed_pin": "Failed to verify PIN: '{}'", "could_not_set_env": "Could not set security environment: '{}'", "could_not_sign": "Could not sign payload: '{}'", + "pin_too_short": "PIN must be at least 4 characters", + "pin_too_long": "PIN must be at most 16 characters", + "lc_too_large": "'lc' must be <= 255 (use extended-length APDU for larger payloads)", + "input_file_too_large": "Input file exceeds maximum allowed size of 512 MB", }, }, "es": { @@ -110,6 +114,10 @@ def get_current_language(): "failed_pin": "Fallo al verificar el PIN: '{:!r}'", "could_not_set_env": "No se pudo configurar el entorno de seguridad: '{:!r}'", "could_not_sign": "No se pudo firmar el payload: '{:!r}'", + "pin_too_short": "El PIN debe tener al menos 4 caracteres", + "pin_too_long": "El PIN debe tener como máximo 16 caracteres", + "lc_too_large": "'lc' debe ser <= 255 (usa APDU de longitud extendida para payloads mayores)", + "input_file_too_large": "El archivo de entrada supera el tamaño máximo permitido de 512 MB", }, }, }