Skip to content

Conversation

ptrgits
Copy link

@ptrgits ptrgits commented Sep 10, 2025

fix the problem must ensure that OGNL expressions resolved and evaluated by Struts cannot originate from untrusted user input (i.e., header names, attribute names, etc.) unless robust validation is applied. The best way is to validate the OGNL expression specifically before parsing and evaluating, according to a whitelist of allowed expression patterns or types. This can be achieved by enhancing the OGNL guard with additional logic: if the expression is derived from a request header or attribute name, block its evaluation unless it passes strict validation (such as allowing only property lookup, no method calls, no reflection, and so forth).

To implement this:

  1. Add a utility method to check (validate) that an OGNL expression is safe—e.g., only matches a whitelist pattern.
  2. In the critical region, before parsing/evaluating an OGNL expression (e.g., in OgnlUtil.ognlGet, or, better, in the OgnlGuard itself), invoke this validation and block the expression if not safe.
  3. Optionally, maintain a blacklist of known dangerous OGNL strings or patterns.
  4. The fix needs to be in OgnlGuard (isRawExpressionBlocked(String expr)) and called during parsing as designed (but ensure its implementation), and possibly in OgnlUtil to catch unsafe situations not covered by configuration.
  5. Add a simple pattern that allows bare property lookup (e.g., ^\w+$) and blocks expressions containing (, ), method calls, array access, or reflection (@, #, . except for single property), etc.

Only modify snippets from files you're shown, notably the interface and wherever its implementation is likely (within interface itself for default, but ideally in a concrete implementing class; add a strong default if none exists).

Refferences

Apache Commons OGNL
Proactively protect from OGNL Expression Injections attacks

Co-Authored-By: Jörmungandrk <github@zerodaysec.org>
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements security fixes for OGNL expression language injection vulnerabilities by adding validation to block unsafe expressions from user-controlled input. The changes enhance the OGNL guard system to prevent malicious expressions containing method calls, reflection, or other dangerous operations from being executed.

Key changes:

  • Converts StrutsOgnlGuard class to the OgnlGuard interface with default security implementations
  • Adds raw expression validation that only allows simple property lookups (alphanumeric and underscores)
  • Integrates validation checks in OgnlUtil.ognlGet() to block unsafe expressions before parsing

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
StrutsOgnlGuard.java Converted from implementation class to interface with default security validation methods
OgnlUtil.java Added pre-parsing validation check to block unsafe expressions in the ognlGet method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +71 to +79
/**
* Determines whether the OGNL expression is blocked based on the raw string.
* By default blocks any expression except simple attribute/bean property lookups
* (alphanumeric and underscores only, no symbols, dots, method calls, array access, etc).
* Override this for stricter/looser heuristics.
*
* @param expr OGNL expression
*/
default boolean isRawExpressionBlocked(String expr) {
Copy link
Preview

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are duplicate Javadoc comments for the isRawExpressionBlocked method. Lines 64-70 contain the first Javadoc comment, and lines 71-78 contain a second one. Remove the duplicate documentation to avoid confusion.

Copilot uses AI. Check for mistakes.

import static org.apache.struts2.util.TextParseUtil.commaDelimitedStringToSet;
import static java.util.Collections.emptySet;
import static java.util.Collections.unmodifiableSet;
import org.apache.struts2.ognl.OgnlUtil;
Copy link
Preview

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import for org.apache.struts2.ognl.OgnlUtil creates a circular dependency since this interface is used by OgnlUtil. Remove this unnecessary import as it's not used in the interface definition.

Suggested change
import org.apache.struts2.ognl.OgnlUtil;

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants