WIP: Use libhtp-rs v4

[cccs-rtmorti]

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.io/about/contribution-agreement/
• I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket: 2696

Describe changes:

• Use libhtp-rs.
• Rebased from WIP: Libhtp rs v3 #8255
• A single commit (for now, at least), since separating the API renames / changes from some of the other changes is tricky, and one commit makes it easier to work with for the rest of the CI.

libhtp-repo: https://github.com/cccs-rtmorti/libhtp-rs
libhtp-branch: main

[catenacyber]

Looking better :-)
I see some red builder because of #include <htp/htp.h> not found
Should this include be removed ?

[cccs-rtmorti]

Looking better :-) I see some red builder because of #include <htp/htp.h> not found Should this include be removed ?

Yeah, I am working through the build failures now and seeing what's up. I actually don't understand this one: the file htp/htp.h should be generated and dropped into rust/gen right next to rust-bindings.h, so I am not sure why it's not being found on that platform. You can see it builds fine on Fedors36 and some others without issue.

I am also seeing a suricata-verify fail on Fedora36 (Suricata verify), but this test passes for me here, so I am not sure what the problem is.

Will keep working through the red ones and pushing fixes. :)

[catenacyber]

When they finish, you can retry failed builds.

There are some S-V tests that fail sometimes due to some different workload...

This must be an issue about includes paths, try changing to "htp/htp.h" with quotes instead of brackets (as the include is now local to the project, right ?)

[cccs-rtmorti]

This must be an issue about includes paths, try changing to "htp/htp.h" with quotes instead of brackets (as the include is now local to the project, right ?)

Oh, that is almost certainly it! :)

[catenacyber]

Only 3 more failures to go : macOS needs to link a special lib ? fuzz found a null deref ? And fix make distclean somehow...

[cccs-rtmorti]

Only 3 more failures to go : macOS needs to link a special lib ? fuzz found a null deref ? And fix make distclean somehow...

One down, two to go. Currently trying to reproduce the fuzz failure.

[cccs-rtmorti]

So the failing fuzz test is from GetDetectTx() in src/detect.c:1268:

static DetectTransaction GetDetectTx(const uint8_t ipproto, const AppProto alproto,
        void *alstate, const uint64_t tx_id, void *tx_ptr, const int tx_end_state,
        const uint8_t flow_flags)
{
    uint64_t detect_flags;
    AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx_ptr);
    if (likely(txd != NULL)) {
        detect_flags = (flow_flags & STREAM_TOSERVER) ? txd->detect_flags_ts : txd->detect_flags_tc;
    } else {
        detect_flags = 0;
    }
[...]
    const int tx_progress = AppLayerParserGetStateProgress(ipproto, alproto, tx_ptr, flow_flags);
    const int dir_int = (flow_flags & STREAM_TOSERVER) ? 0 : 1;
    DetectEngineState *tx_de_state = txd->de_state;

The segfault is on the last line here. It is dereferencing txd, but txd is NULL. txd is NULL because there was no AppLayerTxData stored in the transaction previously. Note that we explicitly check for txd being NULL after we fetch it, and set detect_flags = 0 if it is NULL. Later we unconditionally dereference it on the last line there.

The input in question is this:

<<<
HTTP/1.1 101 Switching Protocols
Connection: Upgrade
Upgrade: h2c


>>>
PRI * HTTP/2.0

SM

I am checking into how libhtp-c and libhtp-rs differ on this input and see why one works and the other doesn't, and will fix any differences. But while I am figuring that out, I wanted to ask about whether or not we should support txd being NULL here? It looks like we have a check for txd == NULL above, but then expect it to be not NULL later. Is this correct? Should we do something else if we detect txd = NULL here?

[catenacyber]

I am checking into how libhtp-c and libhtp-rs differ on this input and see why one works and the other doesn't, and will fix any differences.

Looks like the good thing to do :-)

I wanted to ask about whether or not we should support txd being NULL here?

