[Toybox] [PATCH] mkflags: fix a sscanf buffer off-by-one.

[enh]

On Sun, Feb 23, 2020 at 1:04 AM Rob Landley <rob at landley.net> wrote:
>
> On 2/22/20 4:47 PM, enh wrote:
> > On Sat, Feb 22, 2020 at 4:05 AM Rob Landley <rob at landley.net> wrote:
> >>
> >> On 2/21/20 11:10 AM, enh via Toybox wrote:
> >>> Sadly, the compilers don't even catch this common mistake if you use
> >>> sscanf_s(3). Luckily, ASan does.
> >>
> >> I haven't given the build-time stuff the same level of scrutiny the runtime
> >> stuff gets because it's not deployed on target so presumably not as exploitable.
> >> (And because it's not building with the full toybox lib/*.c infrastructure and
> >> is working with known fixed inputs, so there's a lot of half-assed "read into a
> >> static buffer" and such going on, which this is an instance of.)
> >>
> >> At least I bounds-checked it. :)
> >
> > yeah, that's what makes scanf's %s and friends such an "unsafe at any
> > speed" API.
>
> No more than memcpy and friends. The numbers just have to add up. :)

but "adding up" is hard for humans to get right because the number is
off by one because you need to leave one for santa^Wthe \0.

that off by one is the mistake that everyone -- myself included -- makes.

> The build stuff has always been a bit quick-and-dirty because sometime after 1.0
> I vaguely expect to rewrite it when I get around to doing a replacement kconfig.
> (By which time I hope to know what it should look like.)
>
> In the meantime, this is an internal build tool working on fixed inputs provided
> exclusively by the build system, none of which should be anywhere near that size
> limit. I put in a check largely for aesthetic reasons (not doing it was just
> _too_ ugly) but if anything supplies a string long enough to trigger the
> truncation, that's the _real_ bug.
>
> Keep in mind if the truncation ever did happen it would be its own kind of
> breakage.

indeed. another reason to recommend %ms for production code.

> Not only would the input field be damaged, but I think the unused
> portion of the string would overflow into the next field?

yeah, ASan even outputs a handy diagram showing that:

  This frame has 5 object(s):
    [32, 36) '<unknown>'
    [48, 52) '<unknown>'
    [64, 320) 'command' (line 157)
    [384, 1407) 'flags' (line 157)
    [1536, 2560) 'allflags' (line 157) <== Memory access at offset
1407 partially underflows this variable

but, yes, it's firing here because the scanf interceptor deliberately
always writes the maximum allowed string so you find issues during
testing rather than production.

> A 1023 byte optstr would wrap an 80 column line almost 13 times. We have a hard
> limit of 64 flags (long long bitfield); even with longopts that's a bit of a
> stretch. I believe the longest optstr we currently have is tar, at 351 bytes.
>
> The use-after-free is a real bug, which I should cycle back to tracking down
> after checking in a patch fuzz fix...
>
> > i usually encourage Android folks to use %ms etc instead and just let
> > libc allocate.
>
> Has posix caught up to that one yet? (I think the last time I checked, they were
> up to considering things introduced in 2003?)

surprisingly, yes, since issue 7:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fscanf.html
(search for 'm').

> >> Rob
>
> Still Rob