Skip to content

prototype(azure_execution_binding): bind HCLAkPub exponent e and key size, reject duplicate JSON keys, and harden _secure_boot_state and authenticode_sha256 - #620

Open
drk-ram wants to merge 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-azure-execution-binding-613
Open

drk-ram wants to merge 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-azure-execution-binding-613

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

Closes #613. Current source head: 5ab7fbc1478c4b702e2190130c68deb376ca9099.

prototype/azure_execution_binding.py rejects duplicate HCL runtime JSON keys and validates the RSA HCLAkPub JWK: no private/CRT parameters, canonical unpadded minimal Base64urlUInt n and e, a modulus of at least 2048 bits, exponent 65537, and equality with the supplied AK public key.

_secure_boot_state checks event byte types, exact variable framing, and UTF-16LE decoding. authenticode_sha256 checks PE header/section bounds and requires a present certificate table to be outside the headers, terminal in the file, and disjoint from hashed sections. Malformed PE inputs raise ValueError("malformed PE image").

The latest review's missing independent certificate-in-headers regression is already corrected: a 512-byte image with no sections and a 256-byte certificate table at offset 256 isolates cert_offset < headers_size, without an overlapping-section check masking it. No new production change was needed during this follow-through.

The latest review asked for a test that isolates the certificate-table/header boundary. The current fixture uses the suggested zero-section shape. This description refreshes the stale test count to the current-head rerun.

Type of change

  • Editorial (typo, link fix, clarification: no normative effect)
  • Non-breaking spec change (new optional field, new platform profile, informative addition)
  • Breaking spec change (requires 14-day comment period and Project Lead sign-off)
  • Schema change
  • Example addition

Prototype implementation and regression-test fix; no normative specification or schema edit.

Spec section

None. Production changes are confined to prototype/azure_execution_binding.py.

Tests for a fix

tests/test_execution_appraisal_and_azure_binding_hardening.py covers duplicate JSON keys, RSA key type/exponent/encoding/modulus/binding, SecureBoot framing/decoding, and PE bounds. test_authenticode_sha256_rejects_each_bounds_clause_alone[cert_in_headers] uses the zero-section 512-byte fixture to exercise the previously unisolated header boundary. Other cases cover a section table beyond headers, headers beyond EOF, a certificate table beyond or before EOF, a nonzero certificate offset with zero size, and overlapping sections.

Rerun on this head with Python 3.11.9 and this checkout's src and conformance/src in PYTHONPATH:

python -m pytest tests\test_execution_appraisal_and_azure_binding_hardening.py tests\test_azure_execution_binding.py -q --tb=short -> 37 passed, 14 skipped.

This rerun is software regression evidence, not live hardware evidence or a new claim of independently measured mutation coverage. Current-head hosted CI, Conformance CI, Review bar, and CodeQL runs are successful. The changes-requested review and independent maintainer-approval requirement still need re-review.

Changelog entry

Checklist

  • DCO sign-off on all commits (git commit -s)
  • Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text
  • Backward compatibility statement included (for breaking changes)

@drk-ram
drk-ram requested review from a team, lywinged and rajnisht7 as code owners October 23, 2026 03:55
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label Oct 23, 2026

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 1d772c0. Checked canonical RSA integer decoding, the single RSA HCLAkPub selection, exponent/modulus and supplied-key matching, duplicate runtime JSON rejection, secure-boot decoding and PE bounds. Focused local suites: 29 passed, 14 skipped; Ruff and diff checks passed. The skipped cases need the absent pinned WCM/cloud capture snapshot; the passing mocked authentication cases are unit boundary evidence, not live vTPM or hardware-attestation evidence. Current hosted CI/conformance/review-bar checks are successful. No blocking finding in this bounded change.

@rajnisht7 rajnisht7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the overall changes looks good, the duplicate-key rejection, _b64url_uint + full (n, e) binding, and the SecureBoot framing/UTF-16 checks look solid

One gap in authenticode_sha256 still needs fixing before this is complete:
The new bounds checks reject some bad certificate tables, but they still accept:

  • certificate table overlapping a section
  • certificate table that is in-bounds but not at EOF (cert_offset + cert_size != len(image))
  • non-zero cert_offset with cert_size == 0