Ted should become a mandatory part of a transaction (whatever the protocol), but libhtp is the only thing standing in the way for now...

cf https://redmine.openinfosecfoundation.org/issues/4952#note-2

If you feel like implementing https://redmine.openinfosecfoundation.org/issues/5739#note-1 be our guest :-)

[cccs-rtmorti]

I am checking into how libhtp-c and libhtp-rs differ on this input and see why one works and the other doesn't, and will fix any differences.

Looks like the good thing to do :-)

So what is happening here is this:

• libhtp detects the HTTP/2 tunnel and sets both ends to state TUNNEL
• Suricata later calls htp_connp_req_close(), and this puts the request side into state CLOSED and pushes an empty data input through htp_connp_req_data() in order to finalize the parsers. This ends up in htp_connp_REQ_IDLE, which sees the empty chunk and asks for more data. In the request data processing loop this sets the stream state to HTP_STREAM_DATA, which is essentially 're-opening' the connection, after it has already been in TUNNEL and been explicitly set to CLOSED by Suricata.
• Request data is pushed into the parser, and it is parsed normally. This is the client HTTP/2 preamble: PRI * HTTP/2.0.... After this, the parser is just proceeding normally.
• A gap is pushed into the request parser. The parser interprets this as a gap in the leading part of the next transaction. So it sets the next transaction to state GAP on the request side. This lets the caller know that this transaction exists because we know we lost something on the wire that would have been HTTP data, but we lost it. Crucially, there is no callback to suricata's HTPCallbackRequestStart() because there is nothing to handle. This is why there is no Transaction data in suricata at the time, and why it segfaults later.

As near as I can tell, libhtp-rs and libhtp-c have the same behaviour right up to the last step. libhtp-c sees the gap and just returns HTP_STREAM_CLOSED here. I am not sure if this is the most correct behaviour, but it is why there is no segfault later.

I can fix this a number of ways, but I am not sure which one is the most correct, so I wanted to ask your opinion.

1. If the stream is in state TUNNEL, do not later change to state CLOSED, even if the caller asks for it.
2. If the stream is in state CLOSED, do not later allow transitions back to parsing.
3. If there is a leading gap in a transaction, perform the HTPCallbackRequestStart callback even though there is nothing to see.
4. If there is a leading gap in a transaction, just ignore it and say the stream is closed (this is what libhtp-c seems to do).

I think 3 is probably the "most correct" way, since it acknowledges there is something on the wire, but we just don't know what it is. It is possible that a response comes through for this later and maybe we can do something with it, and it also allows the caller to set up any framing or anything they want to do to track the data flow. I can also see 1 being reasonable, but this opens the parser up to evasion where the attacker pretends to be in a tunnel, but then just proceeds normally. 2 is also reasonable, since the caller earlier asked that the stream be closed, but then sent more data anyway, so there shouldn't be any expectation on the parser to actually do something with it. 4 is practically correct, since we can't do anything with a leading gap anyway, but also ignores the fact that there was something there, and the transaction isn't created as a placeholder to acknowledge the existence of something on the wire at that spot.

Thoughts?

[cccs-rtmorti]

Okay, only one test left, and it's one of the chrono dependencies that is failing on MacOS. I think it will be simpler to just switch to time since it is already a dep in suricata, so I am working on that.

[cccs-rtmorti]

CI is all green. :-)

[catenacyber]

CI : ✅
Code : I need to pat a closer look. Do you have a script to minimize the difference without all the constants renaming ?
Still needs to revert the change about the maximum chunk size ;-) cf differential fuzzing
Commits segmentation : I guess it is ok, maybe needs some refining if possible...
Commit messages : Really nice. Still missing a reference to a ticket in redmine
Git ID set : looks fine for me
CLA : I do not have access, but looks like some commits were already merged
Doc update : I guess there needs to be some changes + an addition in doc/userguide/upgrade.rst
Redmine ticket : To be done
Rustfmt : was not enforced
Tests : green CI is good
Dependencies added: License to figure out cc @victorjulien

