Skip to content

Harden signing pipeline - #1

Closed
Loggableim wants to merge 1 commit into
Le-Syl21:mainfrom
Loggableim:agent/harden-signing-pipeline
Closed

Loggableim wants to merge 1 commit into
Le-Syl21:mainfrom
Loggableim:agent/harden-signing-pipeline

Conversation

@Loggableim

Copy link
Copy Markdown
Contributor

What changed

  • Require explicit opt-in before using Certum's HTTP-only timestamp authority, and document the transport risk in the CLI, README, protocol reference, and release CI.
  • Reject unexpected timestamp response types and non-CMS timestamp tokens before embedding them.
  • Match the cloud signature response to the exact requested digest.
  • Validate PE certificate-table bounds before hashing malformed inputs.
  • Write signed outputs through synced temporary files with atomic replacement, preserve permissions, and create non-overwritable <file>.orig backups.

Why

The signing path handles security-sensitive credentials and release artifacts. Plaintext timestamp transport, unvalidated response selection, malformed PE offsets, and direct in-place writes could otherwise yield invalid output or overwrite recoverable input.

Validation

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test --release (13 passed; 2 existing ignored tests)
  • Release CLI check: default HTTP timestamping is rejected unless --allow-insecure-timestamp is provided.

@Le-Syl21

Copy link
Copy Markdown
Owner

Thanks for this — there's real substance in here, and a couple of things I want regardless of what happens to the rest.

What I want to take:

  • sign.rs, matching the response to the requested digest. This is the most valuable line in the PR. Taking m.values().next() means that when signing a batch, the signature attached to a file is whatever the server listed first. Please split this out and I'll merge it on its own.
  • The certificate-table bounds check. Yes — a malformed PE shouldn't be able to walk us off the end of the buffer.
  • Atomic writes. Agreed, the current in-place fs::write will corrupt the input if the process dies mid-write, and there's no way back. Preserving permissions and refusing to clobber an existing .orig are both right.
  • Structural validation of the timestamp token before embedding it — reasonable.

What I don't want to take, and why:

--allow-insecure-timestamp. I think this misreads the threat. RFC3161 over HTTP is the norm precisely because the token is signed by the TSA — a network attacker cannot forge a timestamp, and a substituted token won't verify. Certum, DigiCert, Sectigo and GlobalSign all serve plain HTTP for this reason. What's actually left is denial or a stale token, which is a real but modest concern and not what the flag name implies. Meanwhile the cost is concrete: every existing user's CI breaks on upgrade until they add a flag that reads like a security warning, and the README has to teach people to pass it by default. If there's a specific attack I'm not seeing, I'm genuinely open to it — but as written I don't think the trade is worth it.

The Content-Type check I'm cautious about rather than opposed to. It's what the RFC says, but it's fail-closed on a third party's behaviour, and TSAs and proxies vary. I'd want it tested against a few real authorities first.

One practical thing: this is written against the old layout, before the crate was split into ssign-core, so it doesn't apply cleanly any more — and it now conflicts with #2, which just landed and touches pe_hash. It'll need a rebase.

Could you split it up? The sign.rs fix on its own, then the hardening (bounds + atomic writes + token validation) as a second one. Those I can review and land quickly. The timestamp transport question is worth its own thread rather than riding along with the rest.

@Le-Syl21

Copy link
Copy Markdown
Owner

Closing this one — it's been superseded by the split I asked for.

#3 carries the sign.rs digest matching, and #4 carries the bounds check, the atomic writes and the timestamp token validation. That's everything I said I wanted to take, and both apply cleanly against the ssign-core layout, which this branch predates.

What's left here is the timestamp transport question — --allow-insecure-timestamp and the Content-Type enforcement. I'd still rather discuss that on its own than have it ride along with the hardening, so if you want to make the case for it, open an issue and we can argue it there properly. My reasoning is in the review above, and I meant it when I said I'm open to being wrong if there's an attack I'm not seeing.

Thanks for splitting it up — it made both halves reviewable.

@Le-Syl21 Le-Syl21 closed this Jul 22, 2026
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