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

Michael Niedermayer michaelni
Fri Apr 23 16:22:39 CEST 2010


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



> 
> 
> >
> > [...]
> >
> > > >
> > > >
> > > > [...]
> > > > > +/** 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


> 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


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20100423/c9ad528f/attachment.pgp>



More information about the ffmpeg-devel mailing list