then the code does end = len(image) - cert_size, which only matches the declared range when the table is terminal so a malformed PE can pass the new validation and still get the wrong exclusion window, either enforce the terminal placement invariant or exclude the actual [cert_offset, cert_offset + cert_size) range, reject section/cert overlap, and add regression tests for those three cases

also, the PR description and checklist still mention CHANGELOG + a few items (EV_SEPARATOR, Enabled integers, ak_pub_sha256) that aren’t in this head worth cleaning up so the description matches the diff.

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes. Reviewed at d33ab87 on c5d49a5.

rajnisht7's latest review asks for authenticode_sha256 to refuse three certificate-table inputs it still digests, with regression tests, and for the description to match the diff. One more thing is missing that CONTRIBUTING's "Before a pull request is reviewed" asks for: a test for each rejection, failing when that rejection is switched off. Three clauses in authenticode_sha256 have none: table + 40 * sections > headers_size, headers_size > len(image) and cert_offset + cert_size > len(image). Removing any one of them leaves the whole suite passing, because their test inputs are refused anyway, in order, by the struct.error catch, pointer < hashed and pointer + size > end. The first three cases below are each refused only by their own clause, and the last two catch a narrower bound, pointer against headers_size or pointer + size against len(image), which passes all 15 new tests. With any one clause removed, or either narrower bound in its place, exactly one case fails; against main's module all five do:

@pytest.mark.parametrize(
    "case",
    [
        "table_past_headers",
        "headers_past_eof",
        "cert_past_eof",
        "section_in_section",
        "section_in_cert",
    ],
)
def test_authenticode_sha256_rejects_each_bounds_clause_alone(case: str) -> None:
    plain, _ = _build_pe_image(magic=0x20B)
    signed, _ = _build_pe_image(magic=0x20B, cert_bytes=b"CERT" * 8)
    opt, table = 0x40 + 24, 0x40 + 24 + 240
    image = bytearray(signed if case in ("cert_past_eof", "section_in_cert") else plain)
    if case == "table_past_headers":
        struct.pack_into("<H", image, 0x40 + 6, 5)
    elif case == "headers_past_eof":
        struct.pack_into("<H", image, 0x40 + 6, 0)
        struct.pack_into("<I", image, opt + 60, len(plain) + 64)
    elif case == "cert_past_eof":
        struct.pack_into("<II", image, opt + 144, len(signed) - 8, 32)
    elif case == "section_in_section":
        struct.pack_into("<H", image, 0x40 + 6, 2)
        struct.pack_into("<II", image, table + 56, 32, 544)
    else:
        struct.pack_into("<II", image, table + 16, 80, 512)
    with pytest.raises(ValueError, match="^malformed PE image$"):
        x.authenticode_sha256(bytes(image))

I will review the rest once these are in.

CI at d33ab87 failed in the portability step for a reason this pull request did not cause: main failed the same step at da69b31, before #624, and passes at dcca4e0.


Generated by Claude Code

@drk-ram

drk-ram commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @rajnisht7 and @lywinged! Addressed all points in 51d7175 (merged with origin/main at e9ab302):

  1. Terminal certificate-table & zero-size checks in authenticode_sha256:
    • Updated the certificate-table check to if (cert_offset == 0) != (cert_size == 0) or (cert_offset and (cert_offset < headers_size or cert_offset + cert_size != len(image))): raise ValueError("malformed PE image"), rejecting non-zero cert_offset with cert_size == 0 as well as non-terminal certificate tables (cert_offset + cert_size < len(image) or > len(image)).
    • Updated cert_in_headers in test_authenticode_sha256_rejects_invalid_certificate_table_bounds to use size = len(pe64) - 256 so cert_offset < headers_size is tested in isolation, and added the 7-case test_authenticode_sha256_rejects_each_bounds_clause_alone (table_past_headers, headers_past_eof, cert_past_eof, cert_before_eof, nonzero_offset_zero_size, section_in_section, section_in_cert).
  2. PR description & checklist:
    • Updated the PR description and checklist to match the exact diff on this branch.

