diff --git a/framework-docs/modules/ROOT/pages/core/expressions/evaluation.adoc b/framework-docs/modules/ROOT/pages/core/expressions/evaluation.adoc index dfbb450f48b..7dc0764f3eb 100644 --- a/framework-docs/modules/ROOT/pages/core/expressions/evaluation.adoc +++ b/framework-docs/modules/ROOT/pages/core/expressions/evaluation.adoc @@ -286,6 +286,102 @@ registering property accessors, resolvers, variables, or functions – to ensure of the objects reachable via the context expose operations that would be dangerous if invoked by an expression from an untrusted source. +Furthermore, a property "getter" reachable from an expression is not necessarily a pure, +side-effect-free read operation. A JavaBean-style accessor (such as `getName()` or +`isActive()`) and a plain accessor method used to support data classes such as Java +records and Kotlin data classes (such as `name()`) are indistinguishable from a method +that performs an action and happens to return a value (that is, a method which is +*accessor-shaped*). For example, the `public boolean delete()` method in `java.io.File` +looks like a plain accessor method to SpEL. Specifically, neither +`ReflectivePropertyAccessor` nor `DataBindingPropertyAccessor` can determine whether such +a method is free of side effects. Moreover, restricting a `SimpleEvaluationContext` to +read-only data binding governs only whether *assignment* to a property is permitted: it +does not verify that reading a property is side-effect-free. When exposing a root object +or other reachable object to an untrusted expression, you must ensure that none of its +accessor-shaped methods perform an action that would be unsafe if triggered by that +expression. + +[NOTE] +.What makes a method "accessor-shaped"? +==== +A method is accessor-shaped if it is `public`, takes no arguments, and returns a value – +the same shape that `ReflectivePropertyAccessor` and `DataBindingPropertyAccessor` look +for when resolving a property "getter" by name. That shape says nothing about whether +invoking the method is actually free of side effects. For example, the following methods +are all accessor-shaped, but only some of them are safe to invoke as a property read. + +Side-effect-free (safe to expose as properties): + +* `getName()` and `isActive()`: conventional JavaBean-style accessors. +* `name()` and `active()`: plain accessor methods used by data classes such as Java + records and Kotlin data classes. + +Side-effecting (unsafe to expose as properties, despite the identical shape): + +* `java.io.File#delete()`: deletes the underlying file and returns whether the deletion + succeeded. +* `java.util.Queue#poll()`: removes and returns the head element, mutating the queue. +* `java.util.concurrent.atomic.AtomicInteger#incrementAndGet()`: increments and returns + a counter, mutating it. + +If an untrusted expression can reference `someFile.delete`, `someQueue.poll`, or +`someCounter.incrementAndGet` as a property, SpEL invokes the corresponding method just +as readily as it would invoke a genuine getter. +==== + +[[expressions-evaluation-context-object-design]] +=== Object Design + +Similar to the design guidance for +xref:web/webmvc/mvc-data-binding.adoc#mvc-data-binding-design[web data binding], you +should carefully design any object that may be reached from a SpEL expression evaluated +against untrusted input. This applies not only to the root object supplied to an +`EvaluationContext` but also to every object that such an expression can navigate to from +that root object – for example, an object returned by a property, a method, an index +operation, a variable, or a function. + +When exposing an object to expressions from an untrusted source, consider the following +recommendations. + +Use a dedicated type:: + Prefer a dedicated type, designed specifically to be evaluated against untrusted + expressions, over passing an existing domain or infrastructure type "as is". A + dedicated type lets you control exactly which properties and methods are reachable from + an expression, rather than exposing the full surface area of a class such as a JPA + entity, `java.io.File`, or a JDBC `Connection` – most of which were never designed with + SpEL evaluation in mind. + +Prefer immutability:: + An immutable type – for example, a Java record or a Kotlin data class exposing only + `val` properties – rules out property writes and eliminates any concern that a "getter" + might mutate state as a side effect, since there is no mutable state to affect. + Immutability does not, on its own, rule out an accessor-shaped method with an external + side effect (such as a network call or a file system operation), but it removes an + entire class of risk. + +Limit scope:: + Expose only the properties and methods that the expression is expected to use, and + nothing more. Because a `PropertyAccessor` cannot restrict access to specific + properties or methods on a per-expression basis, every accessor-shaped method reachable + on an exposed object is reachable by any expression that can reach that object – + regardless of which property or method the application intended the expression to use. + +Audit accessor-shaped methods:: + Review every accessor-shaped method exposed by a type before making it reachable from + an untrusted expression, keeping the <> discussed above in mind. None of the reachable methods should + perform an action that would be unsafe if triggered by that expression. + +[WARNING] +==== +These recommendations apply transitively. If the root object exposes a property or method +that returns another object, and an untrusted expression can navigate to it (for example, +`rootObject.child.grandchild`), the nested object is just as reachable as the root object +itself and must meet the same design requirements. The same is true for an object reached +via indexing (for example, `rootObject.items[0]` or `rootObject.items['key']`): whatever +is returned by the index operation is just as reachable as any other nested object. +==== + [[expressions-evaluation-context-lifecycle]] === Lifecycle and Reuse diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/DataBindingPropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/DataBindingPropertyAccessor.java index 79d647509db..30119d90a4e 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/DataBindingPropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/DataBindingPropertyAccessor.java @@ -29,6 +29,16 @@ import java.lang.reflect.Method; * resolve technical properties on {@code java.lang.Object} or {@code java.lang.Class}. * For unrestricted resolution, choose {@link ReflectivePropertyAccessor} instead. * + *

