Re: null iv parameter passed to combo_init()

On Sat, Jan 8, 2022 at 7:11 PM Noah Misch <noah(at)leadboat(dot)com> wrote:

> On Sat, Jan 08, 2022 at 06:52:14PM -0800, Zhihong Yu wrote:
> > On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > On Fri, Jan 07, 2022 at 04:32:01PM -0800, Zhihong Yu wrote:
>
> > > I agree it's time to fix cases like this, given
> > > https://postgr.es/m/flat/20200904023648(dot)GB3426768(at)rfd(dot)leadboat(dot)com(dot)
> However,
> > > it should be one patch fixing all (or at least many) of them.
>
> > > > --- a/contrib/pgcrypto/px.c
> > > > +++ b/contrib/pgcrypto/px.c
> > > > @@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key,
> > > unsigned klen,
> > > > if (ivs > 0)
> > > > {
> > > > ivbuf = palloc0(ivs);
> > > > - if (ivlen > ivs)
> > > > - memcpy(ivbuf, iv, ivs);
> > > > - else
> > > > - memcpy(ivbuf, iv, ivlen);
> > > > + if (iv != NULL)
> > > > + {
> > > > + if (ivlen > ivs)
> > > > + memcpy(ivbuf, iv, ivs);
> > > > + else
> > > > + memcpy(ivbuf, iv, ivlen);
> > > > + }
> > > > }
> > >
> > > If someone were to pass NULL iv with nonzero ivlen, that will silently
> > >
> > Hi,
> > If iv is NULL, none of the memcpy() would be called (based on my patch).
> > Can you elaborate your suggestion in more detail ?
>
> On further thought, I would write it this way:
>
> --- a/contrib/pgcrypto/px.c
> +++ b/contrib/pgcrypto/px.c
> @@ -202,3 +202,3 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned
> klen,
> memcpy(ivbuf, iv, ivs);
> - else
> + else if (ivlen != 0)
> memcpy(ivbuf, iv, ivlen);
>
> That helps in two ways. First, if someone passes iv==NULL and ivlen!=0, my
> version will tend to crash, but yours will treat that like ivlen==0. Since
> this would be a programming error, crashing is better. Second, a compiler
> can
> opt to omit the "ivlen != 0" test from the generated assembly, because the
> compiler can know that memcpy(any_value_a, any_value_b, 0) is a no-op.
>

Hi,
Updated patch is attached.

>
> > Since the referenced email was old, line numbers have changed.
> > It would be nice if an up-to-date list is provided in case more places
> > should be changed.
>
> To check whether you've gotten them all, configure with CC='gcc
> -fsanitize=undefined -fsanitize-undefined-trap-on-error' and run
> check-world.
>