@drk-ram
drk-ram force-pushed the drk-ram-fix-azure-execution-binding-613 branch from 51d7175 to abcb526 Compare October 29, 2026 17:13

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes. Reviewed at abcb526 on e9ab302.

What the first round asked for is in. The three certificate-table inputs rajnisht7 named are refused: a table overlapping a section, a table in bounds but not at the end of the file, and a non-zero offset with a zero size. The three bounds clauses that had no test, table + 40 * sections > headers_size, headers_size > len(image) and the certificate end against len(image), now have one each; the two narrower section bounds, pointer against headers_size and pointer + size against len(image), which passed the module's 15 test functions at d33ab87, now fail section_in_section and section_in_cert; and the description matches the diff. abcb526 carries the same tree as 51d7175, remade as one commit on e9ab302.

The one thing. One clause has no test that fails when it is switched off: cert_offset < headers_size. Removing it leaves every test passing. The case meant to pin it, cert_in_headers, puts a table of len(pe64) - 256 bytes at offset 256 of a one-section image, and without the clause the section check refuses the same image, since end becomes 256 and the section starts at 512. At d33ab87 the same case, with a 32-byte table at offset 256, pinned the clause: removing it there fails the test. The push changed that case's size to reach the end of the file, which the new != clause requires, and the clause lost its test. CONTRIBUTING asks for a test per rejection that fails when the rejection is switched off, so this wants a case only that clause refuses. One such: a 512-byte image whose headers run to its end, with no sections and a 256-byte certificate table at offset 256. This head says malformed PE image for it, main returns a digest, and so does this head without the clause.

What I checked rather than read:

  • CI's commands, run here on 3.11 and 3.12: ruff, the dash check and mypy clean; the suite at 5242 passed and 98 skipped on both; the replay experiment, the two LC_ALL=C PYTHONUTF8=0 runs, the PYTHONOPTIMIZE=2 command and the two --help calls pass.
  • Between d33ab87 and this head the module changes in one hunk, the certificate-table condition; authenticate_ak and _secure_boot_state, measured in the first round, are byte for byte the same.
  • Nine other mutations of the bounds checks, one at a time, each fail at least one test: each of the three header bounds removed, the zero-pair check removed, != relaxed to >, each section clause removed, and each section bound narrowed as above.
  • authenticode_sha256 gives the same digest here and on main for the 72 .exe and .dll files on this machine, none of which carries a certificate table. The 474-byte vmlinuz fixture under the Go source trees here, which does, is refused here and digested on main; the refusal is right, since its headers claim 512 bytes.
  • tools/review_bar.py reports 22 changed test cases failing on behaviour at the merge base.

Generated by Claude Code

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-head review follow-up at abcb526: I prepared the test-only adoption companion dinakarjs#6, signed commit 5b144e5, for Lywinged's remaining cert_offset < headers_size regression request.

The no-section 512-byte control hashes successfully; a terminal certificate table overlapping its headers is refused. Removing only that predicate makes the new test fail with DID NOT RAISE. All 23 cases in the hardening test module pass normally and under -O; changed-file Ruff, dash and whitespace checks pass. Runtime code is unchanged; no fresh full-suite/OS/hardware or independent implementation approval is claimed. @drk-ram please cherry-pick into this existing PR and ask @lywinged to disposition the current-head request. This is a contribution/adoption COMMENT, not self-approval.

…size, reject duplicate JSON keys, and harden _secure_boot_state and authenticode_sha256

Closes agentrust-io#613

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram

drk-ram commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@lywinged Rebased onto origin/main (b938790) in 5ab7fbc and updated cert_in_headers (and added it to test_authenticode_sha256_rejects_each_bounds_clause_alone) using a 512-byte zero-section image with a 256-byte certificate table at offset 256, so removing cert_offset < headers_size alone fails the test (23 passed).

@drk-ram

drk-ram commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

The current head 5ab7fbc uses the requested independent certificate-in-headers fixture: 512 bytes, zero sections, certificate offset 256 and size 256. The focused Azure binding suites now report 37 passed and 14 skipped. I updated the description with current-head evidence. Please re-review the remaining changes request.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:MEDIUM Contributor check flagged MEDIUM risk

Projects

None yet

4 participants