Repository navigation
[cherry-pick][4.10][CDAP-21270] Enable JEXL expressions allowlisting in Wrangler Directives - #1048
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a JEXL allowlisting mechanism in Wrangler to restrict the classes, methods, and properties that can be executed within JEXL expressions. It updates DirectiveConfig to include allowlist rules, integrates a JexlSandbox during script compilation in EL.java, and propagates compilation options across various directives and service handlers. Feedback on these changes highlights a critical security vulnerability in EL.java where the sandbox permission logic is inverted (incorrectly mapping blocklists and allowlists), as well as a potential NullPointerException in DirectivesHandler.java if the configuration store returns a null configuration.
| JexlSandbox.Permissions permissions = sandbox.permissions( | ||
| className, | ||
| !rule.blockAllProperties(), | ||
| !rule.blockAllProperties(), | ||
| !rule.blockAllMethods()); |
There was a problem hiding this comment.
The logic for configuring JEXL sandbox permissions is inverted. In JEXL 3, JexlSandbox.Permissions uses WhiteSet (allowlist) when the boolean parameter is true, and BlackSet (blocklist) when it is false.
Using !rule.blockAllMethods() means:
- If
blockAllMethods()istrue(block all), it passesfalse(BlackSet) and adds no methods, which actually allows all methods. - If
allowAllMethods()istrue(allow all), it passestrue(WhiteSet) and adds no methods, which actually blocks all methods.
This is a critical security vulnerability. It should be changed to use !rule.allowAllProperties() and !rule.allowAllMethods() respectively.
| JexlSandbox.Permissions permissions = sandbox.permissions( | |
| className, | |
| !rule.blockAllProperties(), | |
| !rule.blockAllProperties(), | |
| !rule.blockAllMethods()); | |
| JexlSandbox.Permissions permissions = sandbox.permissions( | |
| className, | |
| !rule.allowAllProperties(), | |
| !rule.allowAllProperties(), | |
| !rule.allowAllMethods()); |
| DirectiveConfig config = configStore.getConfig(); | ||
| if (!jexlAllowlistEnabled) { | ||
| JsonObject configJson = config.toJson().getAsJsonObject(); | ||
| configJson.remove(DirectiveConfig.JEXL_ALLOWLIST_KEY); | ||
| return new ServiceResponse<>(configJson); | ||
| } | ||
| return new ServiceResponse<>(config); |
There was a problem hiding this comment.
If configStore.getConfig() returns null (which is now possible when the database is not initialized), calling config.toJson() will throw a NullPointerException when jexlAllowlistEnabled is false.
We should add a null check and default to DirectiveConfig.EMPTY to make this endpoint robust.
DirectiveConfig config = configStore.getConfig();
if (config == null) {
config = DirectiveConfig.EMPTY;
}
if (!jexlAllowlistEnabled) {
JsonObject configJson = config.toJson().getAsJsonObject();
configJson.remove(DirectiveConfig.JEXL_ALLOWLIST_KEY);
return new ServiceResponse<>(configJson);
}
return new ServiceResponse<>(config);
Cherry pick for #1040