applayer/htp: convert to new FAIL/PASS API /v5 #11973

Nancyenos · 2024-10-16T07:50:32Z

Ticket: #6935

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

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/

Describe changes:

applayer/htp: convert to new FAIL/PASS API
-Previous pr applayer/htp: convert to new FAIL/PASS API v-4 #11960

Provide values to any of the below to override the defaults.

To use an LibHTP, Suricata-Verify or Suricata-Update pull request,
link to the pull request in the respective _BRANCH variable.
Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Ticket: OISF#6935

github-actions · 2024-10-16T20:13:12Z

NOTE: This PR may contain new authors.

codecov · 2024-10-16T20:31:28Z

Codecov Report

Attention: Patch coverage is 98.87955% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.84%. Comparing base (37fa2a6) to head (ed07b85).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11973      +/-   ##
==========================================
+ Coverage   82.77%   82.84%   +0.06%     
==========================================
  Files         910      910              
  Lines      249016   248530     -486     
==========================================
- Hits       206134   205890     -244     
+ Misses      42882    42640     -242

Flag	Coverage Δ
fuzzcorpus	`60.57% <ø> (-0.21%)`	⬇️
livemode	`18.71% <ø> (ø)`
pcap	`43.94% <ø> (-0.15%)`	⬇️
suricata-verify	`62.23% <ø> (+0.03%)`	⬆️
unittests	`59.09% <98.87%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

jufajardini

I know this is a very big file, with lots of tests. You're brave for picking up this one!

Test HTPBodyReassemblyTest01 still has a mixed style: it hasn't been fully converted to the FAIL/PASS style.

I think I've covered all cases where we need some improvement, still, so you can get a good understanding of what the next steps are.

There is one thing that I asked someone else to do a sanity check. If you are idly waiting, feel free to claim another task, in this case, ok?

jufajardini · 2024-10-18T01:42:16Z

src/app-layer-htp.c

+    FAIL_IF(bstr_ptr(tx_ud->request_uri_normalized)[0] != '/' ||
            bstr_ptr(tx_ud->request_uri_normalized)[1] != '%' ||
            bstr_ptr(tx_ud->request_uri_normalized)[2] != '0' ||
-            bstr_ptr(tx_ud->request_uri_normalized)[3] != '0')
-        {
-            printf("normalized uri \"");
-            PrintRawUriFp(stdout, bstr_ptr(tx_ud->request_uri_normalized), bstr_len(tx_ud->request_uri_normalized));
-            printf("\": ");
-            goto end;
-        }
-    }
+            bstr_ptr(tx_ud->request_uri_normalized)[3] != '0');


let's break each of these into their own FAIL... macros

jufajardini · 2024-10-18T02:03:55Z

src/app-layer-htp.c

-        printf("Could not get config for: %s\n", addr);
-        goto end;
-    }
+    FAIL_IF_NULL(user_data);


As the tests passed, I suppose there's no harm here. But wanted to point out that this changes the original format a bit, since we don't jump to end in this case, even if user_data was NULL

jufajardini · 2024-10-18T02:07:24Z

src/app-layer-htp.c

    if (inet_pton(AF_INET, addr, buf) == 1) {
        (void)SCRadixFindKeyIPV4BestMatch(buf, cfgtree, &user_data);
-        if (user_data != NULL) {
-            HTPCfgRec *htp_cfg_rec = user_data;
-            htp = htp_cfg_rec->cfg;
-            SCLogDebug("LIBHTP using config: %p", htp);
-        }
-        if (htp == NULL) {
-            printf("Could not get config for: %s\n", addr);
-            goto end;
-        }
+        FAIL_IF_NULL(user_data);
+        HTPCfgRec *htp_cfg_rec = user_data;
+        htp = htp_cfg_rec->cfg;
+        SCLogDebug("LIBHTP using config: %p", htp);
+
+        FAIL_IF_NULL(htp);
+
    }


I wonder if we can simplify this to just check:

FAIL_IF_NOT(inet_pton(AF_INET, addr, buf) == 1)

since the else case will simply be a FAIL. What do you think, @inashivb ?

I think we need to check user data and htp config too. So, yes to your suggestion, followed by the rest of the matches too. The block could look like:

FAIL_IF(inet_pton(AF_INET, addr, buf) != 1) (void)SCRadixFindKeyIPV4BestMatch(buf, cfgtree, &user_data); if (user_data != NULL) { HTPCfgRec *htp_cfg_rec = user_data; htp = htp_cfg_rec->cfg; SCLogDebug("LIBHTP using config: %p", htp); FAIL_IF_NULL(htp); }

Then maybe... Would this until make sense?

FAIL_IF(inet_pton(AF_INET, addr, buf) != 1); (void)SCRadixFindKeyIPV4BestMatch(buf, cfgtree, &user_data); FAIL_IF_NULL(user_data); HTPCfgRec *htp_cfg_rec = user_data; FAIL_IF_NULL(htp); htp = htp_cfg_rec->cfg; SCLogDebug("LIBHTP using config: %p", htp);

yeah this was my first choice but then I was trying to make it replicate the current code.. I guess this works though. Let's do this! :)

jufajardini · 2024-10-18T02:09:31Z

src/app-layer-htp.c

+    FAIL_IF(bstr_ptr(tx_ud->request_uri_normalized)[0] != '/' ||
            bstr_ptr(tx_ud->request_uri_normalized)[1] != '?' ||
            bstr_ptr(tx_ud->request_uri_normalized)[2] != 'a' ||
            bstr_ptr(tx_ud->request_uri_normalized)[3] != '=' ||
            bstr_ptr(tx_ud->request_uri_normalized)[4] != '%' ||
            bstr_ptr(tx_ud->request_uri_normalized)[5] != '0' ||
-            bstr_ptr(tx_ud->request_uri_normalized)[6] != '0')


Same as with other case :)

jufajardini · 2024-10-18T02:14:45Z

src/app-layer-htp.c

-            goto end;
-        }
+    FAIL_IF_NULL(tx);
+    FAIL_IF_NULL(tx_ud);


This check doesn't seem necessary here (also comparing with other similar tests in this file)

Nancyenos · 2024-10-18T04:58:23Z

Thankyou @jufajardini ..It was first time naivety! I did not realize how much work was required, almost gave up but am glad i stuck through.
yes i will be working on something else as i await that final check

Nancyenos · 2024-10-22T12:17:23Z

I know this is a very big file, with lots of tests. You're brave for picking up this one!

Test HTPBodyReassemblyTest01 still has a mixed style: it hasn't been fully converted to the FAIL/PASS style.

I think I've covered all cases where we need some improvement, still, so you can get a good understanding of what the next steps are.

There is one thing that I asked someone else to do a sanity check. If you are idly waiting, feel free to claim another task, in this case, ok?

Hey @jufajardini , did the sanity test pass?

jufajardini · 2024-10-22T17:52:24Z

I know this is a very big file, with lots of tests. You're brave for picking up this one!
Test HTPBodyReassemblyTest01 still has a mixed style: it hasn't been fully converted to the FAIL/PASS style.
I think I've covered all cases where we need some improvement, still, so you can get a good understanding of what the next steps are.
There is one thing that I asked someone else to do a sanity check. If you are idly waiting, feel free to claim another task, in this case, ok?

Hey @jufajardini , did the sanity test pass?

Yep! See Shivani's comment on the thread where we were discussing the check formats :)

You can submit a new version of this work whenever you have the time ^^

applayer/htp: convert to new FAIL/PASS API

ed07b85

Ticket: OISF#6935

Nancyenos requested a review from victorjulien as a code owner October 16, 2024 07:50

Nancyenos mentioned this pull request Oct 16, 2024

applayer/htp: convert to new FAIL/PASS API v-4 #11960

Closed

5 tasks

jufajardini added the outreachy Contributions made by Outreachy applicants label Oct 16, 2024

jufajardini self-assigned this Oct 17, 2024

jufajardini requested changes Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applayer/htp: convert to new FAIL/PASS API /v5 #11973

applayer/htp: convert to new FAIL/PASS API /v5 #11973

Nancyenos commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

jufajardini left a comment

jufajardini Oct 18, 2024

jufajardini Oct 18, 2024

jufajardini Oct 18, 2024

inashivb Oct 18, 2024 •

edited

Loading

jufajardini Oct 18, 2024

inashivb Oct 21, 2024

jufajardini Oct 18, 2024

jufajardini Oct 18, 2024

Nancyenos commented Oct 18, 2024 •

edited

Loading

Nancyenos commented Oct 22, 2024

jufajardini commented Oct 22, 2024

applayer/htp: convert to new FAIL/PASS API /v5 #11973

Are you sure you want to change the base?

applayer/htp: convert to new FAIL/PASS API /v5 #11973

Conversation

Nancyenos commented Oct 16, 2024 • edited Loading

Contribution style:

Our Contribution agreements:

Changes (if applicable):

Describe changes:

Provide values to any of the below to override the defaults.

github-actions bot commented Oct 16, 2024

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

jufajardini left a comment

Choose a reason for hiding this comment

jufajardini Oct 18, 2024

Choose a reason for hiding this comment

jufajardini Oct 18, 2024

Choose a reason for hiding this comment

jufajardini Oct 18, 2024

Choose a reason for hiding this comment

inashivb Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

jufajardini Oct 18, 2024

Choose a reason for hiding this comment

inashivb Oct 21, 2024

Choose a reason for hiding this comment

jufajardini Oct 18, 2024

Choose a reason for hiding this comment

jufajardini Oct 18, 2024

Choose a reason for hiding this comment

Nancyenos commented Oct 18, 2024 • edited Loading

Nancyenos commented Oct 22, 2024

jufajardini commented Oct 22, 2024

Nancyenos commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

inashivb Oct 18, 2024 •

edited

Loading

Nancyenos commented Oct 18, 2024 •

edited

Loading