Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)

Em dom., 14 de fev. de 2021 às 08:22, Michael Paquier <michael(at)paquier(dot)xyz>
escreveu:

> On Sat, Feb 13, 2021 at 09:33:48PM -0300, Ranier Vilela wrote:
> > Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier <
> michael(at)paquier(dot)xyz>
> > escreveu:
> >
> >> You are missing the point here. What you are proposing here would not
> >> be backpatched. However, reusing the same words as upthread, this has
> >> a cost in terms of *future* maintenance. In short, any *future*
> >> potential bug fix that would require to be backpatched in need of
> >> using this function or touching its area would result in a conflict.
> >
> > Ok. +1 for back-patching.
>
> Please take the time to read again my previous email.
>
> And also, please take the time to actually test patches you send,
> because the list of things getting broken is impressive. At least
> you make sure that the internals of cryptohash.c generate an error as
> they should because of those incorrect sizes :)
>
> git diff --check complains, in various places.
>
> @@ -330,7 +330,8 @@ SendBackupManifest(backup_manifest_info *manifest)
> * twice.
> */
> manifest->still_checksumming = false;
> - if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0)
> + if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf,
> +
> sizeof(checksumbuf) - 1) < 0)
> elog(ERROR, "failed to finalize checksum of backup
> manifest");
> This breaks backup manifests, due to an incorrect calculation.
>
Bad habits.
sizeof - 1, I use with strings.

> I think that in pg_checksum_final() we had better save the digest
> length in "retval" before calling pg_cryptohash_final(), and use it
> for the size passed down.
>
pg_checksum_final I would like to see it like this:

case CHECKSUM_TYPE_SHA224:
retval = PG_SHA224_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA256:
retval = PG_SHA256_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA384:
retval = PG_SHA384_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA512:
retval = PG_SHA512_DIGEST_LENGTH;
break;
default:
return -1;
}

if (pg_cryptohash_final(context->raw_context.c_sha2,
output, retval) < 0)
return -1;
pg_cryptohash_free(context->raw_context.c_sha2);

What do you think?

regards,
Ranier Vilela