[Rule Tuning] EQL bugs using regex and backslashes (Windows Paths)

[w0rk3r]

Rules affected

• https://github.com/elastic/detection-rules/blob/main/rules/windows/credential_access_cmdline_dump_tool.toml
• Suspicious PowerShell Engine ImageLoad
• Image File Execution Options Injection

Description

A change in Lucene seems to break the usage of \ in regex conditions and marked as a no-fix, so we need to adjust the logic of these rules accordindly. The impact is low, as these regex conditions are used as exceptions on our rules and not the main logic, but they can cause FPs.

The use of triple quotes seems to prevent the rule from throwing a error, but the regex will not working based on my testing.

SDH issue for context.

[mark-dufresne]

@w0rk3r I had a question about the expected solution. Are you expecting to get rid of regex to work around for the bug/breaking change discussed in the SDH?

[w0rk3r]

Yeah, as it seems to be a no-fix, and our use here is to exclude paths, I'll replace the regex expressions with wildcards or just an OR condition on Program Files and Program Files (x86)

[rw-access]

hello team! it's been a while 😉
my understanding of what I can see publicly about that Lucene change is that it's actually a good thing! I wish it was done much earlier, to be honest. It removes leniency for \ so that you can only use proper character escapes, and \\ is valid but \a won't quietly become a literal \ then a anymore.

As long as you use \ properly and put it inside a """ it looks like it will work correctly. If you don't put it inside a """ string, then you'll need to remember to double escape: once for EQL, and once for regex.

It looks like there are a lot of issues here that are linked but not public so I can't grasp the full context. Everything I can see points to process.parent.executable regex~ """C:\\Program Files( \(x86\))?\\Cisco Systems\\.*""" functioning the same. The only slashes used there are used to escape special characters like \, ( and ), so it looks like it would work as expected.

Is there any (public) documentation for how this breaks existing rules? All the existing rules should be using \ properly like the above example, so I think you should be okay.

[brokensound77]

Thanks @rw-access, that sounds spot on!

I did a review of all existing rules using regex and I saw no issues. The only rule using regex which is not triple quoted was AdminSDHolder SDProp Exclusion Added, which has no escapes in the query

...
  winlog.event_data.AttributeValue regex~ "[0-9]{15}([1-9a-f]).*"
...

The original source of the issue was centered around custom rules, which were not triple quoting.

I think we can close #2284 with no further action beyond continuing to ensure proper syntax in our own rules

[w0rk3r]

Yeah, just validated our cases and seems there are really no issues on them (sorry and thanks!)... But the initial bug was something like this, but using single quotes:

Query:

process where
(
    process.name like "control.exe"
    and process.args like~ "*.cpl"
    and not process.command_line regex~ """.*:\\Windows\\System32\\\\w+\\.cpl.*"""
)

I suggested using triple quotes:

Which doesn't throw any error, but doesn't match the condition neither:

When I use a single backslach for \w+ it throws this error:

search_phase_execution_exception: [query_shard_exception] Reason: failed to create query: Query must not be null; [query_shard_exception] Reason: failed to create query: Query must not be null; [query_shard_exception] Reason: failed to create query: Query must not be null

Any clues around this?

[rw-access]

not sure about the exception to be honest. it does look like you have \\w when you should have just \w and \\. when it should be \.. but if it got an exception when doing just \w then I'm not sure why.

also, keep in mind that it will have different results for different versions of Lucene and Elasticsearch because \w hasn't always been interpreted as a predefined character class, so for an old version it'll be the same as \\w. if you switch \w to [a-z0-9_] you'll get maximal compatibility!

you could also do [^\\] if you want to match all non-\ characters so you get one directory

[w0rk3r]

I'm doing some more testing, and seems that the problem is that I can't use \w+, \d+, \S+, etc in 8+, not the backslashes:

In 8.3.3:

In 7.17.x it works just fine:

Wdyt, is this a bug that we should track somewhere? I'm working with the docs team to adjust this to be more precise about the bug we got on the SDH, how should we report this? @rw-access @brokensound77

[rw-access]

Personally, I would switch to [a-z...] style character classes for full compatibility. I believe the 7.x series treats \w literally, so it's the same as \\w. I don't know the best part forward for reporting, documentation, etc., so I'll let you all handle that.

[luigidellaquila]

I tested a similar query on a fresh 8.4.1 cloud instance and everything seems to work fine

maybe I'm missing something, or the problem is already fixed in v 8.4.x... As far as I remember, we did not do much on these aspects recently, so if it's a fix, probably it's in Lucene.

[w0rk3r]

@luigidellaquila I just tested the following query in a new 8.4.1 and got the same error:

process where process.command_line regex~ """\w+"""

Diving a bit more, I just saw that it works normal with process.args:

Maybe this is a issue with wildcard fields?

[luigidellaquila]

@w0rk3r yes, the problem seems to be specific to wildcard fields.
I'm investigating it, I'll keep you updated

[luigidellaquila]

@w0rk3r I can confirm that the problem happens specifically on wildcard fields.
Actually, the problem is not strictly related to EQL, I tried to run a simple search query:

{
    "query": {
        "regexp": {
            "name": {
                "value": "\\w+"
            }
        }
    }
}

the query works fine if name is a text or a keyword field, but fails if it's a wildcard.

I think I found the problem in the code and a possible fix, I'll keep you updated

[luigidellaquila]

Hi all,

the PR with the fix was just merged on main branch (8.5)

[w0rk3r]

Awesome, I'll keep this one open until we have the BC for testing. @nastasha-solomon I think @luigidellaquila may give you a better picture of the bug/limitation to get elastic/security-docs#2337 right, let me know if any input from my side is needed.

[w0rk3r]

Already worked with Nastasha and Luigi via Slack to get the bug properly documented, so I'm closing this as completed.