Re: ntp security update

[Kurt Roeckx <kurt@roeckx.be>]

On Mon, Oct 26, 2015 at 06:13:07AM +0900, Ben Hutchings wrote:
> On Sun, 2015-10-25 at 11:19 +0100, Kurt Roeckx wrote:
> > On Sun, Oct 25, 2015 at 01:30:18PM +0900, Ben Hutchings wrote:
> > > I've looked through the upstream repository for the patches that fix he
> > > recently announced issues.  Quite a few of them turned out not to apply
> > > to squeeze, or the newer stable releases, and I've updated the security
> > > tracker accordingly.
> > > 
> > > I backported the remaining fixes as best I can, and uploaded the source
> > > package to:
> > > https://people.debian.org/~benh/packages/squeeze-lts/
> > > 
> > > Would you be willing to review this package?
> > > 
> > > I noticed that you entirely reverted the upstream patch that was
> > > supposed to fix CVE-2015-7704 and -7705, and then applied a different
> > > fix for -7704.  I think this means -7705 isn't fixed in sid, though the
> > > security tracker currently says it is.  Who's right?
> > 
> > I can't seem to ge getting much information out of anything from
> > upstream.  Lots of things don't seem to be affecting the 4.2.6
> > version.
> >
> > From what I currently understand the following don't apply to the
> > 4.2.6 versions:
> > CVE-2015-5196
> [...]
> > So it seems they renamed CVE-2015-5196 to CVE-2015-7703.  Your
> > patch probably makes sense and I should get that fixed in jessie
> > and wheezy too.
> > 
> > I'm just wondering why you didn't move the T_Pidfile like upstream
> > did, that part seems to apply.
> 
> Not in squeeze; there aren't any separate parsing rules for local and
> remote.
> 
> > Your bug-2899.patch patch looks a little different.  You have:
> > @@ -2207,8 +2221,8 @@ crypto_bob(
> >        vp->sig = emalloc(sign_siglen);
> >        EVP_SignInit(&ctx, sign_digest);
> >        EVP_SignUpdate(&ctx, (u_char *)&vp->tstamp, 12);
> > -      EVP_SignUpdate(&ctx, vp->ptr, vallen);
> > -      if (EVP_SignFinal(&ctx, vp->sig, &vallen, sign_pkey))
> > +      EVP_SignUpdate(&ctx, vp->ptr, len);
> > +      if (EVP_SignFinal(&ctx, vp->sig, &len, sign_pkey))
> >                vp->siglen = htonl(sign_siglen);
> >        return (XEVNT_OK);
> >  }
> > 
> > The patch from upstream and the one from redhat has:
> > @@ -2214,9 +2228,9 @@ crypto_bob(
> >         vp->sig = emalloc(sign_siglen);
> >         EVP_SignInit(&ctx, sign_digest);
> >         EVP_SignUpdate(&ctx, (u_char *)&vp->tstamp, 12);
> > -       EVP_SignUpdate(&ctx, vp->ptr, vallen);
> > -       if (EVP_SignFinal(&ctx, vp->sig, &vallen, sign_pkey))
> > -               vp->siglen = htonl(sign_siglen);
> > +       EVP_SignUpdate(&ctx, vp->ptr, len);
> > +       if (EVP_SignFinal(&ctx, vp->sig, &len, sign_pkey))
> > +               vp->siglen = htonl(len);
> >         return (XEVNT_OK);
> >  }
> > 
> > 
> > As in, the htonl() call changes sign_siglen to len.
> 
> No, it changes vallen to len.  But in 4.2.6 vallen is ignored and the
> previously calculated sign_siglen is assumed to be correct.  I didn't
> want to change that.

So from the EVP_SignFinal manpage:
| The number of bytes of data written (i.e. the length of the
| signature) will be written to the integer at s, at most
| EVP_PKEY_size(pkey) bytes will be written.

That is, the signature can be shorter than the key, it depends on
the signature scheme.

And sign_siglen in both 4.2.6 and 4.2.8 is:
        sign_siglen = EVP_PKEY_size(sign_pkey);

So maybe the variable name is a little misleading, it's the size
of the key not the signature.


Kurt