Skip to content

fix(933100): reduce false positive on Extensible Metadata Platform and xsl-stylesheets#4445

Merged
fzipi merged 14 commits into
coreruleset:mainfrom
touchweb-vincent:patch-11
Feb 26, 2026
Merged

fix(933100): reduce false positive on Extensible Metadata Platform and xsl-stylesheets#4445
fzipi merged 14 commits into
coreruleset:mainfrom
touchweb-vincent:patch-11

Conversation

@touchweb-vincent
Copy link
Copy Markdown
Contributor

@touchweb-vincent touchweb-vincent commented Feb 6, 2026 •
edited
Loading

Proposed changes

Hello,

Following this issue: #4443 , I see no reason why we should not treat this as a false positive, and I do not see any abuse or bypass use case at this time on the scope of this rule.

<?xpacket begin="?" id="W5M0MpCehiHzreSzNTczkc9d"?>
<x:xmpmeta xmlns:x="adobe:ns:meta/" x:xmptk="Adobe XMP Core 5.4-c002 1.000000, 0000/00/00-00:00:00        ">

At this point, I don’t think it would be relevant to create a custom rule to block this, but perhaps one of you may have a different opinion on the matter.

PR Checklist

  • I have read the CONTRIBUTING doc
  • I have added positive tests proving my fix/feature works as intended.
  • I have added negative tests that prove my fix/feature considers common cases that might end in false positives
  • In case you changed a regular expression, you are not adding a ReDOS for pcre. You can check this using regexploit
  • My test use the comment field to write the expected behavior
  • I have added documentation for the rule or change (when appropriate)

Further comments

For the reviewer

  • Positive and negative tests were added
  • Tests cover the intended fix/feature properly
  • No usage of dangerous constructs like ctl:requestBodyAccess=Off were used in the rule
  • In case a regular expression was changed, there is no ReDOS
  • Documentation is clear for the rule/change

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 6, 2026 •
edited
Loading

📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

@fzipi
Copy link
Copy Markdown
Member

fzipi commented Feb 10, 2026

Honestly, at this point I'm trying to understand what the hell is XML doing here.

@fzipi
Copy link
Copy Markdown
Member

fzipi commented Feb 11, 2026

I think I now got it. The idea was to eliminate FPs with <?xml. After simplification, no:

  • should catch all PHP injection attacks (<?php, <?=, <? , [php], <?xml : polyglots)
  • excludes legitimate XML patterns (<?xml-stylesheet, <?xsl-stylesheet, <?xpacket)

@fzipi fzipi requested a review from Copilot February 11, 2026 01:36
Copilot AI reviewed Feb 11, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates CRS rule 933100 (PHP open tag detection) to reduce false positives triggered by XMP metadata processing instructions such as <?xpacket ...?>, and expands the regression test suite accordingly.

Changes:

  • Adjusts rule 933100 regex to narrow matching and avoid <?xpacket/similar non-PHP processing instructions.
  • Updates the regex-assembly source for 933100 to reflect the new matching logic.
  • Adds extensive positive/negative regression tests for PHP open tags, XML lookalikes, and XMP <?xpacket cases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
tests/regression/tests/REQUEST-933-APPLICATION-ATTACK-PHP/933100.yaml Adds many new regression tests, including XMP <?xpacket negative cases and additional PHP/XML variants.
rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf Modifies rule 933100 detection regex to reduce false positives on XMP-like processing instructions.
regex-assembly/933100.ra Updates the regex-assembly definition for rule 933100 to align with the new intended matching behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/regression/tests/REQUEST-933-APPLICATION-ATTACK-PHP/933100.yaml Outdated
Comment thread regex-assembly/933100.ra Outdated
Comment thread regex-assembly/933100.ra
touchweb-vincent and others added 10 commits February 15, 2026 09:47
@touchweb-vincent @fzipi
fix(933100): reduce false positive against Extensible Metadata Platform
2356c77
@pre-commit-ci @fzipi
chore(formatting): auto fixes from pre-commit hooks
501b44b 
for more information, see https://pre-commit.ci
@touchweb-vincent @fzipi
Update 933100.yaml
91851fa
@touchweb-vincent @fzipi
Update 933100.yaml
e1dcabb
@fzipi
tests(933100): add comprehensive tests
28893bb 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi
feat: add support for xsl-stylesheet and short tags
4afbd03 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi
fix: simplify regex
2105659 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi
fix: update regex patterns
91e5ddf 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi
docs(933100): add comments for BBCode and Smarty regex assembly blocks
80811b0
@fzipi
fix(933100): update tests 24-25 to reflect php\s regex change
40d97b8 
<?php without trailing whitespace should no longer match.
@fzipi fzipi changed the title fix(933100): reduce false positive against Extensible Metadata Platform fix(933100): reduce false positive on Extensible Metadata Platform and xsl-stylesheets Feb 15, 2026
fzipi
fzipi approved these changes Feb 15, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

LGTM

@fzipi
Copy link
Copy Markdown
Member

fzipi commented Feb 15, 2026

@azurit Can I get your eyes on this one also?

@azurit
Copy link
Copy Markdown
Member

azurit commented Feb 16, 2026

@fzipi Sure, give me some time.

@azurit azurit linked an issue Feb 17, 2026 that may be closed by this pull request
False positive for 933100 for <?xpacket #4443
Closed
@fzipi
Copy link
Copy Markdown
Member

fzipi commented Feb 24, 2026

ping @azurit so this one makes the next release

airween
airween approved these changes Feb 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@airween airween left a comment

Choose a reason for hiding this comment

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

LGTM

@fzipi fzipi added this pull request to the merge queue Feb 26, 2026
Merged via the queue into coreruleset:main with commit cab0768 Feb 26, 2026
8 checks passed
@fzipi fzipi mentioned this pull request Mar 2, 2026
Closed
@touchweb-vincent touchweb-vincent deleted the patch-11 branch April 15, 2026 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@airween airween airween approved these changes

@fzipi fzipi fzipi approved these changes

Assignees

No one assigned

Labels

release:fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive for 933100 for <?xpacket

5 participants

@touchweb-vincent @fzipi @azurit @airween