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

.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

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.

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])
 

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))

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)))

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)

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")
 

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",
         },
     },
 }