Обсуждение: pg_cryptohash_final possible out-of-bounds access (per Coverity)

At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> Hi Hackers,
> 
> Per Coverity.
> 
> Coverity complaints about pg_cryptohash_final function.
> And I agree with Coverity, it's a bad design.
> Its allows this:
> 
> #define MY_RESULT_LENGTH 32
> 
> function pgtest(char * buffer, char * text) {
> pg_cryptohash_ctx *ctx;
> uint8 digest[MY_RESULT_LENGTH];
> 
> ctx = pg_cryptohash_create(PG_SHA512);
> pg_cryptohash_init(ctx);
> pg_cryptohash_update(ctx, (uint8 *) buffer, text);
> pg_cryptohash_final(ctx, digest); // <--  CID 1446240 (#1 of 1):
> Out-of-bounds access (OVERRUN)
> pg_cryptohash_free(ctx);
> return
> }
>
> Attached has a patch with suggestions to make things better.

I'm not sure about the details, but it looks like broken.

make complains for inconsistent prototypes abd cryptohahs.c and sha1.c
doesn't seem to agree on its interface.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 10 Feb 2021 12:13:44 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> > Hi Hackers,
> > 
> > Per Coverity.
> > 
> > Coverity complaints about pg_cryptohash_final function.
> > And I agree with Coverity, it's a bad design.
> > Its allows this:
> > 
> > #define MY_RESULT_LENGTH 32
> > 
> > function pgtest(char * buffer, char * text) {
> > pg_cryptohash_ctx *ctx;
> > uint8 digest[MY_RESULT_LENGTH];
> > 
> > ctx = pg_cryptohash_create(PG_SHA512);
> > pg_cryptohash_init(ctx);
> > pg_cryptohash_update(ctx, (uint8 *) buffer, text);
> > pg_cryptohash_final(ctx, digest); // <--  CID 1446240 (#1 of 1):
> > Out-of-bounds access (OVERRUN)
> > pg_cryptohash_free(ctx);
> > return
> > }
> >
> > Attached has a patch with suggestions to make things better.
> 
> I'm not sure about the details, but it looks like broken.
> 
> make complains for inconsistent prototypes abd cryptohahs.c and sha1.c
> doesn't seem to agree on its interface.

Sorry, my messages was broken.

make complains for inconsistent prototypes, and cryptohahs.c and
sha1.c don't seem to agree on the interface of pg_sha1_final.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> Hi Hackers,
> 
> Per Coverity.
> 
> Coverity complaints about pg_cryptohash_final function.
> And I agree with Coverity, it's a bad design.
> Its allows this:
> 
> #define MY_RESULT_LENGTH 32
> 
> function pgtest(char * buffer, char * text) {
> pg_cryptohash_ctx *ctx;
> uint8 digest[MY_RESULT_LENGTH];
> 
> ctx = pg_cryptohash_create(PG_SHA512);
> pg_cryptohash_init(ctx);
> pg_cryptohash_update(ctx, (uint8 *) buffer, text);
> pg_cryptohash_final(ctx, digest); // <--  CID 1446240 (#1 of 1):
> Out-of-bounds access (OVERRUN)
> pg_cryptohash_free(ctx);
> return
> }

It seems to me that the above just means the caller must provide a
digest buffer that fits the use. In the above example digest just must
be 64 byte.  If Coverity complains so, what should do for the
complaint is to fix the caller to provide a digest buffer of the
correct size.

Could you show the detailed context where Coverity complained?

> Attached has a patch with suggestions to make things better.

So it doesn't seem to me the right direction. Even if we are going to
make pg_cryptohash_final to take the buffer length, it should
error-out or assert-out if the length is too small rather than copy a
part of the digest bytes. (In short, it would only be assertion-use.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Feb 10, 2021 at 01:44:12PM +0900, Kyotaro Horiguchi wrote:
> It seems to me that the above just means the caller must provide a
> digest buffer that fits the use. In the above example digest just must
> be 64 byte.  If Coverity complains so, what should do for the
> complaint is to fix the caller to provide a digest buffer of the
> correct size.
>
> Could you show the detailed context where Coverity complained?

FWIW, the community Coverity instance is not complaining here, so
I have no idea what kind of configuration it uses to generate this
report.  Saying that, this is just the same idea as cfc40d3 for
base64.c and aef8948 for hex.c where we provide the length of the
result buffer to be able to control any overflow.  So that's a safety
belt to avoid a caller to do stupid things where he/she would
overwrite some memory with a buffer allocation with a size lower than
the size of the digest expected in the result generated.

> So it doesn't seem to me the right direction. Even if we are going to
> make pg_cryptohash_final to take the buffer length, it should
> error-out or assert-out if the length is too small rather than copy a
> part of the digest bytes. (In short, it would only be assertion-use.)

Yes, we could be more defensive here, and considering libpq I think
that this had better be an error rather than an assertion to remain on
the safe side.  The patch proposed is incomplete on several points:
- cryptohash_openssl.c is not touched, so this patch will fail to
compile with --with-ssl=openssl (or --with-openssl if you want).
- There is nothing actually checked in the final function.  As we
already know the size of the result digest, we just need to make sure
that the size of the output is at least the size of the digest, so we
can just add a check based on MD5_DIGEST_LENGTH and such.  There is no
need to touch the internal functions of MD5/SHA1/SHA2 for the
non-OpenSSL case.  For the OpenSSL case, and looking at digest.c in
the upstream code, we would need a similar check, as
EVP_DigestFinal_ex() would happily overwrite the area if the caller is
not careful (note that the third argument of the function reports the
number of bytes written, *after* the fact).

I don't see much the point to complicate scram_HMAC_final() and
scram_H() here, as well as the manipulations done for SCRAM_KEY_LEN in
scram-common.h.
--
Michael

On Wed, Feb 10, 2021 at 09:14:46AM -0300, Ranier Vilela wrote:
> It is necessary to correct the interfaces. To caller, inform the size of
> the buffer it created.

Well, Coverity likes nannyism, so each one of its reports is to take
with a pinch of salt, so there is no point to change something that
does not make sense just to please a static analyzer.  The context
of the code matters.

Now, the patch you sent has no need to be that complicated, and it
partially works while not actually solving at all the problem you are
trying to solve (nothing done for MD5 or OpenSSL).  Attached is an
example of what I finish with while poking at this issue. There is IMO
no point to touch the internals of SCRAM that all rely on the same
digest lengths for the proof generation with SHA256.

> I think it should be error-out, because the buffer can be malloc.

I don't understand what you mean here, but cryptohash[_openssl].c
should not issue an error directly, just return a status code that the
caller can consume to generate an error.
--
Michael

At Thu, 11 Feb 2021 19:55:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier <michael@paquier.xyz>
> escreveu:
>
> > On Wed, Feb 10, 2021 at 09:14:46AM -0300, Ranier Vilela wrote:
> > > It is necessary to correct the interfaces. To caller, inform the size of
> > > the buffer it created.
> >
> > Now, the patch you sent has no need to be that complicated, and it
> > partially works while not actually solving at all the problem you are
> > trying to solve (nothing done for MD5 or OpenSSL).  Attached is an
> > example of what I finish with while poking at this issue. There is IMO
> > no point to touch the internals of SCRAM that all rely on the same
> > digest lengths for the proof generation with SHA256.
> >
> Ok, I take a look at your patch and I have comments:
>
> 1. Looks missed contrib/pgcrypto.
> 2. scram_HMAC_final function still have a exchanged parameters,
>     which in the future may impair maintenance.

The v3 drops the changes of the uuid_ossp contrib.  I'm not sure the
change of scram_HMAC_final is needed.

In v2, int_md5_finish() calls pg_cryptohash_final() with
h->block_size(h) (64) but it should be h->result_size(h)
(16). int_sha1_finish() is wrong the same way. (and, v3 seems fixing
them in the wrong way.)

Although I don't oppose to make things defensive, I think the derived
interfaces should be defensive in the same extent if we do. Especially
the calls to the function in checksum_helper.c is just nullifying the
protection.

For now, we can actually protect from too-short buffers in the
following places.  pg_cryptohash_final receives the buffer length
irrelevant to the actual length in other places.

0/3 places in pgcrypto.
2/2 places in uuid-ossp.
1/1 place in auth-scram.c
1/1 place in backup_manifest.c
1/1 place in cryptohashfuncs.c
1/1 place in parse_manifest.c
0/4 places in checksum_helper.c
1/2 place in md5_common.c
2/4 places in scram-common.c (The two places are claimed not to need the protection.)

Total 9/19 places.  I think at least pg_checksum_final() should take
the buffer length.  I'm not sure about px_md_finish() and
hmac_md_finish()..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Feb 12, 2021 at 03:21:40PM +0900, Kyotaro Horiguchi wrote:
> The v3 drops the changes of the uuid_ossp contrib.  I'm not sure the
> change of scram_HMAC_final is needed.

Meaning that v3 would fail to compile uuid-ossp.  v3 also produces
compilation warnings in auth-scram.c.

> In v2, int_md5_finish() calls pg_cryptohash_final() with
> h->block_size(h) (64) but it should be h->result_size(h)
> (16). int_sha1_finish() is wrong the same way. (and, v3 seems fixing
> them in the wrong way.)

Right.  These should just use h->result_size(h), and not
h->block_size(h).

-extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx);
+extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result);
There is no point in this change.  You just make back-patching harder
while doing nothing about the problem at hand.

-   if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0)
+   if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf,
+                           PG_SHA256_DIGEST_LENGTH) < 0)
Here this could just use sizeof(checksumbuf)?  This pattern could be
used elsewhere as well, like in md5_common.c.

> Although I don't oppose to make things defensive, I think the derived
> interfaces should be defensive in the same extent if we do. Especially
> the calls to the function in checksum_helper.c is just nullifying the
> protection.

The checksum stuff just relies on PG_CHECKSUM_MAX_LENGTH and there are
already static assertions used as sanity checks, so I see little point
in adding a new argument that would be just PG_CHECKSUM_MAX_LENGTH.
This backup checksum code is already very specific, and it is not
intended for uses as generic as the cryptohash functions.  With such a
change, my guess is that it becomes really easy to miss that
pg_checksum_final() has to return the size of the digest result, and
not the maximum buffer size allocation.  Perhaps one thing this part
could do is just to save the digest length in a variable and use it
for retval and the third argument of pg_cryptohash_final(), but the
impact looks limited.

> Total 9/19 places.  I think at least pg_checksum_final() should take
> the buffer length.  I'm not sure about px_md_finish() and
> hmac_md_finish()..

I guess that you mean px_hmac_finish() for the second one.  The first
one is tied to passing down result_size() and down to the cryptohash
functoins, meaning that there is no need to take about it more than
that IMO.  The second one would be tied to the HMAC refactoring.  This
would be valuable in the case of pgcrypto when building with OpenSSL,
meaning that the code would go through the defenses put in place at
the PG level.
--
Michael

On Sat, Feb 13, 2021 at 05:37:32PM -0300, Ranier Vilela wrote:
> IMO there is no necessity in back-patching.

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.
This changes makes no sense.
--
Michael

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@paquier.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.

@@ -78,7 +78,8 @@ pg_md5_hash(const void *buff, size_t len, char *hexsum)
     if (pg_cryptohash_init(ctx) < 0 ||
         pg_cryptohash_update(ctx, buff, len) < 0 ||
-        pg_cryptohash_final(ctx, sum) < 0)
+        pg_cryptohash_final(ctx, sum,
+                            sizeof(sum) - 1) < 0)
This one breaks MD5 hashing, due to an incorrect size calculation,
again.

@@ -51,7 +51,8 @@ scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen)
             return -1;
         if (pg_cryptohash_init(sha256_ctx) < 0 ||
             pg_cryptohash_update(sha256_ctx, key, keylen) < 0 ||
-            pg_cryptohash_final(sha256_ctx, keybuf) < 0)
+            pg_cryptohash_final(sha256_ctx, keybuf,
+                                sizeof(keybuf) - 1) < 0)
[...]
-    if (pg_cryptohash_final(ctx->sha256ctx, h) < 0)
+    if (pg_cryptohash_final(ctx->sha256ctx, h,
+                            sizeof(h) - 1) < 0)
This breaks SCRAM authentication, for the same reason.  In three
places.

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.

contrib/uuid-ossp/ fails to compile.

contrib/pgcrypto/ fix is incorrect, requiring h->result_size(h) in
three places.

I think that as a whole we should try to minimize the number of times
we use any DIGEST_LENGTH variable, relying a maximum on sizeof().
This v4 is a mixed bad of that.  Once you switch to that, there is an
interesting result with uuid-ossp, where you can notice that there is
a match between the size of dce_uuid_t and MD5_DIGEST_LENGTH.

No need to send a new patch, the attached taking care of those
issues, and it is correctly indented.  I'll just look at that again
tomorrow, it is already late here.
--
Michael

On Sun, Feb 14, 2021 at 11:39:47AM -0300, Ranier Vilela wrote:
> What do you think?

That's not a good idea for two reasons:
1) There is CRC32 to worry about, which relies on a different logic.
2) It would become easier to miss the new option as compilation would
not warn anymore if a new checksum type is added.

I have reviewed my patch this morning, tweaked a comment, and applied
it.
--
Michael