[catenacyber]

Hey Todd, thanks for your work, here are some quick remarks about polishing the PR...
Could you open a new PR for the v5 ?

[catenacyber]

BSD3 License looks good ;-)

[cccs-rtmorti]

Code : Do you have a script to minimize the difference without all the constants renaming ?

I do not have such a script. I could write one - it would just be a bunch of sed commands to 'unrename' the constants. The resulting code would not compile, and you would have to look at the diff outside of github.

Still needs to revert the change about the maximum chunk size ;-) cf differential fuzzing

This is done, no?

cccs-rtmorti/libhtp-rs@2aef042

The maximum accepted chunk size is std::i32::MAX, like libhtp-c

Commits segmentation : I guess it is ok, maybe needs some refining if possible...

Yeah, I can have another look at this, but it's hard to pull things apart.

Commit messages : Really nice. Still missing a reference to a ticket in redmine

Oops. It's redmine 2696: https://redmine.openinfosecfoundation.org/issues/2696

CLA : I do not have access, but looks like some commits were already merged

I did sign the CLA. :)

Doc update : I guess there needs to be some changes + an addition in doc/userguide/upgrade.rst

I will have a look. I think in upgrade.rst it should be mentioned that libhtp is no longer built separately, so downstream packagers no longer need to include it as a dep or part of their environments. (?) Otherwise, the configuration options and such are the same - from a user point of view there should not be much difference? Anything else that would normally be mentioned?

[catenacyber]

Code : Do you have a script to minimize the difference without all the constants renaming ?

I do not have such a script. I could write one - it would just be a bunch of sed commands to 'unrename' the constants. The resulting code would not compile, and you would have to look at the diff outside of github.

Yes, that is what I would enjoy...
If you have a csv with the constants renaming, I can work on it

Still needs to revert the change about the maximum chunk size ;-) cf differential fuzzing

This is done, no?

cccs-rtmorti/libhtp-rs@2aef042

The maximum accepted chunk size is std::i32::MAX, like libhtp-c

Indeed

Commits segmentation : I guess it is ok, maybe needs some refining if possible...

Yeah, I can have another look at this, but it's hard to pull things apart.

Do not spend too much effort on it yet ;-)

Commit messages : Really nice. Still missing a reference to a ticket in redmine

Oops. It's redmine 2696: https://redmine.openinfosecfoundation.org/issues/2696

Could you put this in the commit message like Ticket: #2696

Doc update : I guess there needs to be some changes + an addition in doc/userguide/upgrade.rst

I will have a look. I think in upgrade.rst it should be mentioned that libhtp is no longer built separately, so downstream packagers no longer need to include it as a dep or part of their environments. (?) Otherwise, the configuration options and such are the same - from a user point of view there should not be much difference? Anything else that would normally be mentioned?

You can try git grep libhtp | cut -d: -f1 | uniq to see which files refer to libhtp... There may be some scripts or .github files to change as well ;-)

[cccs-rtmorti]

Code : Do you have a script to minimize the difference without all the constants renaming ?

I do not have such a script. I could write one - it would just be a bunch of sed commands to 'unrename' the constants. The resulting code would not compile, and you would have to look at the diff outside of github.

Yes, that is what I would enjoy... If you have a csv with the constants renaming, I can work on it

No problem. When I wrote the commit message I went through and found all the constants that changed, so turning that into a csv is no problem.

constants.csv

[cccs-rtmorti]

You can try git grep libhtp | cut -d: -f1 | uniq to see which files refer to libhtp... There may be some scripts or .github files to change as well ;-)

Will do! :)

[catenacyber]

Thanks for the csv
Are you planning on a next version soon ? Or should I do some code review first ?

[cccs-rtmorti]

I am planning to go through the docs tomorrow, and then I will update with a new PR v5 that includes the other changes.

If you want to do some review, that would be great - I can incorporate anything else at the same time.

[catenacyber]

I will wait for v5 first then :-)

[cccs-rtmorti]

Opened v5 in #8335