Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
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
Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
On 2012/09/19 14:50, Jurgen Kramer gtmkra...@xs4all.nl wrote: Attached is an old fashioned patch with the updated code against current mpd git. There's no patch description. I cannot make heads or tales from the mess that is my mpd git repo. Everything I try only seems to make it worse. Please fix your repository. Merging patches manually costs me a lot of time, more than it costs you to learn enough about git to know about its reset command. -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
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. -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
On Mon, 2012-08-20 at 08:44 +0200, Max Kellermann wrote: On 2012/08/16 18:18, Jurgen Kramer gtmkra...@xs4all.nl wrote: Lots of activity in git. Just a reminder for above commit. Memory leak in dsdlib_tag_id3(). Fixed. Added id3_tag_delete 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)? This looks suspicious, too: + uint64_t length = (uint64_t)GUINT32_FROM_BE(metatag.size); + char string[length]; Why is this 32 bit integer casted to 64 bit and then back to 32 bit, anyway? Changed that, length still needs conversion from uint32_t to size_t for usage in dsdlib_read unfortunately. Remember: bad files must *never* cause MPD to crash! If my theory is true, this is a serious security vulnerability you're about to add to MPD. I already have a version laying around where a put in some tighter checks on files being good DSF or DSDIFF files. Maybe I'll sent in a patch for that first. scan_id3_tag() has duplicate documentation. API documentation belongs to the header file. OK, removed comments from .c file Remove the version number from the commit message. I will not merge the old versions of your patch, these should be deleted. Rebase on my master branch, and submit *only* the patches you want me to merge, not all the old versions. I'll remove the version from the patch. Regarding only submitting the needed patches. What the commit I supplied (http://git.musicpd.org/cgit/gtmkramer/mpd.git/commit/?id=9696eff5f075180e88fca504f2502c4f12aef71b) you get more then needed? (I am still not fully proficient with git). Some API documentation for variables like diar_offset, diti_size could be useful. I don't understand what they mean. I'll add comments to the struct. Jurgen -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
On Tue, Aug 21, 2012 at 08:01:07PM +0200, Jurgen Kramer wrote: Regarding only submitting the needed patches. What the commit I supplied (http://git.musicpd.org/cgit/gtmkramer/mpd.git/commit/?id=9696eff5f075180e88fca504f2502c4f12aef71b) you get more then needed? (I am still not fully proficient with git). Try git rebase -i HEAD~9 on (a copy of) that branch. That will allow you to do several different things with the history in that branch. StGit (stacked git) is another useful tool for managing patchsets in git. (Its usage isn't overly obvious, but it has a good tutorial.) hope that helps, Jonathan Neuschäfer -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
On 2012/08/16 18:18, Jurgen Kramer gtmkra...@xs4all.nl wrote: Lots of activity in git. Just a reminder for above commit. Memory leak in dsdlib_tag_id3(). 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]; This looks suspicious, too: + uint64_t length = (uint64_t)GUINT32_FROM_BE(metatag.size); + char string[length]; Why is this 32 bit integer casted to 64 bit and then back to 32 bit, anyway? Remember: bad files must *never* cause MPD to crash! If my theory is true, this is a serious security vulnerability you're about to add to MPD. scan_id3_tag() has duplicate documentation. API documentation belongs to the header file. Remove the version number from the commit message. I will not merge the old versions of your patch, these should be deleted. Rebase on my master branch, and submit *only* the patches you want me to merge, not all the old versions. Some API documentation for variables like diar_offset, diti_size could be useful. I don't understand what they mean. -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
[Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6
Updated version of my patch to add tag support to the DSD decoders. Now uses scan_id3_tag() to add id3 tags (the comment in src/tag_id3.c/h for scan_id3_tag() may need smartening up). No more warnings when compiling with id3 support disabled. Removes unneeded code when compiling without id3 support. http://git.musicpd.org/cgit/gtmkramer/mpd.git/commit/?id=9696eff5f075180e88fca504f2502c4f12aef71b Jurgen -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
