(Cc'ing bro-dev, I suggest we continue the thread there).
This sounds generally reasonable, however I think we could take the
opportunity here to generalize this a bit more for generally including
link-layer information into connection handling.
One thing that I didn't quite get form your description is if the
objective is really just to get the VLAN ID into conn.log, or whether
you also want to use it for defining what constitutes a connection in
the first place. The latter would aim at the situations where the same
IP addresses can appear on different VLANs for independent
connections. Right now, Bro can't keep them apart, but if we made the
VLAN part of the connection index into the session table, it would
treat them separately. Same applies to other link-level features that
could sometimes be useful to be a part of a connection's ID (like MAC
addresses).
With that in mind, some thoughts on generalizing this (note, I not
sureif you're working from 2.3 or git master. The PktSrc API has
changed a bit recently, I'll take git as my starting point).
- One challenge is passing the the VLAN ID through to the various
packet-related methods. You're suggesting additional parameters,
which would work. However, these methods are already taking a
bunch of parameters, and if in the future we wanted to pass
through further link-layer info, we'd have to add even more. A
more flexible alternative would be switching to simply passing a
Packet structure around that encapsulates all the information,
including what's already there (e.g., timestmap, pcap_hdr,
payload, etc.). The new PktSrc API already has such a class:
PktSrc::Packet; from a quick look I think we could elevate that
to be something passed around more generally, and then extend it
accordingly.
- For the connections, I would store the VLAN inside the ConnID
struct, and then modify BuildConnIDHashKey() to take it into
account. That way, the session table will make it part of its
index. Same for the script-land conn_id record; that will then
make script-level tables work that index by conn_id.
- Extending the ConnID like this could actually be made a run-time
option: I believe it shouldn't be too difficult to let users
chose the fields defining a ConnID, so that they can decide if,
say, they want to VLAN to be in there or not. We could predefine
a set of potential features to choose from, along with some
script-land API to pick the set to use, with the current 4-tuple
being the default. (This could be a 2nd step for later; if the
first two points above were in place, this extension should
become mainly a question of finding the right configuration
interface.)
I haven't thought this thruogh too carefully, so it's conceivable that
I'm missing something. But I think it would be really helpful for many
folks to get more flexibility into the definition of what consitutes a
connection, with VLANs being a good initial target to support.
Robin
On Tue, Apr 14, 2015 at 16:59 +0000, you wrote:
> Dear Bro developers,
>
> I've been tasked with trying to modify the Bro source code so that
> conn.log includes the VLAN IDs (including 802.1ah) that have been observed
> in packets associated with that connection. I've scoped out a solution,
> but I want to run it by you first before I start to go for it, in case I'm
> missing something really big.
>
> PktSrc::Process() does processing of VLAN and 802.1ah, but it just skips
> over them by advancing the data pointer. I will, in addition, store those
> VLAN IDs in a new member of the modified PktSrc class. This gets passed on
> through net_packet_dispatch() and NetSessions::DispatchPacket(). At this
> point NetSessions::NextPacket() gets called, but since the PktSrc doesn't
> get passed to it, I'd need another way to pass it the VLAN ID. I am
> considering two options:
>
> 1. duplicate NextPacket() adding a new parameter to pass it the VLAN IDs,
> and call that instead, or
> 2. store the VLAN IDs in the NetSessions class, in DispatchPacket() so
> it¹s available to NextPacket() and DoNextPacket() <- Is there a reason
> this wouldn¹t work, e.g. issues with multi-threading/multi-processing?
>
> Is there one option that seems better to you?
>
> NetSessions::DoNextPacket() is called next and I would also need a
> modification to pass it VLAN IDs, using one of the options above. In this
> method we finally get access to the appropriate Connection instance, so I
> would store the VLAN IDs in that instance in DoNextPacket().
>
> I'd need to modify the Connection class in Conn.h to include a new member
> for tracking VLAN IDs. I'd modify Connection::BuildConnVal() and
> scripts/base/init-bare.bro's connection record to make the VLAN IDs
> available to scripts. Lastly, I'd write a script to redef the conn Info
> structure and handle one or more connection events (perhaps
> connection_state_remove) to copy the VLAN IDs from the connection record
> to the Info record.
>
> Is there anything I'm missing? Is there a better way to approach this?
>
--
Robin Sommer * ICSI/LBNL * robin(a)icir.org * www.icir.org/robin
Vern Paxson created BIT-1407:
--------------------------------
Summary: -f silently fails if base/frameworks/packet-filter isn't loaded
Key: BIT-1407
URL: https://bro-tracker.atlassian.net/browse/BIT-1407
Project: Bro Issue Tracker
Issue Type: Problem
Components: Bro
Reporter: Vern Paxson
I know we've been through this before (though searching the tickets in Jira, I couldn't find the thread). But to revisit: the "-f filter" option silently does nothing if base/frameworks/packet-filter isn't loaded (so the scenario here is using -b to suppress its automatic loading). This can lead to seriously confusing behavior. It would be preferable if there's either an error message indicating that the option won't be supported, or if it forced loading of packet-filter.
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)
Vern Paxson created BIT-1406:
--------------------------------
Summary: Trouble locating -b documentation
Key: BIT-1406
URL: https://bro-tracker.atlassian.net/browse/BIT-1406
Project: Bro Issue Tracker
Issue Type: Problem
Components: Documentation
Reporter: Vern Paxson
I'm trying to locate the documentation for -b (run Bro in "bare" mode) but the paths I'd expect to work aren't. I don't see it or command line arguments (or flags) in the general index at [https://www.bro.org/sphinx/genindex.html], and at [https://www.bro.org/sphinx/search.html] a search on -b doesn't turn up anything. While the "using Bro from the command-line section" mentions the flag, it doesn't link to any documentation for it.
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)
[ https://bro-tracker.atlassian.net/browse/BIT-1405?page=com.atlassian.jira.p… ]
Vern Paxson updated BIT-1405:
-----------------------------
Description: The [Notice documentation|https://www.bro.org/sphinx/frameworks/notice.html] includes the phrase "_Users should directly make modifications to the Notice::Info record given as the argument to the hook_". Initially I read this with a presumption that it was a typo and what was meant was instead "should *not* directly make ...". Then when I got to the example I see that it actually does mean go-ahead-and-modify, though presumably it only makes sense to modify some fields (such as $actions) and not others (context provided by the Info record). So this phrasing should be clarified, maybe along the lines of "Users alter notice processing by directly modifying certain fields in the Notice::Info record given as the argument ...". (was: The [Notice documentation|https://www.bro.org/sphinx/frameworks/notice.html] includes the phrase "_Users should directly make modifications to the Notice::Info record given as the argument to the hook_". Presumably this is instead "should *not* directly make ...")
Summary: Notice framework documentation confusion (was: Notice framework documentation glitch)
> Notice framework documentation confusion
> ----------------------------------------
>
> Key: BIT-1405
> URL: https://bro-tracker.atlassian.net/browse/BIT-1405
> Project: Bro Issue Tracker
> Issue Type: Problem
> Components: Documentation
> Reporter: Vern Paxson
> Priority: Low
>
> The [Notice documentation|https://www.bro.org/sphinx/frameworks/notice.html] includes the phrase "_Users should directly make modifications to the Notice::Info record given as the argument to the hook_". Initially I read this with a presumption that it was a typo and what was meant was instead "should *not* directly make ...". Then when I got to the example I see that it actually does mean go-ahead-and-modify, though presumably it only makes sense to modify some fields (such as $actions) and not others (context provided by the Info record). So this phrasing should be clarified, maybe along the lines of "Users alter notice processing by directly modifying certain fields in the Notice::Info record given as the argument ...".
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)
Vern Paxson created BIT-1405:
--------------------------------
Summary: Notice framework documentation glitch
Key: BIT-1405
URL: https://bro-tracker.atlassian.net/browse/BIT-1405
Project: Bro Issue Tracker
Issue Type: Problem
Components: Documentation
Reporter: Vern Paxson
Priority: Low
The [Notice documentation|https://www.bro.org/sphinx/frameworks/notice.html] includes the phrase "_Users should directly make modifications to the Notice::Info record given as the argument to the hook_". Presumably this is instead "should *not* directly make ..."
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)
[ https://bro-tracker.atlassian.net/browse/BIT-1404?page=com.atlassian.jira.p… ]
Robin Sommer updated BIT-1404:
------------------------------
Fix Version/s: 2.4
> decompose_uri() builtin throws errors on URIs with select parameters
> --------------------------------------------------------------------
>
> Key: BIT-1404
> URL: https://bro-tracker.atlassian.net/browse/BIT-1404
> Project: Bro Issue Tracker
> Issue Type: Problem
> Components: Bro
> Affects Versions: 2.4
> Reporter: Stephen Hosom
> Assignee: Seth Hall
> Fix For: 2.4
>
>
> URIs with odd query strings cause errors in reporter.log.
> For example:
> local something = decompose_uri("dfasjdfasdfasdf?asd");
> results in:
> error in /usr/local/bro-master/share/bro/base/utils/urls.bro, line 79: no such index (parts[1])
> http://try.bro.org/#/trybro/saved/8505 demonstrates a pretty alright example.
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)
[ https://bro-tracker.atlassian.net/browse/BIT-1404?page=com.atlassian.jira.p… ]
Robin Sommer reassigned BIT-1404:
---------------------------------
Assignee: Seth Hall
> decompose_uri() builtin throws errors on URIs with select parameters
> --------------------------------------------------------------------
>
> Key: BIT-1404
> URL: https://bro-tracker.atlassian.net/browse/BIT-1404
> Project: Bro Issue Tracker
> Issue Type: Problem
> Components: Bro
> Affects Versions: 2.4
> Reporter: Stephen Hosom
> Assignee: Seth Hall
> Fix For: 2.4
>
>
> URIs with odd query strings cause errors in reporter.log.
> For example:
> local something = decompose_uri("dfasjdfasdfasdf?asd");
> results in:
> error in /usr/local/bro-master/share/bro/base/utils/urls.bro, line 79: no such index (parts[1])
> http://try.bro.org/#/trybro/saved/8505 demonstrates a pretty alright example.
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)
[ https://bro-tracker.atlassian.net/browse/BIT-1400?page=com.atlassian.jira.p… ]
Robin Sommer updated BIT-1400:
------------------------------
Fix Version/s: (was: 2.4)
2.5
> topic/jsiwek/mime-multipart-boundary-leniency
> ---------------------------------------------
>
> Key: BIT-1400
> URL: https://bro-tracker.atlassian.net/browse/BIT-1400
> Project: Bro Issue Tracker
> Issue Type: Problem
> Components: Bro
> Reporter: Jon Siwek
> Assignee: Seth Hall
> Fix For: 2.5
>
>
> Seth had a private pcap showing HTTP multipart content using boundary strings containing the '<' and '>' characters which causes HTTP/MIME content parsing to fail. This branch changes it so those characters are allowed (even though not explicitly permitted by the RFC). It feels a bit hacky to me (but so do most changes I've done to HTTP/MIME analyzers), so please review and check if the analysis looks "more correct" now.
> I scheduled this for 2.4 because I think Seth mentioned it might be something to try to get fixed in the final release, but it might be better to put it as part of 2.5 -- it's not really a severe bug but more of an oddity from a particular HTTP implementation and Bro's behavior with respect to it hasn't changed anytime recently (i.e. it's not a regression).
--
This message was sent by Atlassian JIRA
(v6.5-OD-04-052#65000)