Fix a shell command injection vulnerability in metadata

This has been reported by Alexandre Rebert in February 2013(!). The time to fix is terrible; luckily, the affected user base is likely to be very small.
2024-09-29 04:25:56 -04:00 · 2015-01-07 22:49:09 +01:00 · 2015-01-07 22:49:09 +01:00 · 6211bc8506
commit 6211bc8506
parent 538b497192
7 changed files with 138 additions and 97 deletions
--- a/28
+++ b/28
@ -1,6 +1,34 @@
 Changes in n.n.n, released on XXXX-XX-XX:

+ * This release contains a SECURITY FIX for a command injection vulnerability
+   that was found and reported by Alexandre Rebert:
+
+   The previous handling of metadata placeholders allowed for arbitrary shell
+   commands to be trivially injected and executed as the ezstream user, via
+   malicious media files.
+
+   This vulnerability depends on both a configuration using metadata
+   placeholders and the user streaming media files from untrusted sources
+   without noticing `commands` or $(commands) in artist or title fields.
+
+   While the group of actually affected users may be limited, all users are
+   advised to upgrade.
+
+ * This release requires users to ADJUST their CONFIGURATION:
+
+   To protect against the injection vulnerability above, metadata is now
+   properly quoted and escaped from the shell. This means that any extra
+   quoting must be removed from configuration files.
+
+   Remove all quoting from metadata placeholders in <encode/> and <decode/>
+   commands, e.g. replace "@M@" with @M@, and "@T@" with @T@, etc. Without
+   these changes, stream metadata will look both wrong and the injection
+   vulnerability may be re-introduced.
+
+   Configuration examples have been adjusted accordingly.
+
 * src/ezstream.c:
