Rust add external parsers 2 #2794

chifflier · 2017-06-20T14:07:28Z

Make sure these boxes are signed before submitting your Pull Request -- thank you.

I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
I have signed the Open Information Security Foundation contribution agreement at https://suricata-ids.org/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Describe changes:

This PR adds the '--enable-rust-external' configure flag, for (embedded) rust app layers with external dependencies
It adds support for the NTP protocol, implemented in Rust. The decoding logic is embedded, only the raw parser is an external rust crate (https://github.com/rusticata/ntp-parser)
The parser is mostly used as an example, and does protocol validation for NTP version 3 and 4.
Some event rules have been added, to detect malformed NTP data
This is meant to be an example parser, before working on similar integration for IKEv2 (and later, SNMP, SSH alternative parser, etc.)
All comments from Rust add external parsers 1 #2792 have been merged: directories, debug statements, build warnings, rules sids

Thanks,
Pierre

jasonish · 2017-06-20T14:49:46Z

Should we really have --enable-rust-external? I'm unclear what it gets us. What happens if we use an external nom parser for something more critical like TLS or HTTP2? Would these be wrapped behind --enable-rust-external as well?

inliniac · 2017-06-20T15:23:11Z

The --enable-rust-external is meant to be a clear separation between the parser that we develop/bundle, review and support on one side, and the parsers that depend on (potentially lots of) external crates that are developed by third parties, have different licenses and ownership, different goals and release schedules on the other side. I consider it an experiment within the Rust experiment. Perhaps in the future we can do away with this, but for now I want to keep things manageable and cleanly separated.

This will also mean that 'core' code cannot depend on these modules for now.

jasonish · 2017-06-20T15:33:02Z

Just to be a bit pedantic, NFS and DNS rely on external crates that are developed by third parties, as well. The name to me means external Rust code will not be pulled in (a technical thing). Where it is actually a support level type of thing. If its short term, its probably OK. If its going to exist long term, perhaps the name needs a rethink? --enable-rust-experimental to enable experimental rust modules?

inliniac · 2017-06-20T15:36:33Z

I'm ok with finding another name if that makes things more clear.

inliniac · 2017-06-20T15:47:27Z

In general, with Rust and C all the same, we need to be careful about accepting external dependencies. Every one of them is a liability. I've spent countless hours on working around issues in libraries we use. We also see problems with original developers going away, libhtp is the biggest example for that, but also lua plugins that are buggy but unmaintained, etc.

In my mind something like nom and crc is more comparable to libpcre, libnss, etc. General purpose libraries/frameworks that are (hopefully) well maintained. While the protocol dissectors, at this stage, are more comparable to libhtp. Small separate projects that are much more likely to need updates by us if we would depend on them. That's why we maintain libhtp, we need to change things in there too often.

Maybe things will be better because of cargo, but it remains to be seen how distro's handle packaging. Already some crates are broken on Ubuntu LTS' version of rust.

chifflier · 2017-06-20T15:52:17Z

Hi,
I used the external keywords for the reasons Victor described. But it's also true that it also represents the experimental side of the parser, in some way. External crates often need to have exact version requirements to avoid breaking when the the external crate is updated and breaks the API.
If you think another name is better please tell me, it's quite trivial to change.

Some random thoughts:
I think Rust support is already marked as experimental, so adding one layer of experimental will just make documentation a bit weird :) Maybe something like -staging ? Or, if the Rust support becomes mainstream, rename it at that moment ?

jasonish · 2017-06-20T15:59:07Z

Sounds like in the long run, given that external crates are going to have different licenses, and different levels of quality, that a more fine grained approach might be needed, such as:

--enable-ntp
--enable-ike

And so on, rather than having one flag pull in a grab bag of licenses. This can probably wait though.

Note: I've left "rust" out of the examples above. An end user will need to know that Rust is required to build Suricata (when it becomes required). They do not need to know, and should not need to care that a feature is written in Rust or C.

