[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJvHH_RWLt8Y4aB6fwLtHymsjp8yrbwEdJ20+jZjNYYKmt=aag@mail.gmail.com>
Date: Fri, 14 Feb 2020 10:47:16 +0000
From: Ibrahim el-sayed <i.elsayed92@...il.com>
To: oss-security@...ts.openwall.com
Subject: Re: Potential regression and/or incomplete fix for CVE-2017-12762
Hi Brad,
Thank you very much for your reply. This actually clarifies everything :)
Ibrahim
On Tue, Feb 11, 2020 at 9:37 PM Brad Spengler <spender@...ecurity.net>
wrote:
> Hi Ibrahim,
>
> > I think it is incomplete and can lead to reading out of bound since it
> does
> > *not* check if the src buffer (p) in this case has 10 bytes at least. The
> > fix assumes p has 10 bytes and copies that into newname. The fix
> > uses strscpy (
> >
> https://github.com/torvalds/linux/blob/cc12071ff39060fc2e47c58b43e249fe0d0061ee/lib/string.c#L180
> )
> > which
> > based on its code it starts copying from count and decrements to zero.
>
> This isn't correct. There is a 'count' variable that decrements to zero,
> yes,
> but that's not what is used to index the strings. 'res' is used for that,
> and
> it increments from zero as you'd expect.
>
> Regarding OOB, there is the read-by-word trickery, but it's safe and won't
> trip up KASAN for the max 7 bytes it can end up reading past bounds, and
> won't
> in any instance cross a page boundary.
>
> Since 'param' is guaranteed to be NUL-terminated from the fix (the
> isdn_common.c
> change), so is 'p', so the strscpy is fine here, especially since the later
> use of the buffer (coming from the netdev netname) uses strlen as the
> length
> for the buffer copy to userland here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/isdn/i4l/isdn_common.c?id=9f5af546e6acc30f075828cb58c7f09665033967#n1385
> So strscpy_pad() wasn't necessary in this instance, despite it often being
> needed for kernel work (and the better defensive choice, unless performance
> is critical and you can guarantee the remaining part of the buffer never
> gets
> copied to userland or used in any way).
>
> > ## Regression
> > I looked quickly into latest version for the kernel v3.16.81 and it seems
> > that the patch was probably reverted as the code matches exactly to the
> > vulnerable version to the CVE (
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/isdn/i4l/isdn_net.c?id=v3.16.81#n2646
> > )
> > Not sure if the fix was reworked but wanted to surface that issue as well
>
> This wasn't due to a revert, the fix was just never backported to 3.16.
> Happens all the time. There's never a guarantee that just because a
> security fix is backported to some newer kernel version that it'll be
> backported to all affected versions. If the patch doesn't apply cleanly
> and no one fixes it up, it just never gets fixed.
>
> For this instance, you can confirm it by looking at the git log for that
> tree:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/drivers/isdn/i4l/isdn_net.c?h=linux-3.16.y
>
> The 3.16 kernel has a different maintainer than others listed on
> kernel.org:
> https://www.kernel.org/category/releases.html
> so there may be different critera for what's selected for backporting
> there.
>
> Thanks,
> -Brad
>
--
Regards
Ibrahim M. El-Sayed
Security Engineer
Website: https://www.ibrahim-elsayed.com
@ibrahim_mosaad
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.
