Re: Re: [PATCH] Fix use-after-free after failed pg_checksum_init

Hi Yuriy,

I reviewed and tested v2 (CF 7257).

v2 no longer applies to master: t/013 is now taken by 013_summary_order, so
the meson.build hunk conflicts. The code hunks apply unchanged; I renamed
the test to t/014_checksum_helper to run it.

Linux aarch64, autoconf, --enable-cassert --enable-debug
--enable-tap-tests, no warnings:

- The new test passes, and the pg_combinebackup suite passes.
- With master's checksum_helper.c and the rest of v2 it fails as you
described ("init failure did not clear context", "final failure did not
free context once", for SHA224 to SHA512), so it catches the bug.
- make check builds test_checksum_helper from a clean tree, and make clean
removes it. I did not try the meson build.

The checksum_helper.c change matches Daniel's suggestion, and after the
patch every caller of pg_checksum_init() and pg_checksum_final() in the
tree checks the result (pg_combinebackup was the only one that didn't).

Not for this patch, but related: in pg_verifybackup, if
pg_checksum_update() fails, both astreamer_verify.c and
verify_file_checksum() stop verifying without calling pg_checksum_final(),
so the SHA context is never freed. It is only a leak on an error path that
is already reported.

Regards,
Sai Pranav

On Wed, Nov 04, 2026 05:16 PM, Grigorev Jurij <ju(dot)grigorev(at)ftdata(dot)ru> wrote:

> Hi Daniel,
>
> Thanks for taking a look!
>
> I reworked pg_checksum_final() along the lines you suggested. The NONE and
> CRC32C cases now return directly from the switch, while the SHA cases only
> set
> the digest length. The common final/free/clear logic is after the switch,
> and I
> removed the now-redundant Assert. The patch also keeps the error checks
> at all
> fallible pg_checksum_init() and pg_checksum_final() calls in
> pg_combinebackup.
>
> For the test, I first looked at using the existing injection-point
> facility.
> Sadly it is implemented for backend processes, and pg_combinebackup is
> a frontend program. checksum_helper.c is compiled separately with
> FRONTEND, so
> I discovered that this facility cannot make pg_cryptohash_init() or
> pg_cryptohash_final()
> return -1 in the code path we need to test. The existing "error" callback
> would not
> model this either, because it raises ERROR instead of returning -1.
>
> Instead, my v2 follows the test harness pattern used by
> src/test/modules/test_escape:
> it is a small test-only frontend C executable, not installed, and is
> invoked by a TAP test.
> The difference is that our executable links the real checksum_helper.c
> against
> test implementations of the pg_cryptohash_* API instead of the production
> cryptohash provider. The linker therefore substitutes a deterministic
> mock at
> the API boundary while the code under test remains the real
> checksum_helper.c.
>
> The mock can make pg_cryptohash_init() or pg_cryptohash_final() return -1
> and
> counts calls to pg_cryptohash_free(). For SHA224, SHA256, SHA384, and
> SHA512,
> the test verifies the returned error, that the context is freed exactly
> once,
> and that c_sha2 is cleared. It also checks the corresponding invariants
> after
> successful finalization.
>
> As a negative control, I built the same test executable twice, changing
> only
> checksum_helper.c. With the original file it reports, among other
> failures:
> ```
> checksum type 2: init failure did not clear context
> checksum type 2: final failure did not free context once
> checksum type 3: final failure did not clear context
> checksum type 4: successful finalization did not clear context
> checksum type 5: final failure did not free context once
> ```
> Here types 2 through 5 are SHA224, SHA256, SHA384, and SHA512.
> The TAP result is:
> ```
> not ok 1 - checksum contexts are cleaned up after cryptohash failures
> Result: FAIL
> ```
>
> With the patched checksum_helper.c, the same TAP test produces:
> ```
> ok 1 - checksum contexts are cleaned up after cryptohash failures
> All tests successful.
> Result: PASS
> ```
>
> On back-patching, I agree that the risk/reward is less obvious. I left
> that
> decision separate for now. My v2 is against master.
>
> Best regards,
> Yuriy Grigoryev
> ________________________________________
> От: Daniel Gustafsson <daniel(at)yesql(dot)se>
> Отправлено: 24 августа 2026 г. 20:12:11
> Кому: Григорьев Юрий
> Копия: pgsql-hackers(at)lists(dot)postgresql(dot)org; Каныбеков Канатбек
> Тема: Re: [PATCH] Fix use-after-free after failed pg_checksum_init
>
> > On 17 Aug 2026, at 08:40, Grigorev Jurij <ju(dot)grigorev(at)ftdata(dot)ru> wrote:
> >
> > Hi there, hackers!
> >
> > pg_checksum_init() is documented to return -1 on failure. For the SHA
> types it first stores the requested type in the checksum context, then
> calls pg_cryptohash_create() / pg_cryptohash_init(). If create succeeds and
> init fails, it frees the SHA context but does not clear
> context->raw_context.c_sha2.
>
> Thanks for the report!
>
> > basebackup and pg_verifybackup check that return value. pg_combinebackup
> does not: write_backup_label(), create_manifest_writer(), reconstruct, and
> the copy path in pg_combinebackup.c all ignore a failed pg_checksum_init()
> and later call pg_checksum_update() or pg_checksum_final() with the
> dangling pointer.
>
> Agreed, it seems quite reasonable to fail here like basebackup does.
>
> > create_manifest_writer() always uses CHECKSUM_TYPE_SHA256 for the
> manifest itself, so this is not limited to --manifest-checksums=sha256.
> > The use-after-free is reachable in a frontend OpenSSL build when
> EVP_DigestInit_ex() fails after a successful create.
>
> Reading the OpenSSL code, it's not a particularly likely - albeit possible
> -
> codepath.
>
> > The in-core cryptohash implementation does not fail after create, so a
> non-OpenSSL build does not hit this path.
>
> Such builds are also an extreme minority in production.
>
> > A related issue in pg_checksum_final(): if pg_cryptohash_final() fails,
> the function returns -1 without freeing the SHA context, and there is no
> separate cleanup API.
>
> case CHECKSUM_TYPE_SHA512:
> retval = PG_SHA512_DIGEST_LENGTH;
> if (pg_cryptohash_final(context->raw_context.c_sha2,
> output, retval) < 0)
> - return -1;
> + retval = -1;
> pg_cryptohash_free(context->raw_context.c_sha2);
> + context->raw_context.c_sha2 = NULL;
> + if (retval < 0)
> + return -1;
>
> I'm not sure there is much value in this much repetition in the switch.
> How
> about returning retval from CHECKSUM_TYPE_CRC32 and _TYPE_NONE, and for the
> _SHAXXX cases breaking out the logic to a single place at the end in a
> fallthrough? Something like the below untested sketch (which only looks
> at the
> last case as an example)?
>
> case CHECKSUM_TYPE_SHA512:
> retval = PG_SHA512_DIGEST_LENGTH;
> - if (pg_cryptohash_final(context->
> raw_context.c_sha2,
> -
> output, retval) < 0)
> - return -1;
> - pg_cryptohash_free(context->raw_context.c_sha2);
> break;
> }
>
> - Assert(retval <= PG_CHECKSUM_MAX_LENGTH);
> + if (pg_cryptohash_final(context->raw_context.c_sha2, output,
> retval) < 0)
> + retval = -1;
> + pg_cryptohash_free(context->raw_context.c_sha2);
> + context->raw_context.c_sha2;
> return retval;
>
> Repeating code is a good way of increasing the risk of missing to update
> all
> occurrences when a bug is found. The assertion redundant with the
> StaticAssertDecl calls added to the top of the function and can be removed
> anyways IMO.
>
> >
> > The attached patch is against master. It NULLs the pointer after free on
> init failure, always frees the SHA context in pg_checksum_final(), and
> checks pg_checksum_init() / pg_checksum_final() in pg_combinebackup.
>
>
> > I also think it should be back-patched to 17, where pg_combinebackup was
> added.
>
> Possibly. The risk/reward ratio for an error which is very unlikely to
> happen
> isn't favorable as backpatching always introduce some level of
> destabilization
> risk. You might be right that the value in catching this rare error,
> should it
> happen, is worth it.
>
> > We found this with Clang Static Analyzer. No new regression test is
> included, injecting a post-create cryptohash init failure would need a
> > mock that we do not have today.
>
> Have a look at injection point tests, they are very handy for testing this
> class of almost impossible to hit error paths.
>
> --
> Daniel Gustafsson
>
>