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

[Rob Landley]

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. :)

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. Not only would the input field be damaged, but I think the unused
portion of the string would overflow into the next field?

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?)

>> Rob

Still Rob