[FFmpeg-devel] [patch]MMS protocol over TCP

zhentan feng spyfeng
Sun Apr 25 10:17:37 CEST 2010


Hi

On Fri, Apr 23, 2010 at 10:22 PM, Michael Niedermayer <michaelni at gmx.at>wrote:

> On Fri, Apr 23, 2010 at 12:14:29AM +0800, zhentan feng wrote:
> > Hi
> >
> > On Wed, Apr 21, 2010 at 2:58 AM, Michael Niedermayer <michaelni at gmx.at
> >wrote:
> >
> > > 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?
> > >
> > > in pad_media_packet():
> > if(mms->remaining_in_len<mms->asf_packet_len) is ture,
> > there is memset operation for in_buffer[].
> > memset(mms->in_buffer + mms->remaining_in_len, 0, padding_size);
> >
> > however, padding_size = mms->asf_packet_len - mms->remaining_in_len.
> > in other words, padding_size  is determined by mms->asf_packet_len.
> > so if mms->asf_packet_len < sizeof(mms->incoming_buffer), the
> overwritting
> > won't happen.
> > is this right?
>
> you just print an error, the code does not stop asf_packet_len from
> being used
> thats besides it does not catch asf_packet_len < 0 which it probably
> should too
>
>
fixed.

>
>
> >
> >
> > >
> > > [...]
> > >
> > > > >
> > > > >
> > > > > [...]
> > > > > > +/** 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.
> > >
> > > yes, I learn a lesson from this. I'll check the code carefully.
> > however, as this point,
> > when we read the packet_type, the next thing what we do is checking the
> > value.
> > see the code in Line345 (break from the while() loop);
> > and return the packet_type and check it in Line596.
> > it will throw the unknow packet type exception.
> >
> > After considering it  again, I think you mean I should check the return
> type
> > against what I sent to server.
> > is this right?
>
> what ive meant was that you read the packet_type with
> packet_type= AV_RL16(mms->in_buffer+36);
> and at other places use code like
> packet_type = SC_PKT_ASF_MEDIA;
>
> it seemed to me that this might be wrong if the AV_RL16() returned
> one of the explicit values
>

I added a funtion mms_safe_send_recv() to check sending and receiving by
expected packet type.
to make sure we get the correct packet we want.
please see the new patch.

>
>

> > if it's ture, the code lines below are not a good method.
> >     while (!mms->header_parsed) {
> >         packet_type = get_tcp_server_response(mms);
> >         ret = handle_mms_msg_pkt(mms, packet_type);
> >         if (ret < 0)
> >             break;
> >     }
> > then , I'll break up the function handle_mms_msg_pkt()
> > and add the code to check the returned packet type immediately after
> sending
> > some packet.
>
> thats a good idea as well
>
>
>
> >
> >
> > > [...]
> > > > >
> > > > > [...]
> > > > > > +/** 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
> > >
> > > hmm, well.
> > In fact, we can calculate the CS_PKT_STREAM_ID_REQUEST packet size once
> we
> > have mms->stream_num value.
> > please see function send_stream_selection_request().
> > In the function, start_command_packet() write 40 bytes.
> > then 4bytes.
> > then 6byte*stream_num
> > then 2bytes,
> > that's is 46 + mms->stream_num * 6 bytes.
>
> yes, i know but not everyone reading the code will know, thats why
> i think this should be documented in the code with a comment
>
> fixed.

zhentan
-- 
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mmst_12.patch
Type: application/octet-stream
Size: 26662 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100425/c4d510f1/attachment.obj>



More information about the ffmpeg-devel mailing list