[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAOcQRVU_vqzL77e=QkB24P5r+1=zkjoDzbNKk=HCkAGpDduR8Q@mail.gmail.com>
Date: Mon, 24 Feb 2025 18:18:56 +0100
From: Dmitry Belyavskiy <dbelyavs@...hat.com>
To: Solar Designer <solar@...nwall.com>
Cc: oss-security@...ts.openwall.com, Qualys Security Advisory <qsa@...lys.com>,
Jordy Zomer <jordy@...ing.systems>, Damien Miller <djm@...drot.org>
Subject: Re: MitM attack against OpenSSH's VerifyHostKeyDNS-enabled
client
On Mon, Feb 24, 2025 at 6:13 PM Solar Designer <solar@...nwall.com> wrote:
> Hi Dmitry,
>
> Thank you for taking a look at this.
>
> On Mon, Feb 24, 2025 at 05:57:13PM +0100, Dmitry Belyavskiy wrote:
> > On Sat, Feb 22, 2025 at 4:27???AM Solar Designer <solar@...nwall.com>
> wrote:
> > > +++ openssh-8.7p1-43.el9-tree.krb5-ssh_asprintf_append/auth-krb5.c
> > > 2025-02-21 03:37:13.106465704 +0000
> > > @@ -309,13 +309,14 @@ ssh_asprintf_append(char **dsc, const ch
> > > i = vasprintf(&src, fmt, ap);
> > > va_end(ap);
> > >
> > > - if (i == -1 || src == NULL)
> > > + if (i == -1)
> > > return -1;
> > >
> > > old = *dsc;
> > >
> > > i = asprintf(dsc, "%s%s", *dsc, src);
> > > - if (i == -1 || src == NULL) {
> > > + if (i == -1) {
> > > + *dsc = old;
> > > free(src);
> > > return -1;
> > > }
> > >
> > > This is in RH-added Kerberos support code. The issue was that if the
> > > second asprintf() call failed, it'd leave *dsc undefined, yet the
> caller
> > > of this function would free() memory via that pointer. In practice,
> > > glibc would either leave the pointer unchanged or reset it to NULL
> > > (varying by glibc version and specific error condition), both of which
> > > are safe to free(). Yet resetting "*dsc = old;" should be safer, and
> > > should avoid the memory leak that happens if *dsc got reset to NULL.
> > > That memory leak shouldn't have mattered anyway because it'd only occur
> > > when the process already has trouble allocating more memory here.
> > >
> > > The "src == NULL" checks are dropped because the first one shouldn't
> > > matter if asprintf() behaves correctly and wouldn't help if it does not
> > > (as src isn't initialized to NULL before the call), the second one
> > > is wrong (was probably meant to check *dsc, not src), and further code
> > > in this same source file relies on asprintf() return value anyway.
> >
> > I'm not sure that the check for the src == NULL should be removed at
> least
> > for the 1st branch.
>
> It's OK to keep it. This really shouldn't matter.
>
> > Unfortunately I came across implementations that caused segfault on
> passing
> > NULL pointers to sprintf-like functions.
>
> Of course, we shouldn't pass NULL pointers to sprintf-like functions.
> But if the first asprintf() call returns other than -1, the pointer is
> supposed to be non-NULL. And if we somehow don't trust asprintf()
> return value (even though it's standardized, unlike what happens to the
> pointer on error), then the check for NULL is insufficient because the
> pointer may as well remain uninitialized (formally it's undefined), so
> you'd need to start by "src = NULL;" before the first asprintf() call
> for this defensive programming to make sense. And the second "src ==
> NULL" check is redundant with the first (not reached if src is NULL).
>
Ah. Fair point, I missed that src is freshly allocated. Yes, you are
correct.
--
Dmitry Belyavskiy
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
