[asterisk-dev] [Code Review] Security Event Framework Proposal

Tue Jun 16 12:36:47 CDT 2009

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/273/#review854
-----------------------------------------------------------


/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/273/#comment2075>

    I believe you can save yourself a getsockname() call by setting sin_local to pvt->socket.tcptls_session->parent->local_address


/trunk/doc/tex/security-events.tex
<http://reviewboard.digium.com/r/273/#comment2076>

    After Kevin did this work, he realized that in Asterisk lingo, he actually added dynamic logger "levels," not "channels." Logger channel refers to a destination for log messages. Logger level refers to the type of message being logged.
    
    It is best to maintain consistent terminology so that the documentation does not cause confusion.


/trunk/doc/tex/security-events.tex
<http://reviewboard.digium.com/r/273/#comment2077>

    The sections don't appeared to be numbered. Perhaps you should refer to the section by name instead.


/trunk/doc/tex/security-events.tex
<http://reviewboard.digium.com/r/273/#comment2078>

    You should spell out the word "invalid" here and in other places in this document where you have abbreviated it to "inval." The same goes for other abbreviated words, unless the abbreviation is the proper name for an item.


/trunk/doc/tex/security-events.tex
<http://reviewboard.digium.com/r/273/#comment2079>

    What is the difference between these two events?


/trunk/doc/tex/security-events.tex
<http://reviewboard.digium.com/r/273/#comment2080>

    These values should be consistent with the names of the security events in the previous section.


/trunk/doc/tex/security-events.tex
<http://reviewboard.digium.com/r/273/#comment2081>

    This sentence is a bit weird. Remove the word "to" and it will be better.


/trunk/include/asterisk/security_events_defs.h
<http://reviewboard.digium.com/r/273/#comment2082>

    When making an enum that functions as a bitfield, it does not make sense to define a field that is equal to 0.


/trunk/include/asterisk/security_events_defs.h
<http://reviewboard.digium.com/r/273/#comment2083>

    It seems like these same fields are defined in every single security event. I think it would make sense to separate these into a structure of their own or possibly even make them part of the ast_security_event_common structure.


/trunk/main/event.c
<http://reviewboard.digium.com/r/273/#comment2084>

    I am not in favor of this change, nor of any of the similar changes to use ARRAY_LEN in this file.


/trunk/main/event.c
<http://reviewboard.digium.com/r/273/#comment2085>

    This is a bit of a nit, but the old way was more type-safe than the new way. (feel free to ignore this note)


/trunk/main/manager.c
<http://reviewboard.digium.com/r/273/#comment2087>

    I'll knock out two birds with one stone.
    
    1) There's no reason to initialize sin_local in these functions.
    
    2) What's the benefit of using ast_str objects here? I'd think using a char * and ast_strdupa would require less overhead and provide the same benefit. If you're trying to limit the output to a certain length, then a static buffer and ast_copy_string would work, too.


/trunk/main/manager.c
<http://reviewboard.digium.com/r/273/#comment2086>

    I notice that there are very similar functions in manager.c, chan_sip.c, and test_security_events.c for reporting security events. This seems like a maintenance headache if the events should change formats. Perhaps there should be a series of public API calls to generate security events. They may require a massive amount of arguments, but it would also decrease code duplication.


/trunk/main/manager.c
<http://reviewboard.digium.com/r/273/#comment2088>

    I know this isn't something new you've added, but since password was returned by astman_get_header, it is guaranteed to be non-NULL.


/trunk/main/security_events.c
<http://reviewboard.digium.com/r/273/#comment2089>

    Couldn't this be implemented as an O(1) lookup? As long as you bounds-check beforehand, it should work.


- Mark


On 2009-06-16 11:18:17, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/273/
> -----------------------------------------------------------
> 
> (Updated 2009-06-16 11:18:17)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a security framework for Asterisk.  This is essentially the documentation and implementation of the ideas discussed at a couple of the recent developer meetings.  The documentation resides in doc/tex/security-events.tex.
> 
> The code is an implementation of this framework.  The changes can be broken down in this way:
> 
>    1) Security event API
>       - main/security_events.c
>       - include/asterisk/security_events.h
>       - include/asterisk/security_events_defs.h
> 
>    2) Changes to the ast_event API to support security events
>       - include/asterisk/event_defs.h
>       - main/event.c
> 
>    3) A consumer of security events to produce a security log file
>       - res/res_security_log.c
> 
>    4) A completed producer of security events
>       - main/manager.c
> 
>    5) The beginning of having chan_sip produce some security events
>       ******************
>       *** NOTE: I do not propose that this part get merged now.  I think we
>       *** should handle the chan_sip mods as another patch in a second phase.
>       ******************
>       - channels/chan_sip.c
> 
>    6) A test module that generates every type of security event
>       - tests/test_security_events.c
> 
>    7) A simple test script that gets the manager interface to generate
>       one of every type of security event it emits.
>       **************
>       *** NOTE: Is this worth merging?
>       **************
>       - tests/test_ami_security_events.sh
> 
> 
> The security event API is essentially a helper API on top of the ast_event API.  I knew going in to this that there was a lot of data that we wanted in each event.  Forcing producers to use really big ast_event_new() calls, and forcing them to do the payload formatting seemed very error prone and more difficult then necessary.  So, I came up with this API that uses structure definitions of each event type and code in the core that converts these structures into events.  The code also detects if a producer of events forgot to fill in a field that was required.  Also, I put structure version fields in each of these helper structures for the sake of ABI protection.
> 
> As a final note, one notable feature that is not yet present is the ability to fire off custom security events from the dialplan.  I haven't come up with an interface for it that I am happy with just yet.  I think we can handle this as another patch later.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 200840 
>   /trunk/doc/tex/asterisk.tex 200840 
>   /trunk/doc/tex/security-events.tex PRE-CREATION 
>   /trunk/include/asterisk/event_defs.h 200840 
>   /trunk/include/asterisk/security_events.h PRE-CREATION 
>   /trunk/include/asterisk/security_events_defs.h PRE-CREATION 
>   /trunk/main/event.c 200840 
>   /trunk/main/manager.c 200840 
>   /trunk/main/security_events.c PRE-CREATION 
>   /trunk/res/res_security_log.c PRE-CREATION 
>   /trunk/tests/test_ami_security_events.sh PRE-CREATION 
>   /trunk/tests/test_security_events.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/273/diff
> 
> 
> Testing
> -------
> 
> There are two major components of the testing that has been done:
> 
>    1) There is a test module that generates every type of security event.  You can run the test CLI command and verify that the events come out in the custom security log channel, demonstrating the documented security log format:
> 
> Here is some example output:
> 
> *CLI> securityevents test generation
> 
> ...
> 
> SECURITY[17921]: res_security_log.c:125 security_event_cb: SecurityEvent="FailedACL",Service="TEST",EventVersion="1",AccountID="Username",SessionID="Session123",LocalAddress="IPV4/UDP/192.168.1.1/12121",RemoteAddress="IPV4/UDP/192.168.1.2/12345",Module="test_security_events",ACLName="TEST_ACL",SessionTV="1244131376-695232"
> 
> ...
> 
>    2) There is also a script that gets the Asterisk Manager Interface to produce at least one of every type of security event that it produces.  This has been executed and the output has been verified to be what is expected.
> 
> 
> Thanks,
> 
> Russell
> 
>