On Wed, Mar 24, 2021 at 11:09:41AM +0100, Martin Vahlensieck wrote:
> Hi
>
> This fixes mangled output from the openssl(1) -debug option:
>
> Before:
> $ openssl aes-256-cbc -out test -debug
> BIO[0x9102a7e5ctrl(106) - FILE pointer
> BIO[0x9102a7e5ctrl return 1
> BIO[0x9102a801ctrl(108) - FILE pointer
> BIO[0x9102a801ctrl return 1
> ...
>
> After:
> $ openssl aes-256-cbc -out test -debug
> BIO[0x5770f81ce00]:ctrl(106) - FILE pointer
> BIO[0x5770f81ce00]:ctrl return 1
> BIO[0x5770f81c200]:ctrl(108) - FILE pointer
> BIO[0x5770f81c200]:ctrl return 1
> BIO[0x5770f81c200]:write(0,8) - FILE pointer
> ...
>
> The issue is that BIO_debug_callback(3) assumes that the pointer
> formatted with %p takes up 6 chars.
Thanks. Some comments inline.
>
> Best,
>
> Martin
>
> Index: bio/bio_cb.c
> ===================================================================
> RCS file: /cvs/src/lib/libcrypto/bio/bio_cb.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 bio_cb.c
> --- bio/bio_cb.c 8 Dec 2014 03:54:19 -0000 1.16
> +++ bio/bio_cb.c 24 Mar 2021 09:50:45 -0000
> @@ -70,15 +70,20 @@ BIO_debug_callback(BIO *bio, int cmd, co
> BIO *b;
> char buf[256];
> char *p;
> + int nbuf;
> long r = 1;
> size_t p_maxlen;
>
> if (BIO_CB_RETURN & cmd)
> r = ret;
>
> - snprintf(buf, sizeof buf, "BIO[%p]:", bio);
> - p = &(buf[14]);
> - p_maxlen = sizeof buf - 14;
> + nbuf = snprintf(buf, sizeof buf, "BIO[%p]:", bio);
> +
> + if (nbuf > sizeof buf)
> + nbuf = sizeof buf;
While this is correct for the upper bound, it looks odd. Especially in
combination with &buf[nbuf] which looks like an out-of-bounds access
(which happens to be ok since no actual access occurs later on). If
snprintf fails, this is an actual out-of-bounds access. I would try to
follow the canonical pattern: "if (nbuf < 0 || nbuf >= sizeof(buf))".
I'd spare the reader the checking and do:
if (nbuf < 0)
nbuf = 0; /* Ignore error; continue printing. */
if (nbuf >= sizeof(buf))
goto out;
Where "out" is a label after the switch.
> +
> + p = &(buf[nbuf]);
I would at least drop the parentheses and probably write "p = buf + nbuf;".
> + p_maxlen = sizeof buf - nbuf;
Please add parentheses around buf.
> switch (cmd) {
> case BIO_CB_FREE:
> snprintf(p, p_maxlen, "Free - %s\n", bio->method->name);
>
