logstash icon indicating copy to clipboard operation
logstash copied to clipboard

[Bug]🐛 Badly anchored regular expression

Open odaysec opened this issue 9 months ago • 1 comments

https://github.com/elastic/logstash/blob/3115c78bf84ae45b8eb8e13fc954f373c3d7dfc4/logstash-core/lib/logstash/util/thread_safe_attributes.rb#L24-L24

Regular expressions in Ruby can use anchors to match the beginning and end of a string. However, if the ^ and $ anchors are used, the regular expression can match a single line of a multi-line string. This allows bad actors to bypass your regular expression checks and inject malicious input.

POC

The following (bad) example code uses a regular expression to check that a string contains only digits.

def bad(input) 
    raise "Bad input" unless input =~ /^[0-9]+$/

    # ....
end

The regular expression /^[0-9]+$/ will match a single line of a multi-line string, which may not be the intended behavior. The following (good) example code uses the regular expression \A[0-9]+\z to match the entire input string.

def good(input)
    raise "Bad input" unless input =~ /\A[0-9]+\z/

    # ....
end

References

Anchors CWE-20

odaysec avatar Feb 27 '25 12:02 odaysec

Hey @odaysec, great catch on this regex hygiene—those anchors can trip folks up, even if Ruby's defaults play nice here. Dived into thread_safe_attributes.rb (around that 3115c78 commit), and yeah, the match? call with /^[_A-Za-z]\w*$/ is checking attribute names for valid identifiers, but swapping to \A...\z would bulletproof it against any future multiline weirdness or edge-case injections. Quick validation:

  • Current: ^/$ work as string anchors (no /m flag), so it should enforce whole-string match on single attrs like :foo_bar. Tested a quick IRB snippet: "valid".match?(/^[_A-Za-z]\w*$/) → true; "invalid\nfoo".match?(/^[_A-Za-z]\w*$/) → false. Solid for now.
  • But POC nails it: If input ever gets multiline (e.g., via env vars or dynamic attrs), bad stuff slips through. Plus, it's low-risk to align with CWE-20 best practices.

Fix vibes (building on #17164):

  • Swap to /\A[_A-Za-z]\w*\z/—zero perf hit, clearer intent.
  • Add a spec in spec/util/thread_safe_attributes_spec.rb for multiline poison pills.
  • Docs nudge in CONTRIBUTING.md for consistent anchoring in validators.

PR #17164 looks like a clean lift—any blockers? Happy to iterate on it, add tests, or repro in a branch. Can you please assign this to me.

0xShubhamSolanki avatar Oct 27 '25 19:10 0xShubhamSolanki