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

[elguero]

> On Aug. 12, 2011, 2:14 a.m., Tilghman Lesher wrote:
> > /branches/10/channels/chan_sip.c, line 1659
> > <https://reviewboard.asterisk.org/r/1362/diff/1/?file=18288#file18288line1659>
> >
> >     These functions should probably be prefixed with sip_ and added to the sip header file, so they can be called from the other sip code files.

That sounds good.  Done.


> 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.

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.


> 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.

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.


> 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.

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?


> On Aug. 12, 2011, 2:14 a.m., Tilghman Lesher wrote:
> > /branches/10/channels/chan_sip.c, lines 25083-25085
> > <https://reviewboard.asterisk.org/r/1362/diff/1/?file=18288#file18288line25083>
> >
> >     Similarly, another case for invalid password.  In a scan attack, any found peer will likely get this same event multiple times, indicating a real problem to the security event watcher.

Same as prior comment.  What do you think of using failed_acl and passing in "Peer Not Dynamic"?


> 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.

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?


- elguero


-----------------------------------------------------------
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/728af160/attachment-0001.htm>