[rtmpdump] [PATCH] Publisher authentication

[Steve McFarlin]

On Feb 2, 2011, at 4:05 PM, Steve McFarlin wrote:

> 
> On Feb 2, 2011, at 1:33 PM, Steve McFarlin wrote:
> 
>> 
>> On Feb 2, 2011, at 1:08 PM, Howard Chu wrote:
>> 
>>> Steve McFarlin wrote:
>>>> 
>>>> On Feb 2, 2011, at 12:03 PM, Steve McFarlin wrote:
>>>> 
>>>>> The attached adobe auth patch has an infinite recursion in it. I believe it is the one from Sergiy.
>>>>> 
>>>>> Stack Trace:
>>>>> ..
>>>>> ...
>>>>> #743	0x0047a818 in RTMP_ConnectStream at rtmp.c:1047
>>>>> #744	0x0047c2c6 in HandleInvoke at rtmp.c:2765
>>>>> #745	0x0047ae92 in RTMP_ClientPacket at rtmp.c:1248
>>>>> #746	0x0047a818 in RTMP_ConnectStream at rtmp.c:1047
>>>>> #747	0x0047c2c6 in HandleInvoke at rtmp.c:2765
>>>>> #748	0x0047ae92 in RTMP_ClientPacket at rtmp.c:1248
>>>>> #749	0x0047a818 in RTMP_ConnectStream at rtmp.c:1047
>>>>> #750	0x0047c2c6 in HandleInvoke at rtmp.c:2765
>>>>> #751	0x0047ae92 in RTMP_ClientPacket at rtmp.c:1248
>>>>> #752	0x0047a818 in RTMP_ConnectStream at rtmp.c:1047
>>>>> 
>>>>> The above file offsets may be different then a auto patched librtmp. I had to manually patch.
>>>>> 
>>>>> This occurs when connecting to a Wowza Media Server using an invalid application for publishing. The server rejects the connection and then the following block in HandleInvoke initiates the infinite recursion. Specifically the call to RTMP_ConnectStream. I have not had a chance to fix this. If I patch the patch I will post it.
>>>>> 
>>>>> else if (AVMATCH(&method,&av_close))
>>>>>   {
>>>>>      RTMP_Log(RTMP_LOGERROR, "rtmp server requested close");
>>>>>      RTMP_Close(r);
>>>>> #ifdef CRYPTO
>>>>>      if ((r->Link.protocol&  RTMP_FEATURE_WRITE)&&
>>>>>          !(r->Link.pFlags&  RTMP_PUB_CLEAN)&&
>>>>>          (  !(r->Link.pFlags&  RTMP_PUB_NAME) ||
>>>>>           !(r->Link.pFlags&  RTMP_PUB_RESP) ||
>>>>>           (r->Link.pFlags&  RTMP_PUB_CLATE) ) )
>>>>>       {
>>>>>          /* clean later */
>>>>>          if(r->Link.pFlags&  RTMP_PUB_CLATE)
>>>>>              r->Link.pFlags |= RTMP_PUB_CLEAN;
>>>>>          RTMP_Log(RTMP_LOGERROR, "authenticating publisher");
>>>>> 
>>>>>          if (!RTMP_Connect(r, NULL) || !RTMP_ConnectStream(r, 0))
>>>>>              goto leave;
>>>>>       }
>>>>> #endif
>>>>>   }
>>>>> 
>>>> 
>>>> The quick and easy fix without understanding the implications is to just 'goto leave;' after the RTMP_Close(r) call. You could optionally remove the CRYPTO code all together. The issue is RTMP_Connect will return true as long as a connection can be made. And as mentioned above RTMP_ConnectStream will end up calling back into HandleInvoke.
>>>> 
>>>> Again, I did not mentally trace this code. I don't know the implications of my suggestion. All I know is that it works in the few tests I preformed.
>>> 
>>> This piece of your patch was almost certainly applied wrong. There's no way you should be trying to connect after receiving a close request.
>>> 
>> 
>> This is a definite possibility as patch had issues applying to the sources, so I had to manually apply a portion of it. However, it does not look like it . Here is the relevant code from the patch
>> 
>>    }
>>  else if (AVMATCH(&method, &av_close))
>>    {
>>      RTMP_Log(RTMP_LOGERROR, "rtmp server requested close");
>>      RTMP_Close(r);
>> +#ifdef CRYPTO
>> +      if ((r->Link.protocol & RTMP_FEATURE_WRITE) &&
>> +              !(r->Link.pFlags & RTMP_PUB_CLEAN) &&
>> +              (  !(r->Link.pFlags & RTMP_PUB_NAME) ||
>> +                 !(r->Link.pFlags & RTMP_PUB_RESP) ||
>> +                 (r->Link.pFlags & RTMP_PUB_CLATE) ) )
>> +        {
>> +          /* clean later */
>> +          if(r->Link.pFlags & RTMP_PUB_CLATE)
>> +              r->Link.pFlags |= RTMP_PUB_CLEAN;
>> +          RTMP_Log(RTMP_LOGERROR, "authenticating publisher");
>> +
>> +          if (!RTMP_Connect(r, NULL) || !RTMP_ConnectStream(r, 0))
>> +              goto leave;
>> +       }
>> +#endif
>>    }
>>  else if (AVMATCH(&method, &av_onStatus))
>> 
>> 
>> I do not see another "else if (AVMATCH(&method, &av_close))" anywhere in the code. It could be possible I am using a early patch. I did notice there were a few floating around from Sergiy.
> 
> The above code is required for the authentication to work. It looks like it takes a few attempts for the authentication to be accepted. I have no idea why. Maybe I will look into it later.
> 
> steve

It may very well be my lack of understanding of how patch works. I believe I patched correctly. Anyway, it looks like the proper place for the code between the CRYPTO tags is in the clause just above it. The patch code has these lines:

/* if PublisherAuth returns 1, then reconnect */
PublisherAuth(r, &description);

But does nothing after this. Putting the above code between the CRYPTO define solves the issue. 

                if (PublisherAuth(r, &description)) {
                    RTMP_Log(RTMP_LOGERROR, "RTMP SERVER REQUESTED CLOSE");
                    RTMP_Close(r);
                  
                     if ((r->Link.protocol & RTMP_FEATURE_WRITE) &&
                        !(r->Link.pFlags & RTMP_PUB_CLEAN) &&
                        (  !(r->Link.pFlags & RTMP_PUB_NAME) ||
                         !(r->Link.pFlags & RTMP_PUB_RESP) ||
                         (r->Link.pFlags & RTMP_PUB_CLATE) ) )
                     {
                        /* clean later */
                        if(r->Link.pFlags & RTMP_PUB_CLATE)
                            r->Link.pFlags |= RTMP_PUB_CLEAN;
                        RTMP_Log(RTMP_LOGERROR, "AUTHENTICATING USER");
                        
                        if (!RTMP_Connect(r, NULL) || !RTMP_ConnectStream(r, 0))
                            goto leave;
                     }
                }

Given this is executed a few times before a successful connect I am going to assume I have introduced a leak. If I am in fact using an old patch, could someone point me to the latest. I did not find anything new about this patch in the archives.


steve