[FFmpeg-devel] [patch]MMS protocol over TCP
Michael Niedermayer
michaelni
Tue Apr 20 20:58:03 CEST 2010
On Tue, Apr 20, 2010 at 11:58:53PM +0800, zhentan feng wrote:
> Hi
>
> On Tue, Apr 20, 2010 at 9:09 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
>
> > On Tue, Apr 20, 2010 at 12:46:17AM +0800, zhentan feng wrote:
> > > Hi
> > >
> > > On Mon, Apr 19, 2010 at 6:39 AM, Michael Niedermayer <michaelni at gmx.at
> > >wrote:
> > [...]
> > > >
> > > > [...]
> > > > > +static int asf_header_parser(MMSContext *mms)
> > > > > +{
> > > > > + uint8_t *p = mms->asf_header, *end = mms->asf_header +
> > > > mms->asf_header_size;
> > > > > + int flags, stream_id;
> > > > > + mms->stream_num = 0;
> > > > > +
> > > > > + if (mms->asf_header_size < sizeof(ff_asf_guid) * 2 + 22 ||
> > > > > + memcmp(p, ff_asf_header, sizeof(ff_asf_guid)))
> > > > > + return -1;
> > > > > +
> > > > > + p += sizeof(ff_asf_guid) + 14;
> > > > > + do {
> > > >
> > > > > + uint64_t chunksize = AV_RL64(p + sizeof(ff_asf_guid));
> > > > > + if (!memcmp(p, ff_asf_file_header, sizeof(ff_asf_guid))) {
> > > > > + /* read packet size */
> > > > > + if (end - p > sizeof(ff_asf_guid) * 2 + 68) {
> > > >
> > > > > + mms->asf_packet_len = AV_RL32(p +
> > sizeof(ff_asf_guid) *
> > > > 2 + 64);
> > > >
> > > > this looks unsafe, there is code that writes based on asf_packet_len
> > >
> > >
> > > how the code make it unsafe? could you please explain it?
> >
> > here:
> > > + int padding_size = mms->asf_packet_len - mms->pkt_buf_len;
> > > + memset(mms->incoming_buffer + mms->pkt_buf_len, 0,
> > padding_size);
> >
> > the memset maybe can write over the end of the array
> >
>
> fixed like this:
> --- mms/mmst.c Mon Apr 19 18:35:30 2010 (r5764)
> +++ mms/mmst.c Tue Apr 20 17:04:45 2010 (r5765)
> @@ -411,6 +411,10 @@ static int asf_header_parser(MMSContext
> /* read packet size */
> if (end - p > sizeof(ff_asf_guid) * 2 + 68) {
> mms->asf_packet_len = AV_RL32(p + sizeof(ff_asf_guid) * 2 +
> 64);
> + if (mms->asf_packet_len > sizeof(mms->incoming_buffer)) {
> + dprintf(NULL,"Too large packet len:%d"
> + " may overwrite incoming_buffer when padding",
> mms->asf_packet_len);
> + }
that doesnt look sufficient to prevent a segfault
or am i missing something?
[...]
> >
> >
> > [...]
> > > +/** Read incoming MMST media, header or command packet. */
> > > +static MMSSCPacketType get_tcp_server_response(MMSContext *mms)
> > > +{
> > > + int read_result;
> > > + MMSSCPacketType packet_type= -1;
> > > +
> > > + for(;;) {
> > > + if((read_result= url_read_complete(mms->mms_hd,
> > mms->incoming_buffer, 8))==8) {
> > > + // handle command packet.
> > > + if(AV_RL32(mms->incoming_buffer + 4)==0xb00bface) {
> > > + mms->incoming_flags= mms->incoming_buffer[3];
> > > + read_result= url_read_complete(mms->mms_hd,
> > mms->incoming_buffer+8, 4);
> > > + if(read_result == 4) {
> > > + int length_remaining=
> > AV_RL32(mms->incoming_buffer+8) + 4;
> > > +
> > > + dprintf(NULL, "Length remaining is %d\n",
> > length_remaining);
> > > + // read the rest of the packet.
> > > + if (length_remaining < 0
> > > + || length_remaining >
> > sizeof(mms->incoming_buffer) - 12) {
> > > + dprintf("Incoming message len %d exceeds buffer
> > len %d\n",
> > > + length_remaining,
> > sizeof(mms->incoming_buffer) - 12);
> > > + break;
> > > + }
> > > + read_result = url_read_complete(mms->mms_hd,
> > mms->incoming_buffer + 12,
> > > + length_remaining) ;
> > > + if (read_result == length_remaining) {
> >
> > > + packet_type= AV_RL16(mms->incoming_buffer+36);
> >
> > is any value valid here?
> > i think this is missing some checks
> >
> > packet_type is handled later with exception checks.
> I think it's not necessary to check it here.
not checking it leads to a number of possible code pathes which are
apparently not supposed to happen, some of them will just segfault due to
varibles like the read ptr being advanced without bound.
It looks like the code has been written completely without considering
security. This is the wrong mindset. Code that communicates over the network
should be written with the assumtation that every bit received has been
crafted by someone manually to exploit or crash the lib.
That is when writing such code you should ask yourself "how can i make it
crash? or how can i overwrite the end of the array and maybe that way
gain access to the computer running the code"
Theres no lack of people trying to find ways to exploit code, we thus have
no choice but to make sure our code is to the best of our knowledge not
exploitable.
[...]
> >
> > [...]
> > > +/** Send MMST stream selection command based on the AVStream->discard
> > values. */
> > > +static int send_stream_selection_request(MMSContext *mms)
> > > +{
> > > + int i;
> > > +
> > > + // send the streams we want back...
> > > + start_command_packet(mms, CS_PKT_STREAM_ID_REQUEST);
> > > + bytestream_put_le32(&mms->write_ptr, mms->stream_num); //
> > stream nums
> > > + if (mms->write_ptr - mms->outgoing_packet_buffer >
> > > + sizeof(mms->outgoing_packet_buffer) - 6 * mms->stream_num) {
> > > + dprintf("buffer will overflow for too many streams: %d.\n",
> > mms->stream_num);
> > > + return AVERROR_IO;
> > > + }
> >
> > this should probably be checked before adding the streams
> >
> > fixed it like this:
> --- mms/mmst.c Tue Apr 20 17:39:15 2010 (r5772)
> +++ mms/mmst.c Tue Apr 20 17:50:20 2010 (r5773)
> @@ -417,7 +417,8 @@ static int asf_header_parser(MMSContext
> } else if (!memcmp(p, ff_asf_stream_header, sizeof(ff_asf_guid))) {
> flags = AV_RL16(p + sizeof(ff_asf_guid)*3 + 24);
> stream_id = flags & 0x7F;
> - if (mms->stream_num < MAX_STREAMS) {
> + if (mms->stream_num < MAX_STREAMS &&
> + 46 + mms->stream_num * 6 < sizeof(mms->out_buffer)) {
this needs a comment explaining what this check does
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100420/c7e3fdf9/attachment.pgp>
More information about the ffmpeg-devel
mailing list