Removed multiple heap-allocated copies in Pm::init & parse_pm_content

[eduar-hte]

what

Removed multiple heap-allocated copies of operator Pm's parameter and other unnecessary heap allocations.

changes

• parse_pm_content
  • The previous version of this function was doing three strdup copies to parse the pm content. The updated version only copies the value once (in order not to modify the operator's m_param member variable), and then performs the updates inline.
  • Fixed parsing of digits (see here) which were not quoted and thus not interpreted as ASCII characters (like the hexadecimal digits) but as binary values (this is incorrect because if that were the case, 0 would have been interpreted as the null terminator when doing strdup before, thus truncating the content).
  • Error message in parse_pm_content would reference freed memory if accessed by caller (see here, here, here, here & here). Removed anyway because it was unused.
  • Moved parse_pm_content to src/operators/pm.cc as this function was introduced in ModSecurity v3 and only used by Pm::init.
• Pm::init
  • Simplified to avoid manually handling of memory buffers (as parse_pm_content now returns a std::string instead of a char *).
  • std::istringstream iss is no longer heap allocated.
  • Removed temporary allocation of std::vector<std::string> vec to store acmp patterns, leveraging std::for_each to add them as they're parsed from the stream.
• Removed src/operators/pm_f.cc (all code in modsecurity::operators::PmF is inline).
• Removed unused included header files.

misc

This PR was originally part of a remove copies branch which included PR #3231 & #3222, but was broken down into separate PRs to group changes and simplify the review process. I'm not including this in the performance improvement series because even though these changes are related to the initialization of rules (and thus the Pm operator).

[airween]

Hi @eduar-hte,

thanks again for this PR.

• Fixed parsing of digits (see here) which were not quoted and thus not interpreted as ASCII characters (like the hexadecimal digits) but as binary values (this is incorrect because if that were the case, 0 would have been interpreted as the null terminator when doing strdup before, thus truncating the content).

uhmm, this is a bit scary. I see you fixed this behavior, but I'm afraid we should definitely must inform users about this issue. I'm not sure we need to open a CVE, but when we release the new version, we need to draw the attention of users.

@marcstern, @fzipi, @theseion, @gberkes (and of course @eduar-hte) - what do you think guys?

This PR was originally part of a remove copies branch which included PR #3231 & #3222, but was broken down into separate PRs to group changes and simplify the review process. I'm not including this in the performance improvement series because even though these changes are related to the initialization of rules (and thus the Pm operator).

Would it be there any conflict between this PR and #3231 (for eg. if I merge this one first...)?

I added one comment to my review.

[eduar-hte]

This PR was originally part of a remove copies branch which included PR #3231 & #3222, but was broken down into separate PRs to group changes and simplify the review process. I'm not including this in the performance improvement series because even though these changes are related to the initialization of rules (and thus the Pm operator).

Would it be there any conflict between this PR and #3231 (for eg. if I merge this one first...)?

No, they're independent from each other. I mentioned that PR (and the previous one, PR #3222) because they were originally on the same remove-copies branch, focused on removing unnecessary heap-allocated copies of strings.

This commit was even part of my original submission for PR #3231, but I moved it to its own PR for that PR to be focused on Transformation classes.

[theseion]

uhmm, this is a bit scary. I see you fixed this behavior, but I'm afraid we should definitely must inform users about this issue. I'm not sure we need to open a CVE, but when we release the new version, we need to draw the attention of users.

Not nice. Fortunately, this affects something that probably only a few people use, which is parsing of |HEX| (I didn't even know that functionality existed; it must be what the docs refer to as "snort/suricata content style"). In any case, it has always been broken in v3 without users complaining. Yes, mention it in the release notes. I don't feel like we have to make a big deal out of this.

[eduar-hte]

uhmm, this is a bit scary. I see you fixed this behavior, but I'm afraid we should definitely must inform users about this issue. I'm not sure we need to open a CVE, but when we release the new version, we need to draw the attention of users.

As @theseion mentions, I assume this is not currently being used because it'd just not work as is.

In addition to the fix, I think it'd be appropriate to exit the function in error when an invalid hex character is expected and not found (similarly to the way an invalid escape sequence is handled). I'll update the commit to include this.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[airween]

LGTM

[airween]

@eduar-hte - thanks for fix (VALID_HEX()).

@theseion, @eduar-hte - thanks guys for answers.

Going to merge this.