Pádraig Brady <[email protected]> writes:
> From: Ismail Ramzi <[email protected]>
>
> parse_ls_color stores color_indicator[C_LINK].string as a pointer into
> the color_buf buffer, then frees color_buf on the unparsable-value path.
> The "ln=target" check after the parse loop still read that freed buffer,
> e.g. with LS_COLORS='ln=target:x'. Skip it on the failure path, where
> color output is disabled anyway and the buffer is gone.
> * src/ls.c (parse_ls_color): Return after freeing color_buf on failure,
> so the color_indicator[C_LINK] check only runs when the buffer is live.
> * tests/ls/color-symlink-target.sh: New test.
> * tests/local.mk (all_tests): Add it.
> * NEWS: Mention the fix.
>
> Link: https://github.com/coreutils/coreutils/pull/352
> ---
> NEWS | 4 ++++
> src/ls.c | 1 +
> tests/local.mk | 1 +
> tests/ls/color-symlink-target.sh | 33 ++++++++++++++++++++++++++++++++
> 4 files changed, 39 insertions(+)
> create mode 100755 tests/ls/color-symlink-target.sh
Thankfully, it seems pretty harmless. The bad read is the here:
if (color_indicator[C_LINK].len == 6
&& !STRNCMP_LIT (color_indicator[C_LINK].string, "target"))
color_symlink_as_referent = true;
Where STRNCMP_LIT expands to:
strncmp (color_indicator[C_LINK].string, "target", 6)
and "color_indicator[C_LINK].string" is the just freed string that
previously contained a string constructed from the values parsed from
LS_COLORS.
But I agree it is worth applying before you make the release.
On a semi-related note, I've been a bit tempted to mess with the
--enable-single-binary code a bit to allow building a library with
ls_main, etc. That would make it possible to use LibFuzzer to try to
catch stuff like this. I expiremented a bit with AFL++, which seems
fine, but I suspect it is less efficient since it needs to execute a
process for each case.
Collin
