#267 Shouldn't print arbitrary ID3 to terminal as-is

[Egmont Koblinger]

mpg123 seems to print the song's properties (title, artist etc.) from the ID3 fields unchanged, as-is, including unprintable and control characters. As such, maliciously crafted mp3 files or live streams can contain data that drive the terminal emulator in unexpected ways.

Control codes, escape sequences might for example (the list is far from complete):

• turn on various attributes, colors etc.;
• move the cursor, erase the screen, clear the scrollback etc.;
• trigger a response (e.g. cursor position) synthesized as if it was typed from the keyboard, getting in turn interpreted by "mpg123 -C";
• in some emulators (at some settings) change the profile, send desktop notifications, do a bunch of other things;
• in some emulators (at some settings) move, resize, iconify the terminal emulator window;
• in some emulators (at some settings) store arbitrary string in the clipboard, including dangerous ones such as "rm -rf ~\n".

mpg123 shouldn't just blindly output ID3 tags to the terminal. It should make sure to filter out control codes (most [or all] of the C0 ones [0x00..0x1F] plus all the C1 ones [0x80..0x9F in 8-bit mode, in some emulators also U+0080..U+009F encoded in UTF-8]), or probably filter out anything that's unprintable according to iswprint() or an analogous method, or just reject any text that's not entirely printable.

[Thomas Orgis]

Is see your point. Actually, it is not that new (src/metaprint.c):

/* We have no idea what encoding this is.
   So, to prevent mess up of our UTF-8 display, filter anything above ASCII.
   But in non-UTF-8 mode, we pray that the verbatim contents are meaningful
   to the user. Might filter non-printable characters, though. */

This is rather sloppy. A safe routine for filtering control characters
at least is needed.

[Thomas Orgis]

Yes, we need to add a filter for the next release.

The worst implications of not filtering are:

• trigger feedback to mpg123's terminal control
• overwrite clipboard, possibly causing command execution

right? Instead of blacklisting some ranges, I'd indeed rather have a whitelist check. The test with iswprint() looks promising.

[Thomas Orgis]

OK, so the basic problem would be about avoiding C0 and C1 characters (probably translating tabs to space beforehand … or just replace all of them by spaces). This is ignoring more esoteric text control characters in Unicode, which at least could mess up the structure of the display.

Windows codepages use C1 for printable characters. Is that relevant? Let's revisit what mpg123 actually does.

I avoided using the locale system so far. There is just the difference between UTF-8 mode and ASCII. I need to check …

    if(utf8env) mpg123_copy_string(source, dest);
    else        utf8_ascii(dest, source);

So … when converting to ASCII, I really mean 7 bits. No C1 there. There is still the use of ID3v1 data if v2 is missing. There, this happens:

            if(utf8env)
            {
                size_t i;
                for(i=0; i<dest->fill-1; ++i)
                if(dest->p[i] & 0x80) dest->p[i] = joker_symbol;
            }

Only for UTF-8 environment, stuff is limited to 7-bit ASCII again. Otherwise it is passed through verbatim, which is good for people using their favourite encoding and controlling their ID3v1 tags, bad for people being surprised by whatever is triggered otherwise. As the encoding of ID3v1 is not specified, we have to accept to be at east partly wrong. So I'd opt for two simple loops over the byte values,

1. Replace C0 by spaces, always for all strings
2. Also replace C1 for verbatim ID3v1 data, assuming the leftovers make sense to the user (ASCII plus some extra characters if the encoding happens to match). This might annoy the odd Windows user with CP1252 and the like, where the C1 range is normal characters.

That leaves C1 in UTF-8 strings. I'd like to work that out in raw UTF, not involving actual Unicode support code. It's not that complicated. A C1 character will be encoded like this:

                *p     = 0xc0 | (s[i]>>6);
                *(p+1) = 0x80 | (s[i] & 0x3f);

So … the leading byte is always 0xc2, followed by … the verbatim C1 character. We keep the last 6 bits, of which only 5 are used, and re-add the most significant bit again. So I need to get rid of the sequences 0xc280 to 0xc29f, right? So either map that to two spaces in-place or, better, to some
Unicode symbol that would also occupy two bytes in the string, keeping the character length unchanged.

Can you follow me so far or am I a madman? ;-)

