Skip to content

security: harden validation pipeline — boolean parsing, CN matching, hash algorithm policy, API guidance, payload handling, cache keys#200

Merged
JeromySt merged 8 commits intomainfrom
users/jstatia/security-fixes
Apr 30, 2026
Merged

security: harden validation pipeline — boolean parsing, CN matching, hash algorithm policy, API guidance, payload handling, cache keys#200
JeromySt merged 8 commits intomainfrom
users/jstatia/security-fixes

Conversation

@JeromySt
Copy link
Copy Markdown
Member

@JeromySt JeromySt commented Apr 30, 2026

Security Hardening

Addresses findings from an internal security review of the validation pipeline.

Fixes

GetOptionBool parsed value handling (df542f81)

  • Boolean flag parsing now honours the parsed value correctly
  • Regression tests added

X509CommonNameValidator equality check (16fc219a)

Indirect signature hash algorithm policy (b38aa6a1)

CoseSign1MessageValidator.None API guidance (60ff09e2)

  • XML documentation corrected to accurately describe behavior
  • Added [Obsolete] and [EditorBrowsable(Never)] annotations

GetCommand payload write ordering (fc7ebfe1)

  • File write now gated on successful validation result
  • Regression tests added

Certificate cache key strengthening (c74b7e79)

  • Cache key replaced with cryptographic digest
  • Eliminates theoretical collision risk in long-running processes

Each commit includes targeted regression tests.

JeromySt and others added 6 commits April 30, 2026 08:55
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JeromySt JeromySt changed the title security: fix 6 findings from agentic security review (3 HIGH, 3 MEDIUM) security: address ScanWorld agentic security review findings — 3 HIGH, 3 MEDIUM (0 CRITICAL) Apr 30, 2026
@JeromySt JeromySt changed the title security: address ScanWorld agentic security review findings — 3 HIGH, 3 MEDIUM (0 CRITICAL) security: harden validation pipeline — boolean parsing, CN matching, hash algorithm policy, API guidance, payload handling, cache keys Apr 30, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JeromySt
Copy link
Copy Markdown
Member Author

Fixed in 1294aa0 — solution builds no longer re-invoke the plugin projects from CoseSignTool.csproj. We now skip the BuildPlugins target during solution builds, which avoids the concurrent write race on CoseSignTool.AzureArtifactSigning.Plugin.runtimeconfig.json while preserving direct project builds/publish behavior.

Comment thread CoseSign1.Certificates/Local/Validators/X509CommonNameValidator.cs Outdated
…erand order

Addresses reviewer feedback: PR #132 intentionally adopted signtool.exe /n
substring matching, but the operands were inverted — pin.Contains(signer)
instead of signer.Contains(pin). This allowed CN='o' to match any pin
containing 'o'.

Fix:
- Correct operand order: signerCN.Contains(requiredName)
- Add 3-char minimum length on required CN to prevent trivially short matches
- XML docs updated to document signtool.exe /n semantics explicitly

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JeromySt
Copy link
Copy Markdown
Member Author

Addressed @elantiguamsft feedback on CN validator in 7f0bae0:

The original PR #132 intentionally adopted signtool.exe /n substring semantics, but the operand order was inverted — it checked if the pin contained the signer CN instead of the other way around. This meant a signer with CN='o' could match any pin containing 'o' (like 'Microsoft Corporation').

Fix: Corrected to signer-contains-pin order, added 3-char minimum on required CN, and documented signtool.exe /n semantics in XML remarks. Substring matching is preserved while closing the gap.

@JeromySt JeromySt merged commit 4fdee16 into main Apr 30, 2026
11 checks passed
@JeromySt JeromySt deleted the users/jstatia/security-fixes branch April 30, 2026 22:33
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.

3 participants