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

Reply via email to