Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

response: fix connect tunneling bug - v3 #426

Draft
wants to merge 2 commits into
base: 0.5.x
Choose a base branch
from
Draft

response: fix connect tunneling bug - v3 #426

wants to merge 2 commits into from

Conversation

cccs-sadugas
Copy link
Contributor

The response was emitting partial body data depending on how you fed the parser with inbound and outbound data chunks. It seems the intended behavior is to not emit body data if HTP_STREAM_TUNNEL will eventually be entered (please correct if mistaken).

The fix was to allow htp_connp_REQ_CONNECT_WAIT_RESPONSE to resume in order to enter the HTP_STREAM_TUNNEL or complete the request.

The tunneling transaction was also incomplete because the request side wasn't being finalized after entering HTP_STREAM_TUNNEL.

See test case for example.

@cccs-sadugas
response: add http tunneling test case
559667a 
This test case shows unexpected state transitions when processing http
tunnels and the emission of body data.
@cccs-sadugas
response: fix connect tunneling bug
4d33275 
The response was emitting partial body data depending on how you
fed the parser with inbound and outbound data chunks. It seems the
intended behavior is to not emit body data if HTP_STREAM_TUNNEL
will eventually be entered.

The fix was to allow htp_connp_REQ_CONNECT_WAIT_RESPONSE to resume in
order to enter the HTP_STREAM_TUNNEL state or complete the request.

The tunneling transaction was also incomplete because the
request side wasn't being finalized after entering HTP_STREAM_TUNNEL.

See test case for example.
@catenacyber
Copy link
Contributor

Thanks @cccs-sadugas

Would you have a redmine ticket for this ? Suricata-verify test ?

@catenacyber
Copy link
Contributor

Running suricata on the pcap created by the test, I see this diff

{                                                                       {
  "event_type": "http",                                                   "event_type": "http",
  "tx_id": 0,                                                             "tx_id": 0,
  "http": {                                                               "http": {
    "hostname": "www.ssllabs.com",                                          "hostname": "www.ssllabs.com",
    "http_port": 443,                                                       "http_port": 443,
    "url": "www.ssllabs.com:443",                                           "url": "www.ssllabs.com:443",
    "http_method": "CONNECT",                                               "http_method": "CONNECT",
    "protocol": "HTTP/1.0",                                                 "protocol": "HTTP/1.0",
    "status": 200,                                                          "status": 200,
    "length": 0                                                 |           "length": 15
  }                                                                       }
}                                                                       }
{                                                                       {
  "event_type": "flow",                                                   "event_type": "flow",
  "src_ip": "127.0.0.1",                                                  "src_ip": "127.0.0.1",
  "src_port": 57207,                                                      "src_port": 57207,
  "dest_ip": "127.0.0.1",                                                 "dest_ip": "127.0.0.1",
  "dest_port": 8001,                                                      "dest_port": 8001,
  "proto": "TCP",                                                         "proto": "TCP",
  "app_proto": "http",                                          |         "app_proto": "failed",
                                                                >         "app_proto_orig": "http",

I am not sure the fix is correct, "app_proto": "failed", looks better to me...

As for the length, the PR looks to improve it...

So, I guess I need to dig into this app_proto diff

@catenacyber
Copy link
Contributor

This delays the callback HTPCallbackResponseComplete

catenacyber
catenacyber reviewed Jul 5, 2024
View reviewed changes
htp/htp_response.c
// was not suspended. Otherwise, yield back to inbound processing in
// case we've suspended because of a CONNECT transaction and are about
// to enter a tunneled state where we won't process body data.
htp_status_t rc = HTP_DATA_OTHER;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why HTP_DATA_OTHER ? This prevents the completion of the transaction...
HTP_OK looks better, does it not ?

@simdugas
Copy link

Thanks for the initial feedback! I will try to create a redmine ticket, look at comments, and do more testing with Suricata in the next week or two.

@catenacyber
Copy link
Contributor

Hello @simdugas will you work on this again ?

@simdugas
Copy link

Hello @simdugas will you work on this again ?

Yes, as soon as I find spare time to do so. Sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catenacyber catenacyber catenacyber left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cccs-sadugas @catenacyber @simdugas