WARNING: Configuring this accessor for read-only access — + * for example, via {@link #forReadOnlyAccess()} or + * {@link SimpleEvaluationContext#forReadOnlyDataBinding()} — disallows + * assignment to a property but does not verify that reading a property is + * free of side effects. See the + * Security Considerations section of the Spring Framework reference + * documentation, as well as the class-level documentation for + * {@link ReflectivePropertyAccessor}, for details. + * * @author Juergen Hoeller * @since 4.3.15 * @see #forReadOnlyAccess() diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java index b9e2fcbd077..0b8fb34abc0 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java @@ -58,6 +58,19 @@ import org.springframework.util.StringUtils; * *

A property can be referenced through a public getter method (when being read) * or a public setter method (when being written), and also through a public field. + * A getter method may be a JavaBean-style accessor (for example, {@code getName()} + * or {@code isActive()}) or a plain accessor method used to support data classes + * such as Java records and Kotlin data classes (for example, {@code name()}). + * + *

WARNING: This accessor cannot determine, via reflection, whether + * a candidate getter method is a side-effect-free read of an underlying property or an + * action that happens to return a value — for example, {@code File.delete()} + * matches the same shape as a plain accessor method. See the + * Security Considerations and + * Object Design sections of the Spring Framework reference documentation for + * details. * * @author Andy Clement * @author Juergen Hoeller @@ -381,7 +394,9 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { method = findMethodForProperty(methodSuffixes, "is", clazz, mustBeStatic, 0, BOOLEAN_TYPES); if (method == null) { - // Record-style plain accessor method, for example, name() + // Plain accessor method for a data class, for example, a Java + // record component accessor or a Kotlin data class accessor + // such as name() method = findMethodForProperty(new String[] {propertyName}, "", clazz, mustBeStatic, 0, ANY_TYPES); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java index e621bb38ee2..fed658a2fa4 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java @@ -92,10 +92,16 @@ import org.springframework.expression.spel.SpelMessage; * the SpEL language to a subset of its features, that restriction is provided on * a best-effort basis and does not guarantee that evaluation of an expression is * safe; {@code SimpleEvaluationContext} must not be considered safe for evaluating - * a SpEL expression obtained from an untrusted source. See the + * a SpEL expression obtained from an untrusted source. This responsibility extends + * to property accessors: restricting a {@code SimpleEvaluationContext} to read-only + * data binding governs only whether assignment to a property is permitted + * and does not verify that reading a property is free of side effects. See the * Security Considerations section of the Spring Framework reference - * documentation for details. + * >Security Considerations and + * Object Design sections of the Spring Framework reference documentation, as well + * as the class-level documentation for {@link ReflectivePropertyAccessor} and + * {@link DataBindingPropertyAccessor}, for details. * *

Because a parsed {@code Expression} may cache accessor and executor state * resolved against a particular {@code EvaluationContext} configuration, a parsed