hussein-awala commented on PR #64610: URL: https://github.com/apache/airflow/pull/64610#issuecomment-4897869893
Hey @pierrejeambrun, I did the security deep-dive on the regex filtering. Summary and what I've done: ## The risk is real, and it's DB-side `partition_key_pattern` is only syntax-checked with Python `re.compile()`, then handed to the **database's own** regex engine via SQLAlchemy `regexp_match` (`~` on PostgreSQL, `REGEXP` on MySQL). So a malicious pattern burns **database** CPU, not app CPU, same impact profile as our prior ReDoS, **CVE-2023-36543** (authenticated user hangs a request), which was fixed by switching to `google-re2`. ## Per-engine picture (only the backends we support) - **PostgreSQL** (primary): Henry Spencer's engine -> more resistant to classic "evil regex" than PCRE, but *not* immune; PostgreSQL's own guidance is to bound it with `statement_timeout`. - **MySQL 8.0+**: ICU engine with built-in `regexp_time_limit`/`regexp_stack_limit` -> protected by default. - **MariaDB** (PCRE, the classic backtracking engine): explicitly **not supported** by Airflow -> so out of scope. - **SQLite**: dev-only, and Airflow doesn't even register a REGEXP function. ## Important subtlety about re2 re2 fixes ReDoS when *we* do the matching (like CVE-2023-36543's Python-side fix). Here the **database** matches, so a Python-side re2 can't protect the DB engine. The SQL-world equivalent of "an engine that stops backtracking" is a **statement timeout**. ## What I implemented 1. **Opt-in feature flag**: `[api] enable_regexp_query_filters`, **default `False`**. Regex filtering is off out of the box; a request using `partition_key_pattern` while disabled gets a 400. So there's zero attack surface unless an operator knowingly turns it on. Exact-match `partition_key` (B-tree indexed) is always available and unaffected. 2. **Bounded query timeout**: `[api] regexp_query_timeout` (seconds, default 30), enforced as a transaction-local `statement_timeout` on PostgreSQL so a pathological pattern is aborted instead of pinning a backend. MySQL relies on its built-in limit. `0` disables it. 3. Docs explain the security rationale for both settings; tests cover the disabled path and the timeout helper. (I initially also added a pattern-length cap but dropped it, it doesn't stop short catastrophic patterns like `(a+)+$` and risks rejecting legitimate ones, so it added little over the flag + timeout.) Net: the feature is safe-by-default (off), and when enabled it's bounded by a configurable timeout on the one backend that needs it. Happy to adjust the default timeout or add anything else you'd like. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
