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

Tue Jun 16 17:22:49 CDT 2009

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/channels/chan_sip.c, lines 11946-11949
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5633#file5633line11946>
> >
> >     I believe you can save yourself a getsockname() call by setting sin_local to pvt->socket.tcptls_session->parent->local_address

That's hot.  Done.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/doc/tex/security-events.tex, lines 61-64
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5635#file5635line61>
> >
> >     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.

Fixed!

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/doc/tex/security-events.tex, line 72
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5635#file5635line72>
> >
> >     The sections don't appeared to be numbered. Perhaps you should refer to the section by name instead.

section numbers removed

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/doc/tex/security-events.tex, line 106
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5635#file5635line106>
> >
> >     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.

Good suggestion.  The notes scribbled down during discussion became the documentation.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/doc/tex/security-events.tex, lines 123-130
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5635#file5635line123>
> >
> >     What is the difference between these two events?

The first is for when a request is not allowed due to permissions.  For example, if you try to use the Originate AMI action without the originate permission for the user.

The second is for a request that we understand, but do not implement support for.  An example of this would be a SIP PUBLISH.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/doc/tex/security-events.tex, lines 167-170
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5635#file5635line167>
> >
> >     These values should be consistent with the names of the security events in the previous section.

I think they are consistent for the most part.  The last section is describing them in English, while this is the definition of the parseable format that will show up in the log file.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/doc/tex/security-events.tex, line 203
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5635#file5635line203>
> >
> >     This sentence is a bit weird. Remove the word "to" and it will be better.

removed

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/include/asterisk/security_events_defs.h, lines 118-131
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5638#file5638line118>
> >
> >     When making an enum that functions as a bitfield, it does not make sense to define a field that is equal to 0.

Good point.  Fixed.

This actually reminds me that I never implemented the option for the security log module to be able to not log the informational events.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/include/asterisk/security_events_defs.h, lines 402-432
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5638#file5638line402>
> >
> >     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.

Good point.  I think I envisioned these fields not always being there, but they are in all of the currently defined events.  So, I moved them all into the common struct.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/main/event.c, line 308
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5639#file5639line308>
> >
> >     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)

Yeah, I suppose so.  However, the value is no longer stored there.  The value is only there as the index into the array.  So, the index is the only place you can get it from.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/main/event.c, line 245
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5639#file5639line245>
> >
> >     I am not in favor of this change, nor of any of the similar changes to use ARRAY_LEN in this file.

Could you elaborate?  ARRAY_LEN() is always safer than a constant when doing a sanity check on array bounds ...

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/main/manager.c, lines 1757-1758
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5640#file5640line1757>
> >
> >     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.

Actually, ast_strdupa() / ast_copy_string() wouldn't work because the session ID is not coming from a string.

However, your comment still applies as I could use snprintf() on a constant size buffer instead of ast_str_set().

Regarding ast_str versus an array, it's a toss up to me.  I suppose my thinking was that it made sense to always use ast_str unless there was a good reason not to for the sake of consistency.  I figured ast_str was the preferred way to do almost all string operations now, but I suppose it's not something we have discussed specifically.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/main/manager.c, line 1782
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5640#file5640line1782>
> >
> >     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.

I'm not sure that I agree here.  If something in the event changes, it would be to add or remove a field.

With the current code, this is done by adding/removing a field from the appropriate structs, and updating the structure version ID.  There is API breakage if you remove something, but no API breakage if you add something.  Also, this method is ABI safe.

If you went the route of an API built with functions with a ton of arguments, you still have to go through and touch every place that generates the event.  You break the API no matter what the change is.  You also break the ABI.

Finally, in both cases, you must have code that gathers up the proper data to feed to the API.  I'm not convinced that mapping the data into a structure versus mapping the data into function call arguments is really any more code, or more code duplication.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/main/manager.c, line 2035
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5640#file5640line2035>
> >
> >     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.

Cool, fixed.

> On 2009-06-16 12:36:47, Mark Michelson wrote:
> > /trunk/main/security_events.c, lines 359-371
> > <http://reviewboard.digium.com/r/273/diff/5/?file=5641#file5641line359>
> >
> >     Couldn't this be implemented as an O(1) lookup? As long as you bounds-check beforehand, it should work.

I'm not sure how you'd implement it since the value we're using is for a bit field, so we can't use it as an index into an array, unless we waste space.

- Russell

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

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