[Bug] Rule Toml Write Formatting Wrongly Formats \\\\x

[eric-forte-elastic]

Pull Request

Issue link(s):

Relates Issue: #4968

Resolves #4977

Summary - What I changed

Note: Please review this fairly carefully as we need to check for unintended consequences and whether or not this is the true root cause of the issue that broke the rule in #4977

I added a hardcoded check to mitigate an unwated behavior of Python's toml library string dump formatting (see Additional Context from #4977).

This checks for the presence of \\\\x which would trigger an unwanted replacement to \\u00 and calls python's string formatting directly instead of toml's string dump.

How To Test

1. Use the version lock file from commit 2d2c5b4
2. Pull the rule contents for rules/windows/privilege_escalation_printspooler_service_suspicious_file.toml from this commit as well.
3. Run a command such as python -m detection_rules dev trim-version-lock 8.14.0 that would same_toml on a rule with \\\\x
4. See that the [rule.filters.query.wildcard."file.path"] values are do not have \\\\x replaced with \\u00 whereas they did in Prep main for 9.1 #4555

Checklist

• Added a label for the type of pr: bug, enhancement, schema, maintenance, Rule: New, Rule: Deprecation, Rule: Tuning, Hunt: New, or Hunt: Tuning so guidelines can be generated
• Added the meta:rapid-merge label if planning to merge within 24 hours
• Secret and sensitive material has been managed correctly
• Automated testing was updated or added to match the most common scenarios
• Documentation and comments were added for features that require explanation

Contributor checklist

• Have you signed the contributor license agreement?
• Have you followed the contributor guidelines?

[github-actions]

Bug - Guidelines

These guidelines serve as a reminder set of considerations when addressing a bug in the code.

Documentation and Context

• Provide detailed documentation (description, screenshots, reproducing the bug, etc.) of the bug if not already documented in an issue.
• Include additional context or details about the problem.
• Ensure the fix includes necessary updates to the release documentation and versioning.

Code Standards and Practices

• Code follows established design patterns within the repo and avoids duplication.
• Ensure that the code is modular and reusable where applicable.

Testing

• New unit tests have been added to cover the bug fix or edge cases.
• Existing unit tests have been updated to reflect the changes.
• Provide evidence of testing and detecting the bug fix (e.g., test logs, screenshots).
• Validate that any rules affected by the bug are correctly updated.
• Ensure that performance is not negatively impacted by the changes.
• Verify that any release artifacts are properly generated and tested.
• Conducted system testing, including fleet, import, and create APIs (e.g., run make test-cli, make test-remote-cli, make test-hunting-cli)

Additional Checks

• Verify that the bug fix works across all relevant environments (e.g., different OS versions).
• Confirm that the proper version label is applied to the PR patch, minor, major.

[tradebot-elastic]

⛔️ Test failed

Results

• ❌ Suspicious PrintSpooler Service Executable File Creation (kuery)
  • coverage_issue: no_rta
  • stack_validation_failed: no_rta

[Mikaayenson]

Did you discover why this didn't appear in the past with rule rules/windows/privilege_escalation_printspooler_service_suspicious_file.toml ? We should figure that out in case it influences how we address the root issue.

[eric-forte-elastic]

2. privilege_escalation_printspooler_service_suspicious_file.toml

Yes, the key for this is that this issue occurs when the string contains \\\\x but not when it contains \\x

Example

Following the commit history of the rule, the string had \\x up until this commit at which point we changed it to \\\\x. At this time it did not have a min stack, and had not gone through updated MITRE mappings prior to #4555.

The one of the next updates then added a min stack, which would not be impacted by the trim until #4555.

The only other change prior to #4555, was in generating investigation guides. The generation appears not to have used a dev function, but some custom code as mentioned in: #4358. This custom code used the tomlkit library instead of toml which appears to not have this \\\\x -> \\u00 issue.

At which point, the PR #4555 looks to be the first time we run this rule through the toml library where the rule has \\\\x as opposed to \\x, thereby causing the issue.

Additionally, this looks to be the first time we have sent a rule that had \\\\x through any of our toml write (save_toml) dev pipelines (as it is currently the only rule that has this).

[tradebot-elastic]

⛔️ Test failed

Results

• ❌ Suspicious PrintSpooler Service Executable File Creation (kuery)
  • coverage_issue: no_rta
  • stack_validation_failed: no_rta

[Mikaayenson]

I think we will want a new unit test in test_toml_formatter.py

[eric-forte-elastic]

++ agreed, added new unit test that looks for this specifically. It also will fail if the path behavior changes. E.g. currently we expect \\\\ to be formatted to \\. If this changes the unit test will fail on purpose as we want to match Query DSL's path handling which does this as well.

[Mikaayenson]

can we add another test to make sure \\\\x is not converted to \\u00?

[eric-forte-elastic]

Does this test not accomplish that?

    def test_formatter_paths(self):
        """Test that paths are handled as expected with toml lib."""
        with self.assertRaisesRegex(
            AssertionError,
            r'\+ {"metadata": {"field": "value"}, "rule": {"path": "\?:\\\\Windows\\\\Sys\?\?\?\?\?\\\\x5lrs\.dll"}}',
        ):
            self.compare_test_data([self.test_data[1]])

This output checks the need for "?:\\\\Windows\\\\Sys?????\\\\x5lrs.dll" to explicitly become "?:\\Windows\\Sys?????\\x5lrs.dll"

If the \\\\x is converted to \\u00 it will fail. It will fail if \\\\x is not converted to exactly \\x (e.g. \\\\x -> \\\\x will also fail as is its intention)

The regex filter to catch the specific transformation

The + line needs to match exactly in order for this unit test to pass.

Catches the \\u00 in its current form

Is this accomplishing what you are looking for?

[eric-forte-elastic]

Updated to catch and additional issue where where backslashes in certain strings are not properly escaped in self._old_dump_str(v). The input and output paths should now match for the filters. As a note, Query DSL does require 4 \ to equate to 2 \ which is why we want the string formatting to be preserved (as it now is).

The unit test now includes a path that otherwise would have been escaped or transformed with the magic \\\\x -> \\u00.

To test that the fix does not introduce any other concerns, checkout the commit before the change 2d2c5b4, run python -m detection_rules dev trim-version-lock 8.14.0 and compare the git patches with and without the fix to ensure the following expected output is achieved.

Expected Output

Needs to match the path formatting from https://github.com/elastic/detection-rules/pull/4976/files

Which in our testing it does.

❯ diff my_patch_without_fix.patch my_patch_with_fix.patch 
28856c28856
< index 30d052601..bbfa62985 100644
---
> index 30d052601..4f0f3fb4c 100644
28966c28966
< +value = "?:\\Windows\\Sys?????\\u005lrs.dll"
---
> +value = "?:\\\\Windows\\\\Sys?????\\\\x5lrs.dll"
28976c28976
< +value = "?:\\Windows\\system32\\spool\\DRIVERS\\u0064\\\\*.dll"
---
> +value = "?:\\\\Windows\\\\system32\\\\spool\\\\DRIVERS\\\\x64\\\\*.dll"
28996c28996
< +value = "?:\\Windows\\system32\\spool\\PRTPROCS\\u0064\\\\*.dll"
---
> +value = "?:\\\\Windows\\\\system32\\\\spool\\\\PRTPROCS\\\\x64\\\\*.dll"

[eric-forte-elastic]

Additional multi line testing to demonstrate that replace accomplishes the same outcome for multi-line and not raw

[Mikaayenson]

lgtm!