Re: Refactoring base64 encoding and decoding into a safer interface

> On 23 Jun 2019, at 15:25, Michael Paquier <michael@paquier.xyz> wrote:

> Attached is a refactoring patch for those interfaces, which introduces
> a set of overflow checks so as we cannot repeat errors of the past.

I’ve done a review of this submission.  The patch applies cleanly, and passes
make check, ssl/scram tests etc. There is enough documentation

I very much agree that functions operating on a buffer like this should have
the size of the buffer in order to safeguard against overflow, so +1 on the
general concept.

> Any thoughts?

A few small comments:

In src/common/scram-common.c there are a few instances like this.  Shouldn’t we
also free the result buffer in these cases?

+#ifdef FRONTEND
+               return NULL;
+#else

In the below passage, we leave the input buffer with a non-complete encoded
string.  Should we memset the buffer to zero to avoid the risk that code which
fails to check the returnvalue believes it has an encoded string?

+                       /*
+                        * Leave if there is an overflow in the area allocated for
+                        * the encoded string.
+                        */
+                       if ((p - dst + 4) > dstlen)
+                               return -1;

cheers ./daniel