[Egmont Koblinger]

I have no idea how these things go on Windows, so I'm only commenting for Unix.

I avoided using the locale system so far.

Maybe it's time to get familiar with it. The locale system can easily tell you for each character (note: not byte) whether that's a printable one according to the current locale. E.g. for the same single byte 0x80 it might respond "no" in ISO-8859-1 but "yes" in Windows-1250; whereas for Unicode it would operate on Unicode codepoints as entities, rather than single bytes.

There's no way to query the charset the terminal emulator itself actually runs in, but it's assumed that the locale is configured correctly, otherwise pretty much all the apps fall apart, and it's possible to query the locale's charset. Assuming a well-setup system, it takes you there.

Otherwise it is passed through verbatim

I'd argue that this is bad practice. I know that ID3v1 is broken by not having charset information (as well as its ridiculously low 30-byte limit). Still, you should preferably not print anything that isn't valid and printable in the terminal's encoding. As for escape sequence misuse, the main question is whether what you output is printable, but asking this question only makes sense once the data is already valid in the given encoding.

E.g. if the terminal is in UTF-8 (as today everyone's terminal should be, at least on Unix) but the ID3v1 tag is invalid UTF-8, you're probably better off making a guess of ISO-8859-1 as the tag's encoding and convert accordingly, rather than letting the terminal replace them by replacement symbols, or accidentally combining two of them together to a random glyph.

On the other hand, if the ID3v1 tag is valid in the terminal's charset (e.g. because the terminal's charset is an 8-bit one), it's probably okay to make the guess that the ID3v1 is to be interpreted according to this same charset. Still we wouldn't talk about just blindly copying the bytes: the locale system could tell you which characters are okay to send to the terminal and which are not (and this decision may depend on the exact choice of the 8-bit encoding).

Also replace C1 for verbatim [...] This might annoy the odd Windows user with CP1252 and the like

But why annoy them if there's a better solution? You don't need to filter out let's say 0x80 .. 0x9F if you know that they are printable. Use the locale system for that on Unix, or whatever equivalent surely exists on Windows.

I'd like to work that out in raw UTF, not involving actual Unicode support code. It's not that complicated.

It's not that complicated, but 1) how would you figure out without locale support whether the terminal's encoding is UTF-8 or what else, 2) how would you know which bytes or byte combinations to skip for which encoding, and 3) why do it manually when with proper locale/unicode support it's much cleaner, and already done for you by others?

So I need to get rid of the sequences 0xc280 to 0xc29f, right?

In UTF-8, the C1 representation of U+0080 .. U+009F is indeed 0xC2 0c80 .. 0xC2 0x9F.

So either map that to two spaces in-place or, better, to some Unicode symbol that would also occupy two bytes in the string, keeping the character length unchanged.

I think it's okay to simply drop them. Or, Unicode recommends the use of U+FFFD for symbols that aren't representable, like invalid encoding or so. Picking an exactly two-byte UTF-8 codepoint just because it's two-byte and you're lazy to handle if the string's length changes doesn't really sound the right attitude to me.

I generally disagree with the approach of doing these things "manually" rather than via a standard component that was designed for such tasks. But it's your call, I won't do code review or security audit or such. Thanks!

[Thomas Orgis]

Thanks for your elaborate answer. Actually, it's not correct that mpg123 doesn't use locales. It does inspect environment variables, calls setlocale(LC_CTYPE, "") and nl_langinfo(CODESET) to determine if the locale is UTF-8. My decision was, late as it was to add any encoding-awareness to mpg123, to have proper workings with UTF-8 output and otherwise just ASCII. I decided not to bother with conversions to differing 8-bit codepages. This is error-prone work that I did and still do not see much benefit in.

To put it into perspective: I started working on mpg123 in 2006. Any dealing with encodings started after that, e.g. conversion of everything to UTF-8 for ID3v2 data in libmpg123 API. Whoever doesn't use UTF-8 for terminal output since then, should be happy to still get most of the information by the downgraded 7-bit ASCII printout.

I don't remember a complaint that metadata printout drops some higher characters for non-UTF-8.

