> On 17 Aug 2026, at 08:40, Grigorev Jurij <[email protected]> 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
