On Mon, 2012-09-03 at 23:09 +0200, Max Kellermann wrote:
On 2012/08/21 20:01, Jurgen Kramer gtmkra...@xs4all.nl wrote:
Can you use dsdlib_tag_id3() for a DoS attack? This looks like it
could easily cause a stack overflow:
+ count = is-size - is-offset;
+ id3_byte_t dsdid3[count];
What is your concern here? The allocation of dsdid3 using count or the
value of count being calculated that way (or both)?
The former. A malicious file can cause a stack overflow. Imagine a
file that makes MPD try to allocate a few gigabytes of heap.
Attached is an old fashioned patch with the updated code against current
mpd git.
I cannot make heads or tales from the mess that is my mpd git repo.
Everything I try only seems to make it worse.
Jurgen
diff -uNrp mpd/src/decoder/dsdiff_decoder_plugin.c mpd-git-0.18-jk/src/decoder/dsdiff_decoder_plugin.c
--- mpd/src/decoder/dsdiff_decoder_plugin.c 2012-09-19 14:30:05.059747095 +0200
+++ mpd-git-0.18-jk/src/decoder/dsdiff_decoder_plugin.c 2012-09-19 14:45:04.204797373 +0200
@@ -52,10 +52,23 @@ struct dsdiff_chunk_header {
uint32_t size_high, size_low;
};
+/** struct for DSDIFF native Artist and Title tags */
+struct dsdiff_native_tag {
+ uint32_t size;
+};
+
struct dsdiff_metadata {
unsigned sample_rate, channels;
bool bitreverse;
uint64_t chunk_size;
+#ifdef HAVE_ID3TAG
+ goffset id3_offset;
+ uint64_t id3_size;
+#endif
+ /** offset for artist tag */
+ goffset diar_offset;
+ /** offset for title tag */
+ goffset diti_offset;
};
static bool lsbitfirst;
@@ -187,6 +200,122 @@ dsdiff_read_prop(struct decoder *decoder
return dsdlib_skip_to(decoder, is, end_offset);
}
+static void
+dsdiff_handle_native_tag(struct input_stream *is,
+ const struct tag_handler *handler,
+ void *handler_ctx, goffset tagoffset,
+ enum tag_type type)
+{
+ if (!dsdlib_skip_to(NULL, is, tagoffset))
+ return;
+
+ struct dsdiff_native_tag metatag;
+
+ if (!dsdlib_read(NULL, is, metatag, sizeof(metatag)))
+ return;
+
+ uint32_t length = GUINT32_FROM_BE(metatag.size);
+ char string[length];
+ char *label;
+ label = string;
+
+ if (!dsdlib_read(NULL, is, label, (size_t)length))
+ return;
+
+ string[length] = '\0';
+ tag_handler_invoke_tag(handler, handler_ctx, type, label);
+ return;
+}
+
+/**
+ * Read and parse additional metadata chunks for tagging purposes. By default
+ * dsdiff files only support equivalents for artist and title but some of the
+ * extract tools add an id3 tag to provide more tags. If such id3 is found
+ * this will be used for tagging otherwise the native tags (if any) will be
+ * used
+ */
+
+static bool
+dsdiff_read_metadata_extra(struct decoder *decoder, struct input_stream *is,
+ struct dsdiff_metadata *metadata,
+ struct dsdiff_chunk_header *chunk_header,
+ const struct tag_handler *handler,
+ void *handler_ctx)
+{
+
+ /* skip from DSD data to next chunk header */
+ if (!dsdlib_skip(decoder, is, metadata-chunk_size))
+ return false;
+ if (!dsdiff_read_chunk_header(decoder, is, chunk_header))
+ return false;
+
+#ifdef HAVE_ID3TAG
+ metadata-id3_size = 0;
+#endif
+
+ /* Now process all the remaining chunk headers in the stream
+ and record their position and size */
+
+ while ( is-offset is-size )
+ {
+ uint64_t chunk_size = dsdiff_chunk_size(chunk_header);
+
+ /* DIIN chunk, is directly followed by other chunks */
+ if (dsdlib_id_equals(chunk_header-id, DIIN))
+ chunk_size = 0;
+
+ /* DIAR chunk - DSDIFF native tag for Artist */
+ if (dsdlib_id_equals(chunk_header-id, DIAR)) {
+ chunk_size = dsdiff_chunk_size(chunk_header);
+ metadata-diar_offset = is-offset;
+ }
+
+ /* DITI chunk - DSDIFF native tag for Title */
+ if (dsdlib_id_equals(chunk_header-id, DITI)) {
+ chunk_size = dsdiff_chunk_size(chunk_header);
+ metadata-diti_offset = is-offset;
+ }
+#ifdef HAVE_ID3TAG
+ /* 'ID3 ' chunk, offspec. Used by sacdextract */
+ if (dsdlib_id_equals(chunk_header-id, ID3 )) {
+ chunk_size = dsdiff_chunk_size(chunk_header);
+ metadata-id3_offset = is-offset;
+ metadata-id3_size = chunk_size;
+ }
+#endif
+ if (chunk_size != 0) {
+ if (!dsdlib_skip(decoder, is, chunk_size))
+break;
+ }
+
+ if ( is-offset is-size ) {
+ if (!dsdiff_read_chunk_header(decoder, is, chunk_header))
+return false;
+ }
+ chunk_size = 0;
+ }
+ /* done processing chunk headers, process tags if any */
+
+#ifdef HAVE_ID3TAG
+ if (metadata-id3_offset != 0)
+ {
+ /* a ID3 tag has preference over the other tags, do not process
+ other tags if we have one */
+ dsdlib_tag_id3(is, handler, handler_ctx, metadata-id3_offset);
+ return true;
+ }
+#endif
+
+ if (metadata-diar_offset != 0)
+ dsdiff_handle_native_tag(is, handler, handler_ctx,
+ metadata-diar_offset, TAG_ARTIST);
+
+ if (metadata-diti_offset != 0)
+ dsdiff_handle_native_tag(is, handler, handler_ctx,
+ metadata-diti_offset, TAG_TITLE);
+ return true;
+}
+
/**
* Read and parse all metadata chunks at the beginning. Stop
