[sr-dev] ID generation in rls/notify.c was Re: Random number generation

[Daniel-Constantin Mierla]

Hello,

had not time yet to get and look properly at this thread, is the issue 
fixed by Peter's commit related to some srand() usage?

Cheers,
Daniel

On 4/16/12 1:15 PM, Hugh Waite wrote:
> I now realise that rls was deliberately using the same sequence of 
> random numbers to satisfy RFC4662 §5.5
>
>     5.5. Instance Attributes
>
>     Each resource element contains zero or more instance elements. These
>     instance elements are used to represent a single notifier for the
>     resource. For event packages that allow forking, multiple virtual
>     subscriptions may exist for a given resource. Multiple virtual
>     subscriptions are represented as multiple instance elements in the
>     corresponding resource element. For subscriptions in which forking
>     does not occur, at most one instance will be present for a given
>     resource.
>
>     The "id" attribute contains an opaque string used to uniquely
>     identify the instance of the resource. The "id" attribute is unique
>     only within the context of a resource. Construction of this string
>     is an implementation decision. Any mechanism for generating this
>     string is valid, as long as uniqueness within the resource is
>     assured.
>
> Seeding srand() with the values 0, 1, 2... always gives the same 
> sequence of ID strings. Maybe someone has an opinion on whether this 
> should be implemented differently, or whether a value should be stored 
> for situations where multiple resource instances are removed/reordered.
>
> Maybe kamailio should use a unique ID (from srutils) in the Via header 
> for uac requests (in modules/tm/h_table.h) anyway, to prevent a module 
> from affecting Via branch params by using srand().
>
> Regards,
> Hugh
>
> On 13/04/2012 15:26, Hugh Waite wrote:
>> Hi,
>> One of the places that uses rand() is 
>> modules/tm/h_table.c:init_synonym_id . Requests generated by kamailio 
>> (as a uac) use this to generate the Via branch tag. If any module 
>> calls srand(), it affects the Via branch for subsequent requests.
>>
>> The actual bug we found is in rls/notify.c:generate_string(). The 
>> add_resource_instance() function will re-seed srand() with 0 (zero), 
>> leading to nearly every NOTIFY sent by rls having the same "random" 
>> number in the Via branch. I am sure this was the cause of lost 
>> replies, timeouts and dropped subscriptions that we were seeing (and 
>> appears to have gone away after removing it).
>>
>> Although, I could just remove the srand from rls notify.c, I wondered 
>> if it should be using a different random function, and also whether 
>> init_synonym_id should use something more unique for the Via branch 
>> parameter.
>>
>> A quick check has shown a few places that call srand() within the 
>> code, although they probably have less drastic consequences.
>>
>> Regards,
>> Hugh
>>
>> On 13/04/2012 14:01, Daniel-Constantin Mierla wrote:
>>> Hello,
>>>
>>> I haven't looked at the issue reported by Hugh, so by now just 
>>> comments on srutils/sruid.
>>>
>>> The idea was to have an unique id generator without linking to an 
>>> external library -- my first purpose was to use it for temporary 
>>> GRUU ids (RFC5627), I am just about to push to master the gruu 
>>> support in registrar/usrloc.
>>>
>>> Then I thought it might be useful in other places, such as dialog 
>>> unique id.
>>>
>>> I added it as part of lib, since its target usage was for modules so 
>>> far, but if needed for some core processing, the two files (rather 
>>> small by now) can be moved in the core.
>>>
>>> Cheers,
>>> Daniel
>>>
>>> On 4/13/12 2:50 PM, Henning Westerholt wrote:
>>>> On Friday 13 April 2012, Hugh Waite wrote:
>>>>> I have a question about random number generation within kamailio.
>>>>>
>>>>> A number of modules use rand() to get a random value and in some 
>>>>> places
>>>>> is re-seeding with srand(). I believe this is dangerous because 
>>>>> rand()
>>>>> is used in the Via branch tag generator.
>>>>> We have detected some real bugs (where srand is reseeding with 0 for
>>>>> every message, causing transaction mis-matching) but I'm not sure 
>>>>> of the
>>>>> correct way to fix this (other than remove srand()).
>>>>>
>>>>> Should all modules be using a 'core' random function (e.g. in 
>>>>> srutils?)
>>>>> ? And if so, is this library documented?
>>>>>
>>>>> Regards,
>>>>> Hugh
>>>> Hi Hugh,
>>>>
>>>> for the purpose getting a pseudo-random number (i.e. not for 
>>>> cryptographic
>>>> functionality) we should consolidate on a single random function. 
>>>> There is the
>>>> recent introduced srutils/sruid code, then there exists a (IMHO 
>>>> stronger)
>>>> pseudo-random number generator in rand/fastrand and then there is 
>>>> of course
>>>> rand().
>>>>
>>>> Maybe Daniel can comment about the purpose of the srutils function, 
>>>> IMHO
>>>> consolidating on fastrand or one of the stronger function (d_rand 
>>>> etc..) from
>>>> stdlib.h would be fine.
>>>>
>>>> The re-seeding the internal state of rand() with srand during 
>>>> runtime sounds
>>>> wrong toe me and should be removed/ fixed.
>>>>
>>>> Viele Grüße/ best regards,
>>>>
>>>> Henning Westerholt
>>>>
>>>> _______________________________________________
>>>> sr-dev mailing list
>>>> sr-dev at lists.sip-router.org
>>>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>>>
>>
>>
>
>
> -- 
> Hugh Waite
> Senior Design Engineer
> Crocodile RCS Ltd.
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev at lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- 
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120420/b4fa9e8f/attachment.htm>