As a Linux user who likes to run things compiled from sources, I also learned to detest dependencies on ICU, which changes it's soname very often, breaking existing binaries. But I realize now that I don't need that fancy Unicode support to more properly handle things like the proper display width. I think I should get something going using iswprint() and wcswidth(), although I am dismayed by the existence of the Ambiguous Width characters. Now we got Unicode to represent all languages in one encoding, and still the display width of the characters depends on the environment, actually a preferene of the user, possibly, that the programs can guess about.

This means another round of conversion, copying and transforming characters … where most of the times, just a simple write of bytes is the correct thing (MP3 metadata usually being English song/artist titles, even for non-English artists).

People expecting their cp12xx data intact from ID3v1 probably will get a command-line switch to enforce that, otherwise dropping C0 and C1 control chars is the safe thing to do, IMHO. I doubt that a check for valid UTF-8 in id3v2 is worth it. Given the low amount of characters, that will quickly be inconsistent on a set of files. Do you know of Unicode-aware sofware that does not produce ID3v2 tags (unless you force it, perhaps)? It could make sense to try to interpret it as latin1 and convert to UTF-8 in an Unicode environment, though.

[Egmont Koblinger]

Actually, it's not correct that mpg123 doesn't use locales. It does inspect environment variables, calls setlocale(LC_CTYPE, "") and nl_langinfo(CODESET) to determine if the locale is UTF-8.

Sounds much better :)

Do you know of Unicode-aware sofware that does not produce ID3v2 tags (unless you force it, perhaps)?

I'm not familiar with ID3 generators. Probably there's not any anymore. I, for one, have lots of old mp3s with ID3v1 only, but I don't mind if accented characters are broken there because I know they can't be fixed by design (and for me interpreting them in ISO-8859-1 would also sometimes give incorrect answer). I'll convert them to v2 one day.

(Speaking of which, v2's design of three or four possible ways to store the data (as far as I recall there's ISO-8859-1, UCS2-[L or B?]E and UTF-8 to choose from?) is also somewhat braindamaged and error-prone, as opposed to enforcing one particular Unicode-compatible encoding, but at least there is hope that everyone implements these modes correctly and then the end result is correct.)

although I am dismayed by the existence of the Ambiguous Width characters

Yeah that's a terrible thing to have. Your best bet is to assume whatever wcwidth()/wcswidth() gives you.

Also sometimes the (non-ambiguous width) changes, Unicode 9 changed the width of lots of glyphs. Now if the terminal uses a different Unicode version than the application running inside, the display falls apart.

And there's something even "better": the VS16 emoji variation selector. No matter if the preceding character is narrow or wide, it's supposed to turn that into wide. This is straight incompatible with having a wcswidth() that gives the sum of the wcwidth() of the individual Unicode codepoints. We (terminal emulator developers) have not yet figured out what the heck to do with it. :P

[Thomas Orgis]

Oh … while we're at it. I presume I should also filter control characters out of file names and URLs that mpg123 prints. Are other programs that paranoid? I guess they should.

[Egmont Koblinger]

Yes, that would be the best. "ls" and "find" sure filter them, and so do necessarily all the ncurses-based applications (e.g. interactive file managers).

[Thomas Orgis]

OK, after lots of headache and reading about the atrocities of cramming all the symbols of the world into software that would be much more happy with that 8th bit staying zero, I got the first step implemented: commit 4570 contains the changes to avoid nasty characters entering the terminal from ID3 or ICY metadata.

This for now assumes a POSIX environment with locales for the best support (guessing of printing width using iswprint()), but has fallback to raw filtering of C0 and C1 to limit damage if that system support is not present/disabled. In non-UTF-8 locales, things are reduced to 7 bit ASCII with some ??? for fun.

Reading up on what terminal emulators try to deal with regarding Unicode support frightens me. At some point, someone got carried away with what kind of information (emoji!!!!) should be present in a program designed to emulate a console built back when the concept of an 8th bit was advanced.

[Thomas Orgis]

Filtering of file names needs to be added ... there, I guess I need the complete locale roundtrip, not knowing what encoding the data is to begin with.

[Thomas Orgis]

So, with commit 4636 I hopefully wrapped up the work on this, I got lots of ugly code juggling with temporary strings now to filter those characters and deal with data with unspecified encoding. Perhaps you can have a look at some test cases of your own before I release. I'm trying to do that rather soon-ish, but probably in form of a release candidate first.