Skip to content

gmac: clear the cached message on init - #474

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11551
Open

yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11551

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Problem

wp_gmac_init() never cleared the cached message buffer. wolfSSL has no
streaming GMAC API, so wp_gmac_update() accumulates the whole message in
macCtx->data / macCtx->dataLen and wp_gmac_final() feeds it to the
one-shot wc_GmacUpdate(); only wp_gmac_free() and wp_gmac_dup() ever
touched that buffer. Re-initializing a used EVP_MAC_CTX — the documented way
to reuse one, and the natural GMAC pattern since every message needs a fresh IV
— therefore authenticated previous_message || new_message, reporting success
at every call. A wrong tag with no error.

OpenSSL's default provider resets on every init path (gmac_init() always
calls EVP_EncryptInit_ex()), and OpenSSL's evp_test re-inits and re-runs
every MAC KAT, so scripts/test-openssl.sh reproduces this today: all nine
GMAC entries in scripts/evp_test/evpmac_common.txt fail with TEST_MAC_ERR.

Same defect class as f_11550 (CMAC, PR #473); this is its GMAC sibling.

Fix (src/wp_gmac.c)

wp_gmac_init() discards the cached message once the running check passes and
before the parameters and key are applied, so every init starts a new message:

  • OPENSSL_clear_free() cleanses the plaintext and releases the
    allocation, keeping dataLen equal to the allocated size — the invariant
    wp_gmac_free() and wp_gmac_dup() both rely on for a correct cleanse and
    memdup.
  • data = NULL; dataLen = 0 so wp_gmac_update()'s next
    OPENSSL_realloc() starts clean.

Clearing in init covers both a reset after final and a reset mid-stream, so
wp_gmac_final() is unchanged. No re-keying is needed — wc_GmacUpdate() is a
one-shot over an already-keyed Gmac.

Closes f_11551.

Tests

test_gmac_reinit drives one EVP_MAC_CTX through init/update/final rounds
and an init mid-stream, re-supplying the IV on each init as evp_test does,
and compares every MAC against OpenSSL.

Verification

  • Build clean under -Werror.
  • Negative control fails before the fix — GMAC after reset doesn't match the first MAC — while the OpenSSL half of the same test passes. Clearing only in
    wp_gmac_final() instead fails the mid-stream check.
  • 234/234 unit tests pass; GMAC tests ASan + UBSan clean.
  • evp_test on evpmac_common.txt: 11 errors → 2. All nine GMAC failures
    clear; the two remaining CMAC failures are PR cmac: take a NULL key in init as a restart with the cached key #473.

Not in this PR: a zero-length EVP_MAC_update() on a context with nothing
buffered returns 0, which an init now produces on every reuse.
wp_gmac_update() has always done this on a fresh context; it is tracked
separately.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 26, 2026
Copilot AI lite review requested due to automatic review settings August 26, 2026 05:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a correctness bug in the GMAC implementation where re-initializing an EVP_MAC_CTX could unintentionally authenticate previous_message || new_message due to a stale cached message buffer. It aligns wolfProvider behavior with OpenSSL’s expected “init resets state” semantics and adds a regression test to cover both post-final and mid-stream re-init cases.

Changes:

  • Clear and free the cached GMAC message buffer during wp_gmac_init() so each init starts a fresh message.
  • Add a new unit test (test_gmac_reinit) that exercises re-init after EVP_MAC_final() and re-init mid-stream while reusing the cached key, comparing results against OpenSSL.
  • Register the new GMAC re-init test in the unit test harness.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/wp_gmac.c Clears cached message data in wp_gmac_init() to prevent stale-data MAC concatenation across re-inits.
test/test_gmac.c Adds test_gmac_reinit to validate correct behavior across GMAC context resets vs OpenSSL.
test/unit.h Declares the new GMAC re-init unit test.
test/unit.c Registers the new GMAC re-init unit test in the test case list.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #474

Scan targets checked: wolfprovider-bugs, wolfprovider-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

@ColtonWilley ColtonWilley added the ci:all PR OSP toggle: run all label Sep 29, 2026
- wp_gmac_init() releases macCtx->data with OPENSSL_clear_free() and
  resets data and dataLen once the running check passes, before the
  parameters and key are applied.
- test_gmac_reinit drives one EVP_MAC_CTX through init/update/final
  rounds plus an init mid-stream, re-supplying the IV on each init,
  and compares every MAC against OpenSSL.
- test/unit.c and test/unit.h register the new test case.

Issue: F-11551
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:all PR OSP toggle: run all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants