Re: Refactoring base64 encoding and decoding into a safer interface

> On 23 Jun 2019, at 15:25, Michael Paquier <michael(at)paquier(dot)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