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

zhentan feng spyfeng
Thu Apr 22 18:14:29 CEST 2010


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?


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


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

zhentan
-- 
Best wishes~



More information about the ffmpeg-devel mailing list