diff --git a/packages/volo-packs/src/volo_packs/signing.py b/packages/volo-packs/src/volo_packs/signing.py index 77d57e3..15159e5 100644 --- a/packages/volo-packs/src/volo_packs/signing.py +++ b/packages/volo-packs/src/volo_packs/signing.py @@ -17,7 +17,7 @@ import json from pathlib import Path -from volo_packs.pack import Pack, PackSignature +from volo_packs.pack import Pack, PackSignature, content_checksum HMAC_SHA256 = "hmac-sha256" @@ -40,10 +40,18 @@ def sign_pack(pack: Pack, *, publisher: str, secret: str) -> Pack: def verify_pack_signature(pack: Pack, keyring: Keyring) -> bool: - """True if the pack carries a signature from a keyring publisher over its current content.""" + """True if the pack carries a valid signature from a keyring publisher over its **actual** content. + + The signed message binds the manifest checksum, so verification MUST first confirm the manifest + checksum still matches the real items — otherwise an attacker could swap ``pack.items`` while + leaving ``manifest.checksum`` (and thus the signature) untouched and still verify as valid. + """ sig = pack.manifest.signature if sig is None or sig.algorithm != HMAC_SHA256: return False + # Re-bind to real content: a stale/forged manifest checksum invalidates the signature. + if content_checksum(pack.items) != pack.manifest.checksum: + return False secret = keyring.get(sig.publisher) if secret is None: return False diff --git a/packages/volo-packs/tests/test_signing_content_binding.py b/packages/volo-packs/tests/test_signing_content_binding.py new file mode 100644 index 0000000..ae833ad --- /dev/null +++ b/packages/volo-packs/tests/test_signing_content_binding.py @@ -0,0 +1,33 @@ +"""Regression: a signature must not verify once the pack's actual content is tampered.""" + +from __future__ import annotations + +from volo_packs import build_pack, sign_pack, starter_items, verify_pack_signature +from volo_packs.pack import Pack + + +def _signed() -> Pack: + pack = build_pack(name="p", version="1.0.0", kind="attacks", items=starter_items("attacks")) + sign_pack(pack, publisher="acme", secret="s3cret") + return pack + + +def test_untampered_pack_verifies() -> None: + assert verify_pack_signature(_signed(), {"acme": "s3cret"}) is True + + +def test_content_tamper_invalidates_signature() -> None: + pack = _signed() + # swap the payload but leave manifest.checksum (and thus the HMAC) untouched + pack.items.append( + {"id": "evil", "category": "x", "technique": "pwn", "payload": "rm -rf /", + "expected_behavior": "refuse"} + ) + assert verify_pack_signature(pack, {"acme": "s3cret"}) is False + + +def test_manifest_checksum_forgery_invalidates_signature() -> None: + pack = _signed() + pack.items.clear() + pack.manifest.checksum = "deadbeef" # forged to match nothing real + assert verify_pack_signature(pack, {"acme": "s3cret"}) is False