[asterisk-dev] [Code Review]: Generate security events in chan_sip using new Security Events Framework

[Kevin Fleming]

> On Aug. 12, 2011, 2:14 a.m., Tilghman Lesher wrote:
> > /branches/10/channels/chan_sip.c, line 1713
> > <https://reviewboard.asterisk.org/r/1362/diff/1/?file=18288#file18288line1713>
> >
> >     It is helpful to know whether the invalid password is different from the previous invalid password from this peer (if possible).  You don't need to know what that previous guess was, as a security watcher can be expected to keep history.  This can be important, to distinguish a misconfigured phone with the wrong password (false positive) from a scan attack.
> 
> elguero wrote:
>     Hmm... not sure how we would report that with the current framework as it is.
>     
>     We would have to somehow store the previous guess, so that we could compare the new guess and determine if they are the same.  Is that being done anywhere?  Any quick suggestions?
>     
>     Then we would need to add an element to this security event structure, such as, new_password_attempt and make it optional.
>     
>     Let me know if I am over thinking this.

No, that's the right strategy, but I'm not sure it's achievable. All you have as a response in an authentication attempt is a digest computed from the shared secret password, a nonce and some other bits. When you get a second authentication attempt which fails, you don't have any way to determine whether they used the same wrong password or not, because the digest value is going to be different by virtue of the nonce being different. It would be possible to reuse the nonce in order to determine whether the attempts are using different passwords or not, but that's a larger change that shouldn't be included in this patch.


> On Aug. 12, 2011, 2:14 a.m., Tilghman Lesher wrote:
> > /branches/10/channels/chan_sip.c, line 1740
> > <https://reviewboard.asterisk.org/r/1362/diff/1/?file=18288#file18288line1740>
> >
> >     I don't know that this type of event has merit, as with SIP, you're going to have an event of this type with nearly every call.  When you're looking for a needle in a haystack, the last thing you want to do is add more hay.
> 
> elguero wrote:
>     I actually was wondering about that.
>     
>     But to throw out another argument.  If I had a module tracking failed password attempts, I would want to know if they successfully authenticated in order to reset the counter.
>     
>     Also, would some security policies out there possibly require this to be tracked?  
>     
>     Just some thoughts.

Yes, it's valuable. Monitoring systems would like to be able to see if failed authentication attempts are arriving from a *different* source than the most recent successful authentication.


> On Aug. 12, 2011, 2:14 a.m., Tilghman Lesher wrote:
> > /branches/10/channels/chan_sip.c, lines 25080-25082
> > <https://reviewboard.asterisk.org/r/1362/diff/1/?file=18288#file18288line25080>
> >
> >     This should probably be treated the same as an invalid password.
> 
> elguero wrote:
>     Invalid password is kind of misleading I think.
>     
>     We have failed_acl or inval_acct_id in the framework.  I think I would use failed_acl and I can even use the optional acl_name element which would allow us to state that the failure was due to the domain.  What do you think?

Feel free to add additional event types or modify existing ones if that's the right thing to do; nothing else is using them at the moment so there's not much risk of breakage. With that said, though, I think that 'domain must match' would qualify as an 'access control rule' and thus would fit reasonably well in 'failed_acl'.


> On Aug. 12, 2011, 2:14 a.m., Tilghman Lesher wrote:
> > /branches/10/configs/logger.conf.sample, lines 112-116
> > <https://reviewboard.asterisk.org/r/1362/diff/1/?file=18289#file18289line112>
> >
> >     Could you explain this addition?  I don't see anything either in your patch or in trunk that uses this configuration line.
> 
> elguero wrote:
>     When the security event framework was added, res_security_log was added as well.  This module registers a dynamic logger.  In order to use it, this needs to be added to logger.conf.  https://wiki.asterisk.org/wiki/display/AST/Asterisk+Security+Event+Logger
>     
>     To match the documentation on the wiki, I should probably change that to security_log => security.
>     
>     My thoughts were that if we added this to the sample docs, it might help expose the security events to more people and get more traction behind generating events in other modules.  I remember seeing the framework get committed but failed to realize that I could start logging events immediately (although limited to AMI for now) since I didn't go and read the documentation.  Then I forgot about the security events addition until it was brought up on the mailing list to use the framework in a module someone was writing.
>     
>     Should I remove this and put it in another issue?

No, I think this is fine. It will only affect new installations that use 'make samples', but that's a good thing to start doing.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1362/#review4044
-----------------------------------------------------------


On Aug. 12, 2011, 1:07 a.m., elguero wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1362/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2011, 1:07 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Security Events Framework was added in 1.8 and support was added for AMI to generate events at that time.
> 
> This patch attempts to add support in chan_sip to generate security events.  Hopefully we can get this into Asterisk 10.
> 
> I am looking forward to hearing feedback on where this patch can be improved especially from those who have an intimate knowledge of chan_sip.
> 
> Thanks
> 
> 
> This addresses bug 18264.
>     https://issues.asterisk.org/jira/browse/18264
> 
> 
> Diffs
> -----
> 
>   /branches/10/channels/chan_sip.c 331633 
>   /branches/10/configs/logger.conf.sample 331633 
>   /branches/10/CHANGES 331633 
> 
> Diff: https://reviewboard.asterisk.org/r/1362/diff
> 
> 
> Testing
> -------
> 
> Local dev machine and a softphone.  Generated events by using the wrong username, wrong password, wrong auth name, successful authentication.
> 
> 
> Thanks,
> 
> elguero
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110812/dd20ee20/attachment-0001.htm>