+   - [FIX]   Prevent certain characters from being interpreted by the shell.
   - [FIX]   Prevent ezstream from entering an infinite loop when stopping to
             send data to standard input. From gquintard.
             (Ticket #2045)
--- a/doc/ezstream.1.in.in
+++ b/doc/ezstream.1.in.in
@ -519,9 +519,17 @@ The main tool for handling metadata with
 .Nm
 is placeholders in decoder and encoder commands that are replaced with real
 content during runtime.
-The tricky part is that one of the placeholders has to be handled differently,
-depending on where the metadata comes from.
-This section will explain each possible scenario.
+.Pp
+.Em Note:
+To prevent malicious shell script in metadata
+.Pq such as artist and title tags
+from being executed, all metadata content is automatically enclosed in single
+quotes, with escaped single quote and backslash characters inbetween.
+To prevent this from causing unwanted side-effects
+.Pq or introducting security risk ,
+placeholders
+.Em must not
+be quoted any further.
 .Ss Metadata Placeholders
 .Bl -tag -width -Ds
 .It Sy @T@
--- a/examples/ezstream-file_template.xml
+++ b/examples/ezstream-file_template.xml
@ -40,19 +40,19 @@
        <encdec>
            <format>FLAC</format>
            <match>.flac</match>
-            <decode>flac -s -d --force-raw-format --sign=signed --endian=little -o - "@T@"</decode>
+            <decode>flac -s -d --force-raw-format --sign=signed --endian=little -o - @T@</decode>
        </encdec>
        <encdec>
            <format>MP3</format>
            <match>.mp3</match>
-            <decode>madplay -b 16 -R 44100 -S -o raw:- "@T@"</decode>
+            <decode>madplay -b 16 -R 44100 -S -o raw:- @T@</decode>
            <encode>lame --preset cbr 128 -r -s 44.1 --bitwidth 16 - -</encode>
        </encdec>
        <encdec>
            <format>VORBIS</format>
            <match>.ogg</match>
-            <decode>oggdec -R -b 16 -e 0 -s 1 -o - "@T@"</decode>
-            <encode>oggenc -r -B 16 -C 2 -R 44100 --raw-endianness 0 -q 1.5 -t "@M@" -</encode>
+            <decode>oggdec -R -b 16 -e 0 -s 1 -o - @T@</decode>
+            <encode>oggenc -r -B 16 -C 2 -R 44100 --raw-endianness 0 -q 1.5 -t @M@ -</encode>
        </encdec>
    </reencode>
 </ezstream>
--- a/examples/ezstream_reencode_mp3.xml
+++ b/examples/ezstream_reencode_mp3.xml
@ -61,7 +61,7 @@
            <!-- Support for FLAC decoding: -->
            <format>FLAC</format>
            <match>.flac</match>
-            <decode>flac -s -d --force-raw-format --sign=signed --endian=little -o - "@T@"</decode>
+            <decode>flac -s -d --force-raw-format --sign=signed --endian=little -o - @T@</decode>
            <!-- <encode>Not supported Yet</encode> -->
        </encdec>
        <encdec>
@ -71,7 +71,7 @@
            <format>MP3</format>
            <match>.mp3</match>
            <!-- Note: madplay uses host byte order for raw samples. -->
-            <decode>madplay -b 16 -R 44100 -S -o raw:- "@T@"</decode>
+            <decode>madplay -b 16 -R 44100 -S -o raw:- @T@</decode>
            <encode>lame --preset cbr 128 -r -s 44.1 --bitwidth 16 - -</encode>
        </encdec>
        <encdec>
@ -80,8 +80,8 @@
             -->
            <format>VORBIS</format>
            <match>.ogg</match>
-            <decode>oggdec -R -b 16 -e 0 -s 1 -o - "@T@"</decode>
-            <encode>oggenc -r -B 16 -C 2 -R 44100 --raw-endianness 0 -q 1.5 -t "@M@" -</encode>
+            <decode>oggdec -R -b 16 -e 0 -s 1 -o - @T@</decode>
+            <encode>oggenc -r -B 16 -C 2 -R 44100 --raw-endianness 0 -q 1.5 -t @M@ -</encode>
        </encdec>
    </reencode>
 </ezstream>
--- a/examples/ezstream_reencode_theora.xml
+++ b/examples/ezstream_reencode_theora.xml
@ -67,12 +67,12 @@
             -->
            <format>THEORA</format>
            <match>.avi</match>
-            <decode>ffmpeg2theora -x 192 -y 128 -a 0 -v 4 --title "@M@" -o - "@T@"</decode>
+            <decode>ffmpeg2theora -x 192 -y 128 -a 0 -v 4 --title @M@ -o - @T@</decode>
        </encdec>
        <encdec>
            <format>THEORA</format>
            <match>.mpg</match>
-            <decode>ffmpeg2theora -x 192 -y 128 -a 0 -v 4 --title "@M@" -o - "@T@"</decode>
+            <decode>ffmpeg2theora -x 192 -y 128 -a 0 -v 4 --title @M@ -o - @T@</decode>
        </encdec>
    </reencode>
 </ezstream>
--- a/examples/ezstream_reencode_vorbis.xml
+++ b/examples/ezstream_reencode_vorbis.xml
@ -62,7 +62,7 @@
            <!-- Support for FLAC decoding: -->
            <format>FLAC</format>
            <match>.flac</match>
-            <decode>flac -s -d --force-raw-format --sign=signed --endian=little -o - "@T@"</decode>
+            <decode>flac -s -d --force-raw-format --sign=signed --endian=little -o - @T@</decode>
            <!-- <encode>Not supported Yet</encode> -->
        </encdec>
        <encdec>
@ -72,7 +72,7 @@
            <format>MP3</format>
            <match>.mp3</match>
            <!-- Note: madplay uses host byte order for raw samples. -->
-            <decode>madplay -b 16 -R 44100 -S -o raw:- "@T@"</decode>
+            <decode>madplay -b 16 -R 44100 -S -o raw:- @T@</decode>
            <encode>lame --preset cbr 128 -r -s 44.1 --bitwidth 16 - -</encode>
        </encdec>
        <encdec>
@ -81,8 +81,8 @@
             -->
            <format>VORBIS</format>
            <match>.ogg</match>
-            <decode>oggdec -R -b 16 -e 0 -s 1 -o - "@T@"</decode>
-            <encode>oggenc -r -B 16 -C 2 -R 44100 --raw-endianness 0 -q 1.5 -t "@M@" -</encode>
+            <decode>oggdec -R -b 16 -e 0 -s 1 -o - @T@</decode>
+            <encode>oggenc -r -B 16 -C 2 -R 44100 --raw-endianness 0 -q 1.5 -t @M@ -</encode>
        </encdec>
    </reencode>
 </ezstream>
--- a/src/ezstream.c
+++ b/src/ezstream.c
@ -90,8 +90,8 @@ typedef struct tag_ID3Tag {
 } ID3Tag;

 int		urlParse(const char *, char **, unsigned short *, char **);
-void		replaceString(const char *, char *, size_t, const char *,
-			      const char *);
+char *		shellQuote(const char *);
+char *		replaceString(const char *, const char *, const char *);
 char *		buildCommandString(const char *, const char *, metadata_t *);
 char *		getMetadataString(const char *, metadata_t *);
 metadata_t *	getMetadata(const char *);
@ -197,25 +197,67 @@ urlParse(const char *url, char **hostname, unsigned short *port,
 	return (1);
 }

-void
-replaceString(const char *source, char *dest, size_t size,
-	      const char *from, const char *to)
-{
-	const char	*p1 = source;
-	const char	*p2;
+#define SHELLQUOTE_INLEN_MAX	8191UL

+char *
+shellQuote(const char *in)
+{
+	char		*out, *out_p;
+	size_t		 out_len;
+	const char	*in_p;
+
+	out_len = (strlen(in) > SHELLQUOTE_INLEN_MAX)
+	    ? SHELLQUOTE_INLEN_MAX
+	    : strlen(in);
+	out_len = out_len * 2 + 2;
+	out = xcalloc(out_len + 1, sizeof(char));
+
+	out_p = out;
+	in_p = in;
+
+	*out_p++ = '\'';
+	out_len--;
+	while (*in_p && out_len > 2) {
+		switch (*in_p) {
+		case '\'':
+		case '\\':
+			*out_p++ = '\\';
+			out_len--;
+			break;
+		default:
+			break;
+		}
+		*out_p++ = *in_p++;
+		out_len--;
+	}
+	*out_p++ = '\'';
+
+	return (out);
+}
+
+char *
+replaceString(const char *source, const char *from, const char *to)
+{
+	char		*to_quoted, *dest;
+	size_t		 dest_size;
+	const char	*p1, *p2;
+
+	to_quoted = shellQuote(to);
+	dest_size = strlen(source) + strlen(to_quoted) + 1;
+	dest = xcalloc(dest_size, sizeof(char));
+
+	p1 = source;
 	p2 = strstr(p1, from);
 	if (p2 != NULL) {
-		if ((unsigned int)(p2 - p1) >= size) {
-			printf("%s: replaceString(): Internal error: p2 - p1 >= size\n",
-			       __progname);
-			abort();
-		}
 		strncat(dest, p1, (size_t)(p2 - p1));
-		strlcat(dest, to, size);
+		strlcat(dest, to_quoted, dest_size);
 		p1 = p2 + strlen(from);
 	}
-	strlcat(dest, p1, size);
+	strlcat(dest, p1, dest_size);
+
+	xfree(to_quoted);
+
+	return (dest);
 }

 char *
@ -227,9 +269,7 @@ buildCommandString(const char *extension, const char *fileName,
 	char	*encoder = NULL;
 	char	*decoder = NULL;
 	char	*newDecoder = NULL;
-	size_t	 newDecoderLen = 0;
 	char	*newEncoder = NULL;
-	size_t	 newEncoderLen = 0;
 	char	*localTitle = UTF8toCHAR(metadata_get_title(mdata),
 					 ICONV_REPLACE);
 	char	*localArtist = UTF8toCHAR(metadata_get_artist(mdata),
@ -247,23 +287,16 @@ buildCommandString(const char *extension, const char *fileName,
 		xfree(decoder);
 		return (NULL);
 	}
-	newDecoderLen = strlen(decoder) + strlen(fileName) + 1;
-	newDecoder = xcalloc(newDecoderLen, sizeof(char));
-	replaceString(decoder, newDecoder, newDecoderLen, TRACK_PLACEHOLDER,
-		      fileName);
+	newDecoder = replaceString(decoder, TRACK_PLACEHOLDER, fileName);
 	if (strstr(decoder, ARTIST_PLACEHOLDER) != NULL) {
-		size_t tmpLen = strlen(newDecoder) + strlen(localArtist) + 1;
-		char *tmpStr = xcalloc(tmpLen, sizeof(char));
-		replaceString(newDecoder, tmpStr, tmpLen, ARTIST_PLACEHOLDER,
-			      localArtist);
+		char *tmpStr = replaceString(newDecoder, ARTIST_PLACEHOLDER,
+		    localArtist);
 		xfree(newDecoder);
 		newDecoder = tmpStr;
 	}
 	if (strstr(decoder, TITLE_PLACEHOLDER) != NULL) {
-		size_t tmpLen = strlen(newDecoder) + strlen(localTitle) + 1;
-		char *tmpStr = xcalloc(tmpLen, sizeof(char));
-		replaceString(newDecoder, tmpStr, tmpLen, TITLE_PLACEHOLDER,
-			      localTitle);
+		char *tmpStr = replaceString(newDecoder, TITLE_PLACEHOLDER,
+		    localTitle);
 		xfree(newDecoder);
 		newDecoder = tmpStr;
 	}
@ -280,27 +313,20 @@ buildCommandString(const char *extension, const char *fileName,
 	if (strstr(decoder, METADATA_PLACEHOLDER) != NULL) {
 		if (metadataFromProgram && pezConfig->metadataFormat != NULL) {
 			char *mdataString = getMetadataString(pezConfig->metadataFormat, mdata);
-			size_t tmpLen = strlen(newDecoder) + strlen(mdataString) + 1;
-			char *tmpStr = xcalloc(tmpLen, sizeof(char));
-			replaceString(newDecoder, tmpStr, tmpLen,
-				      METADATA_PLACEHOLDER, mdataString);
+			char *tmpStr = replaceString(newDecoder,
+			    METADATA_PLACEHOLDER, mdataString);
 			xfree(newDecoder);
 			xfree(mdataString);
 			newDecoder = tmpStr;
 		} else {
 			if (!metadataFromProgram && strstr(decoder, TITLE_PLACEHOLDER) != NULL) {
-				size_t tmpLen = strlen(newDecoder) + 1;
-				char *tmpStr = xcalloc(tmpLen, sizeof(char));
-				replaceString(newDecoder, tmpStr, tmpLen,
-					      METADATA_PLACEHOLDER, "");
+				char *tmpStr = replaceString(newDecoder,
+				    METADATA_PLACEHOLDER, "");
 				xfree(newDecoder);
 				newDecoder = tmpStr;
 			} else {
-				size_t tmpLen = strlen(newDecoder) + strlen(localMetaString) + 1;
-				char *tmpStr = xcalloc(tmpLen, sizeof(char));
-				replaceString(newDecoder, tmpStr, tmpLen,
-					      METADATA_PLACEHOLDER,
-					      localMetaString);
+				char *tmpStr = replaceString(newDecoder,
+				    METADATA_PLACEHOLDER, localMetaString);
 				xfree(newDecoder);
 				newDecoder = tmpStr;
 			}
@ -326,42 +352,30 @@ buildCommandString(const char *extension, const char *fileName,
 		return (commandString);
 	}

-	newEncoderLen = strlen(encoder) + strlen(localArtist) + 1;
-	newEncoder = xcalloc(newEncoderLen, sizeof(char));
-	replaceString(encoder, newEncoder, newEncoderLen, ARTIST_PLACEHOLDER,
-		      localArtist);
+	newEncoder = replaceString(encoder, ARTIST_PLACEHOLDER, localArtist);
 	if (strstr(encoder, TITLE_PLACEHOLDER) != NULL) {
-		size_t tmpLen = strlen(newEncoder) + strlen(localTitle) + 1;
-		char *tmpStr = xcalloc(tmpLen, sizeof(char));
-		replaceString(newEncoder, tmpStr, tmpLen, TITLE_PLACEHOLDER,
-			      localTitle);
+		char *tmpStr = replaceString(newEncoder, TITLE_PLACEHOLDER,
+		    localTitle);
 		xfree(newEncoder);
 		newEncoder = tmpStr;
 	}
 	if (strstr(encoder, METADATA_PLACEHOLDER) != NULL) {
 		if (metadataFromProgram && pezConfig->metadataFormat != NULL) {
 			char *mdataString = getMetadataString(pezConfig->metadataFormat, mdata);
-			size_t tmpLen = strlen(newEncoder) + strlen(mdataString) + 1;
-			char *tmpStr = xcalloc(tmpLen, sizeof(char));
-			replaceString(newEncoder, tmpStr, tmpLen,
-				      METADATA_PLACEHOLDER, mdataString);
+			char *tmpStr = replaceString(newEncoder,
+			    METADATA_PLACEHOLDER, mdataString);
 			xfree(newEncoder);
 			xfree(mdataString);
 			newEncoder = tmpStr;
 		} else {
 			if (!metadataFromProgram && strstr(encoder, TITLE_PLACEHOLDER) != NULL) {
-				size_t tmpLen = strlen(newEncoder) + 1;
-				char *tmpStr = xcalloc(tmpLen, sizeof(char));
-				replaceString(newEncoder, tmpStr, tmpLen,
-					      METADATA_PLACEHOLDER, "");
+				char *tmpStr = replaceString(newEncoder,
+				    METADATA_PLACEHOLDER, "");
 				xfree(newEncoder);
 				newEncoder = tmpStr;
 			} else {
-				size_t tmpLen = strlen(newEncoder) + strlen(localMetaString) + 1;
-				char *tmpStr = xcalloc(tmpLen, sizeof(char));
-				replaceString(newEncoder, tmpStr, tmpLen,
-					      METADATA_PLACEHOLDER,
-					      localMetaString);
+				char *tmpStr = replaceString(newEncoder,
+				    METADATA_PLACEHOLDER, localMetaString);
 				xfree(newEncoder);
 				newEncoder = tmpStr;
 			}
@ -389,7 +403,6 @@ char *
 getMetadataString(const char *format, metadata_t *mdata)
 {
 	char	*tmp, *str;
-	size_t	 len;

 	if (mdata == NULL) {
 		printf("%s: getMetadataString(): Internal error: NULL metadata_t\n",
@ -403,34 +416,26 @@ getMetadataString(const char *format, metadata_t *mdata)
 	str = xstrdup(format);

 	if (strstr(format, ARTIST_PLACEHOLDER) != NULL) {
-		len = strlen(str) + strlen(metadata_get_artist(mdata)) + 1;
-		tmp = xcalloc(len, sizeof(char));
-		replaceString(str, tmp, len, ARTIST_PLACEHOLDER,
-			      metadata_get_artist(mdata));
+		tmp = replaceString(str, ARTIST_PLACEHOLDER,
+		    metadata_get_artist(mdata));
 		xfree(str);
 		str = tmp;
 	}
 	if (strstr(format, TITLE_PLACEHOLDER) != NULL) {
-		len = strlen(str) + strlen(metadata_get_title(mdata)) + 1;
-		tmp = xcalloc(len, sizeof(char));
-		replaceString(str, tmp, len, TITLE_PLACEHOLDER,
-			      metadata_get_title(mdata));
+		tmp = replaceString(str, TITLE_PLACEHOLDER,
+		    metadata_get_title(mdata));
 		xfree(str);
 		str = tmp;
 	}
 	if (strstr(format, STRING_PLACEHOLDER) != NULL) {
-		len = strlen(str) + strlen(metadata_get_string(mdata)) + 1;
-		tmp = xcalloc(len, sizeof(char));
-		replaceString(str, tmp, len, STRING_PLACEHOLDER,
-			      metadata_get_string(mdata));
+		tmp = replaceString(str, STRING_PLACEHOLDER,
+		    metadata_get_string(mdata));
 		xfree(str);
 		str = tmp;
 	}
 	if (strstr(format, TRACK_PLACEHOLDER) != NULL) {
-		len = strlen(str) + strlen(metadata_get_filename(mdata)) + 1;
-		tmp = xcalloc(len, sizeof(char));
-		replaceString(str, tmp, len, TRACK_PLACEHOLDER,
-			      metadata_get_filename(mdata));
+		tmp = replaceString(str, TRACK_PLACEHOLDER,
+		    metadata_get_filename(mdata));
 		xfree(str);
 		str = tmp;
 	}