[
https://issues.apache.org/jira/browse/GROOVY-12122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18093776#comment-18093776
]
ASF GitHub Bot commented on GROOVY-12122:
-----------------------------------------
codecov-commenter commented on PR #2668:
URL: https://github.com/apache/groovy/pull/2668#issuecomment-4885122857
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2668?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `82.35294%` with `12 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 68.5763%. Comparing base
([`faee99b`](https://app.codecov.io/gh/apache/groovy/commit/faee99b83998b6557bef2bda9968b9eb5dc0b502?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`46e0280`](https://app.codecov.io/gh/apache/groovy/commit/46e02809586b4e53bc4b33ca575db88a4daba8ea?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2668?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...s/groovy/transform/SafeRegexASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FSafeRegexASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9TYWZlUmVnZXhBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| 77.7778% | [2 Missing and 6 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[src/main/java/groovy/util/regex/RegexGuard.java](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Futil%2Fregex%2FRegexGuard.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvdXRpbC9yZWdleC9SZWdleEd1YXJkLmphdmE=)
| 86.6667% | [1 Missing and 3 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2668 +/- ##
==================================================
+ Coverage 68.5706% 68.5763% +0.0058%
- Complexity 33808 33837 +29
==================================================
Files 1524 1527 +3
Lines 128141 128209 +68
Branches 23292 23305 +13
==================================================
+ Hits 87867 87921 +54
- Misses 32472 32475 +3
- Partials 7802 7813 +11
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2668?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[.../java/groovy/util/regex/RegexTimeoutException.java](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Futil%2Fregex%2FRegexTimeoutException.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvdXRpbC9yZWdleC9SZWdleFRpbWVvdXRFeGNlcHRpb24uamF2YQ==)
| `100.0000% <100.0000%> (ø)` | |
|
[src/main/java/groovy/util/regex/RegexGuard.java](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Futil%2Fregex%2FRegexGuard.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvdXRpbC9yZWdleC9SZWdleEd1YXJkLmphdmE=)
| `86.6667% <86.6667%> (ø)` | |
|
[...s/groovy/transform/SafeRegexASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/2668?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FSafeRegexASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9TYWZlUmVnZXhBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| `77.7778% <77.7778%> (ø)` | |
... and [8 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2668/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> Provide a Regex timeout facility
> --------------------------------
>
> Key: GROOVY-12122
> URL: https://issues.apache.org/jira/browse/GROOVY-12122
> Project: Groovy
> Issue Type: Improvement
> Components: regex
> Reporter: Paul King
> Priority: Major
> Labels: GEP
> Fix For: 7.x
>
>
> h2. Summary
> Add an opt-in, thread-free runtime guard against Regular Expression Denial of
> Service
> (ReDoS / catastrophic backtracking) for Groovy's regex features, offered as a
> runtime
> helper ({{RegexGuard}}), a scoped annotation ({{@SafeRegex}}), and hardening
> of Groovy's
> own internal regex call sites.
> h2. Motivation
> Groovy makes regex frictionless -- {{=~}}, {{==~}}, {{~/.../}},
> {{String#matches}},
> {{replaceAll}}, {{split}}, and the {{Matcher}} sugar -- which also makes
> ReDoS easy to
> reach when a pattern (or its input) comes from outside the program: scripts,
> build files,
> web handlers, {{ConfigSlurper}}, templating.
> {{java.util.regex}} has *no native timeout*, so a crafted input against a
> backtracking-prone pattern can hang the calling thread indefinitely. Nothing
> in Groovy
> guards against this today.
> JLine 4.3.1 shipped a {{SafeRegex}} utility using the well-known
> {{TimeoutCharSequence}}
> technique to close four CVEs (GHSA-r2xf-8xr9-62gw, GHSA-2v9w-34q6-wpqx,
> GHSA-ph9c-7hw9-vhhw, GHSA-5q95-hrpc-m3w3). We can apply the same idea.
> h2. Proposal
> Bound regex matching by a wall-clock deadline, without a watchdog thread. The
> input
> {{CharSequence}} is wrapped so that {{charAt()}} checks a deadline and throws
> a
> {{RegexTimeoutException}} once exceeded; because the regex engine calls
> {{charAt()}}
> continuously while backtracking, a runaway match is interrupted with no extra
> thread and
> no {{Thread.interrupt}} plumbing.
> Three deliverables, from lowest-level to most ergonomic:
> * *(a) Internal hardening* -- wrap regex matches against user-supplied input
> in Groovy's
> own runtime and tools (templating, {{groovysh}} completion, builders),
> exactly as JLine
> did across its modules. Invisible; no public API change.
> * *(b) Runtime helper ({{RegexGuard}})* -- an explicit guarded-match API for
> user code and
> for anything the annotation cannot reach:
> {code:groovy}
> RegexGuard.matches(pattern, input, Duration.ofMillis(200)) // -> boolean,
> throws on timeout
> RegexGuard.matcher(pattern, input, Duration.ofMillis(200)) // -> Matcher
> over a guarded input
> {code}
> * *(c) Scoped annotation ({{@SafeRegex}})* -- at method / class / package
> level; an AST
> transform rewrites the regex match operations lexically within scope to the
> {{RegexGuard}}
> path, so no call-site changes are needed. Fits the existing
> {{@TimedInterrupt}} /
> {{@ThreadInterrupt}} / {{@ConditionalInterrupt}} family in
> {{groovy.transform}}.
> {code:groovy}
> @SafeRegex(millis = 200)
> class Handler {
> boolean check(String input) {
> input ==~ /(a+)+$/ // rewritten ->
> RegexGuard.matches(~/(a+)+$/, input, 200ms)
> }
> }
> {code}
> The guard is on *matching*, not on {{~/.../}} compilation, so the transform
> rewrites the
> match operations ({{=~}}, {{==~}}, {{String#matches}},
> {{replaceAll}}/{{replaceFirst}},
> {{split}}, {{Matcher}} operations), not {{Pattern.compile}}.
> h3. Naming
> || Concern || Name || Notes ||
> | Scoped annotation | {{@SafeRegex}} | in {{groovy.transform}}, alongside
> {{@TimedInterrupt}} et al. |
> | Runtime helper | {{RegexGuard}} | distinct from the annotation to avoid a
> clash |
> | Timeout exception | {{RegexTimeoutException}} | unchecked; lets callers
> distinguish a ReDoS abort from a normal non-match |
> h2. Scope
> *In scope*
> * Runtime deadline guard ({{TimeoutCharSequence}}-style), thread-free
> * Internal hardening of Groovy's own user-input regex sites
> * {{RegexGuard}} helper and {{@SafeRegex}} scoped annotation for user code
> * {{RegexTimeoutException}} type
> *Out of scope / deferred*
> * Making the guard the default for {{=~}} / {{==~}} (behaviour change +
> overhead)
> * Static ReDoS pattern detection -- a possible future {{RegexChecker}}
> extension, not this ticket
> * Rewriting the regex engine or adopting a non-backtracking (RE2-style) engine
> h2. Limitations
> * {{@SafeRegex}} rewrites only *lexically-visible* regex ops -- scoped
> hardening, not
> whole-program. A regex executed inside a called library method is not
> rewritten; use
> {{RegexGuard}} explicitly there.
> * The deadline fires only while the engine is calling {{charAt}} (the
> backtracking phase) --
> true of catastrophic cases but not a hard guarantee for every pathological
> state.
> * Each {{charAt}} pays a small deadline check ({{System.nanoTime}}), so the
> guard is opt-in /
> internal-only, never a blanket default.
> * Reduces hang risk; it is not a correctness or a data-at-rest control.
> h2. Relationship to existing features
> || Layer || Mechanism || Status || Covers ReDoS? ||
> | Compile-time validity | {{RegexChecker}} (groovy-typecheckers) | ships | No
> -- pattern *syntax* only |
> | Compile-time ReDoS lint | possible {{RegexChecker}} extension | idea |
> Partially |
> | Untrusted-input tracking | GEP-25 {{@Tainted}} -> regex sink | draft |
> Identifies risky matches |
> | *Runtime guard (this ticket)* | {{RegexGuard}} / {{@SafeRegex}} | *missing*
> | *Yes -- the actual backstop* |
> {{RegexChecker}} only validates that a literal pattern compiles; it says
> nothing about
> backtracking. This ticket fills the runtime layer none of the others cover.
> It also
> composes with GEP-25: a regex applied to a {{@Tainted}} string is a ReDoS
> sink, and a
> {{RegexGuard}}-guarded (or {{@SafeRegex}}-scoped) match is its runtime
> sanitizer.
> Here's the section with `GROOVY-12123` slotted in — ready to paste into
> GROOVY-12122.
> h2. Relates to / composes with GROOVY-12123 (constant regex hoisting)
> GROOVY-12123 hoists effectively-constant regexes to synthetic {{static final
> Pattern}}
> fields so they compile once. The two tickets touch the same regex AST surface
> and sit on
> opposite ends of the same operation -- that ticket optimizes *Pattern
> construction*, this
> one guards *matching* -- so they compose, but the transforms must be
> ordering-aware:
> * When {{@SafeRegex}} rewrites {{input ==~ /(a+)+$/}} to
> {{RegexGuard.matches(~/(a+)+$/, input, ...)}}, the hoisting pass must still
> recognize the
> {{~/(a+)+$/}} argument as a hoistable constant and lift it to a {{static
> final}} field.
> * When hoisting runs first and produces a {{static final Pattern P}},
> {{@SafeRegex}} must
> still recognize a match against {{P}} and wrap the *input* in the deadline
> guard.
> Desired combined result: the constant {{Pattern}} is compiled once into a
> {{static final}}
> field *and* each match wraps the input in the deadline guard. Whoever
> implements the second
> of the two should account for the first.
> h2. Licensing / reuse
> JLine is BSD-3 (ASF category A) and already on the {{groovysh}} classpath
> (jline-builtins/jansi), so {{groovysh}} may use JLine's {{SafeRegex}}
> directly. For *core*
> Groovy the technique is ~20 lines and well-known, so reimplement the idea
> rather than take
> a core dependency on JLine. Do not copy code; implement from the technique.
> h2. References
> * JLine 4.3.1 release notes -- SafeRegex / TimeoutCharSequence:
> https://github.com/jline/jline3/releases/tag/4.3.1
> * CWE-1333: Inefficient Regular Expression Complexity:
> https://cwe.mitre.org/data/definitions/1333.html
> * OWASP -- Regular expression Denial of Service (ReDoS):
> https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
> * Related: GEP-25 (Data-flow Security -- taint tracking); {{RegexChecker}} in
> groovy-typecheckers; {{@TimedInterrupt}} / {{@ThreadInterrupt}} /
> {{@ConditionalInterrupt}} in groovy.transform
> * Relates to: GROOVY-12123 (constant regex hoisting -- compile-once static
> final Pattern fields): https://issues.apache.org/jira/browse/GROOVY-12123
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