But I think my main issue with the name "external" now has nothing to do with licenses, support, etc. It just sounds like it disables pulling in any external Rust code.

chifflier · 2017-06-21T06:45:03Z

Ok, so what do you prefer (for this pull request, not on the long term):

something like --enable-rust-experimental ?
something like --enable-rust-third-party, --enable-rust-external or --enable-rust-unsupported to emphasize on the fact there is a part not supported by OISF
any other idea

I have no clear preference, I would just like to avoid one word to block the PR :)

inliniac · 2017-06-21T10:44:04Z

src/app-layer-ntp.c

+
+    /* Check if NTP UDP detection is enabled. If it does not exist in
+     * the configuration file then it will be enabled by default. */
+    if (AppLayerProtoDetectConfProtoDetectionEnabled("tcp", proto_name)) {


tcp used instead of udp

inliniac · 2017-06-21T10:44:25Z

src/app-layer-ntp.c

+        }
+        else {
+
+            if (!AppLayerProtoDetectPPParseConfPorts("tcp", IPPROTO_UDP,


inliniac · 2017-06-21T10:44:43Z

src/app-layer-ntp.c

+
+    if (AppLayerParserConfParserEnabled("udp", proto_name)) {
+
+        SCLogNotice("Registering NTP protocol parser.");


inliniac · 2017-06-21T10:44:58Z

src/app-layer-ntp.c

+            NTPGetEvents);
+    }
+    else {
+        SCLogNotice("NTP protocol parsing disabled.");


inliniac · 2017-06-21T10:45:32Z

rules/ntp-events.rules

@@ -0,0 +1,3 @@
+alert ntp any any -> any any (msg:"SURICATA NTP malformed request data"; flow:to_server; app-layer-event:ntp.malformed_data; classtype:protocol-command-decode; sid:2222001; rev:1;)


can you add a reference to the sid assignment page?

inliniac · 2017-06-21T10:47:53Z

rust/src/ntp/ntp.rs

+    }
+}
+
+impl Drop for NTPTransaction {


do you see this being used? I think all tx frees are explicit, or at least they should be as the C-side is responsible.

The Drop Trait is called when the object is destroyed, this is a destructor.

Transactions are freed here:
https://github.com/inliniac/suricata/pull/2794/files#diff-1652b119a6a57d40eb26e9b50c527dcdR115
I made the self.transactions.clear() explicit, but it is not required (vector is cleared when the object becomes out of reach).

When C calls rs_ntp_state_free, the state is freed, automatically freeing the transactions.
The other code path to free a transaction is rs_ntp_state_tx_free, which gets the object in a temporary variable dropped immediately (calling the Drop Trait).
See free_tx:
https://github.com/inliniac/suricata/pull/2794/files#diff-1652b119a6a57d40eb26e9b50c527dcdR134

inliniac · 2017-06-21T10:51:17Z

On 21-06-17 08:45, Pierre Chifflier wrote: Ok, so what do you prefer (for this pull request, not on the long term): * something like --enable-rust-experimental ? * something like --enable-rust-third-party, --enable-rust-external or --enable-rust-unsupported to emphasize on the fact there is a part not supported by OISF * any other idea I have no clear preference, I would just like to avoid one word to block the PR :)

No strong preference on my end. So Jason if you have a strong preference let us know, otherwise I'm just going to accept as-is for now.

jasonish · 2017-06-21T15:20:35Z

On 21-06-17 08:45, Pierre Chifflier wrote:
Ok, so what do you prefer (for this pull request, not on the long term):

something like --enable-rust-experimental ?

something like --enable-rust-third-party, --enable-rust-external or
--enable-rust-unsupported to emphasize on the fact there is a part
not supported by OISF

any other idea

I have no clear preference, I would just like to avoid one word to block
the PR :)
No strong preference on my end. So Jason if you have a strong preference
let us know, otherwise I'm just going to accept as-is for now.
Merge state
Show all checks

