ansi escape sequence injection in add-apt-repository

[Jason A. Donenfeld]

This was reported to oss-security and to <email address hidden>, but I figure I should make a real bug report, as otherwise it'll probably be missed. Original post from https://www.openwall.com/lists/oss-security/2020/08/03/1 follows below.

--

Hi,

I've found a rather low grade concern: I'm able to inject ANSI escape
sequences into PPA descriptions on Launchpad, and then have them
rendered by add-apt-repository *before* the user consents to actually
adding that repository. There might be some sort of trust barrier
issue with that. This could be used to clear the screen and imitate a
fresh bash prompt, upload files, dump the current screen to a file, or
other classic shenanigans, well chronicled in the archives of oss-sec.

PoC time -- I'm using this "feature" for good at the moment to
announce the deprecation in bold text of a PPA that I maintain:
https://data.zx2c4.com/add-apt-repository-ansi-injection.png

The proper fix to this is likely to do sanitization on the
add-apt-repository side.

Regards,
Jason

[Jason A. Donenfeld (zx2c4)]

Looks like this has come up before in other utilities and was fixed, such as https://bugs.launchpad.net/ubuntu/+source/base-files/+bug/1649352 .

[Seth Arnold (seth-arnold)]

Thanks Jason, please use CVE-2020-15709 for this issue.

[Launchpad Janitor (janitor)]

This bug was fixed in the package software-properties - 0.98.9.2

---------------
software-properties (0.98.9.2) focal-security; urgency=medium

  * SECURITY UPDATE: malicious repo could send ANSI sequences to terminal
    (LP: #1890286)
    - add-apt-repository: strip ANSI sequences from the description.
    - CVE-2020-15709

 -- Marc Deslauriers <email address hidden> Fri, 07 Aug 2020 09:15:34 -0400

[Launchpad Janitor (janitor)]

This bug was fixed in the package software-properties - 0.96.24.32.14

---------------
software-properties (0.96.24.32.14) bionic-security; urgency=medium

  * SECURITY UPDATE: malicious repo could send ANSI sequences to terminal
    (LP: #1890286)
    - add-apt-repository: strip ANSI sequences from the description.
    - CVE-2020-15709

 -- Marc Deslauriers <email address hidden> Fri, 07 Aug 2020 10:07:43 -0400

[Jason A. Donenfeld (zx2c4)]

I'm not convinced that really cuts it. Namely, from the diff:

- print(" %s" % (info["description"] or ""))
+ # strip ANSI escape sequences
+ description = re.sub(r"(\x9B|\x1B\[)[0-?]*[ -/]*[@-~]",
+ "", info["description"] or "")
+
+ print(" %s" % description)

There are sequences that don't get filtered by that. Aside from the usual things like \r or \b, it looks like https://man7.org/linux/man-pages/man4/console_codes.4.html lists a few codes that defy it too. While that diff above might be the "stackoverflow answer", it doesn't seem complete.

Instead, why not just adopt a whitelist policy? Only allow visible and space characters, or something like that.

[Marc Deslauriers (mdeslaur)]

Hi,

Could you elaborate which codes in that manpage you feel are dangerous and are actually implemented by the common terminals? The old screendump and window title codes were disabled long ago, I'm not sure any of the others are anything other than a nuisance.

[Jason A. Donenfeld (zx2c4)]

You might be right that the remaining ones that slip through your regex are mere "nuisance"s. But you know how those things go - one man's nuisance is another man's vuln. Some of those, anyhow, are implemented by the Linux console driver.

Why not just take the tried and true "safe" route, as implemented by vis(3)'s VIS_SAFE or similar? Otherwise it sounds like you're playing with a bit of fire.

Put differently, is there some legitimate use case of the ANSI escape characters that make you want to preserve some of their usage while disallowing other parts? If so, that would really surprise me.