[FFmpeg-devel] [patch]MMS protocol over TCP
zhentan feng
spyfeng
Mon May 3 15:37:07 CEST 2010
ping?
On Sun, Apr 25, 2010 at 4:17 PM, zhentan feng <spyfeng at gmail.com> wrote:
> 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~
>
--
Best wishes~
More information about the ffmpeg-devel
mailing list