I think I'd lean to towards --enable-rust-experimental? Reason being, in the past 2 days I've referred to external crates 2 times now, in reference to nom, and a time handling one I thought about pulling but might not be needed now since Ubuntu updated its Rust. Though I suppose saying "crate" and "external" together is redundant.

chifflier · 2017-06-21T20:06:39Z

Replaced by #2798

chifflier added 6 commits June 20, 2017 13:00

Autotools: add switch to build rust parsers with external dependencies

1ee66cd

Rust gen-c-headers: keep 'const' attribute

a33337e

Add NTP parser (rust-external)

654e4fc

NTP: change parse function to return the number of parsed messages

c3264fc

Add event rules for NTP events

e81d829

Add NTP event rules (commented) to the default config

c4a08ab

inliniac reviewed Jun 21, 2017

View reviewed changes

src/app-layer-ntp.c

}

else {

if (!AppLayerProtoDetectPPParseConfPorts("tcp", IPPROTO_UDP,

Copy link

Contributor

inliniac Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tcp

inliniac reviewed Jun 21, 2017

View reviewed changes

src/app-layer-ntp.c

NTPGetEvents);

}

else {

SCLogNotice("NTP protocol parsing disabled.");

Copy link

Contributor

inliniac Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notice

inliniac reviewed Jun 21, 2017

View reviewed changes

inliniac mentioned this pull request Jun 21, 2017

Rust add external parsers 1 #2792

Closed

3 tasks

chifflier mentioned this pull request Jun 21, 2017

Rust add external parsers 3 #2798

Closed

3 tasks

chifflier closed this Jun 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust add external parsers 2 #2794

Rust add external parsers 2 #2794

chifflier commented Jun 20, 2017

jasonish commented Jun 20, 2017

inliniac commented Jun 20, 2017

jasonish commented Jun 20, 2017

inliniac commented Jun 20, 2017

inliniac commented Jun 20, 2017

chifflier commented Jun 20, 2017

jasonish commented Jun 20, 2017

chifflier commented Jun 21, 2017

inliniac Jun 21, 2017

inliniac Jun 21, 2017

inliniac Jun 21, 2017

inliniac Jun 21, 2017

inliniac Jun 21, 2017

inliniac Jun 21, 2017

chifflier Jun 21, 2017

inliniac commented Jun 21, 2017 via email

jasonish commented Jun 21, 2017

chifflier commented Jun 21, 2017


		if (AppLayerParserConfParserEnabled("udp", proto_name)) {

		SCLogNotice("Registering NTP protocol parser.");

		@@ -0,0 +1,3 @@
		alert ntp any any -> any any (msg:"SURICATA NTP malformed request data"; flow:to_server; app-layer-event:ntp.malformed_data; classtype:protocol-command-decode; sid:2222001; rev:1;)

Rust add external parsers 2 #2794

Rust add external parsers 2 #2794

Conversation

chifflier commented Jun 20, 2017

jasonish commented Jun 20, 2017

inliniac commented Jun 20, 2017

jasonish commented Jun 20, 2017

inliniac commented Jun 20, 2017

inliniac commented Jun 20, 2017

chifflier commented Jun 20, 2017

jasonish commented Jun 20, 2017

chifflier commented Jun 21, 2017

inliniac Jun 21, 2017

Choose a reason for hiding this comment

inliniac Jun 21, 2017

Choose a reason for hiding this comment

inliniac Jun 21, 2017

Choose a reason for hiding this comment

inliniac Jun 21, 2017

Choose a reason for hiding this comment

inliniac Jun 21, 2017

Choose a reason for hiding this comment

inliniac Jun 21, 2017

Choose a reason for hiding this comment

chifflier Jun 21, 2017

Choose a reason for hiding this comment

inliniac commented Jun 21, 2017 via email

jasonish commented Jun 21, 2017

chifflier commented Jun 21, 2017