Secure Excecution of Groovy Code #14197

ege-st · 2024-10-09T20:32:38Z

Description

Adds security and safety checks for Groovy scripts by performing static analysis on the AST. When enabled this feature will check every
Groovy script sent to Pinot against an Allowed list of operations and classes. If a script uses anything which is not in the Allowed list then
it will be rejected by Pinot.

The allowed constructs can be managed via Cluster Config APIs. The administrator can define allow lists for: imports, static imports,
receivers, and set a list of disallowed method names. The imports lists define what constructs can be imported and used within a
Groovy script, this includes referencing a construct through it's full canonical name (e.g. System.out.println("hello")). The disallowed
methods list is provided to block calls to methods like invoke and execute which would allow a user to execute some malicious operations
such as shell commands.

The static analysis is performed by the org.codehaus.groovy.control.customizers.SecureASTCustomizer, which traverses the AST of a
Groovy script and applies configuration validation rules to make sure a Groovy script satisfies the security restrictions.

The configuration for the analyzer is stored in the Cluster Config under the pinot.server.query.groovy.analyzer.static field:

{
  "id": "pinot",
  "simpleFields": {
    "allowParticipantAutoJoin": "true",
    "default.hyperloglog.log2m": "8",
    "enable.case.insensitive": "true",
    "pinot.broker.enable.query.limit.override": "false",
    "pinot.broker.disable.query.groovy": "false",
    "pinot.server.query.groovy.analyzer.static": "{\"allowedReceivers\":[\"java.lang.String\",\"java.lang.Math\"],\"allowedImports\":[\"java.lang.Math\"],\"allowedStaticImports\":[\"java.lang.Math\"],\"disallowedMethodNames\":[\"execute\",\"invoke\"]}"
  },
  "mapFields": {},
  "listFields": {}
}

To enable the Analyzer, add a configuration to the pinot.server.query.groovy.analyzer.static field. If this field has a value then Pinot will
enable the static analyzer. This property can be easily configured by POSTing to the /cluster/groovy/analyzer/static/config API, with a
body like the following example:

// Example Static Analyzer Config.  This will allow basic arithmetic operations along with constructs within Math and String to be used.
// List operations, for example, would be blocked.
{
  "allowedReceivers": [
    "java.lang.String",
    "java.lang.Math"
  ],
  "allowedImports": [
    "java.lang.String",
    "java.lang.Math"
  ],
  "allowedStaticImports": [
    "java.lang.String",
    "java.lang.Math"
  ],
  "disallowedMethodNames": [
    "execute",
    "invoke"
  ]
}

The current config can be retrieved by GETting /cluster/groovy/analyzer/static/config. A recommended default configuration can be gotten
by GETting /cluster/groovy/analyzer/static/default.

Testing

Unit tests were added that check the analyzer with legal scripts, illegal scripts, and changing the configuration. Unit tests were also added
to validate the serialization and deserialization for configurations to and from JSON.

A Pinot image with this feature was deployed to a test cluster and verified that when enabled Groovy scripts with illegal operations were blocked
and legal operations were allowed, and, when disabled, all Groovy scripts were allowed. In addition, and independent engineer went through
and tested the analyzer against a set of malicious scripts they had compiled.

… specific method names

…alyzer

…er for groovy executor

…urity configuration

codecov-commenter · 2024-10-09T21:13:38Z

Codecov Report

Attention: Patch coverage is 51.20000% with 61 lines in your changes missing coverage. Please review.

Project coverage is 63.79%. Comparing base (59551e4) to head (e026c21).
Report is 1186 commits behind head on master.

Files with missing lines	Patch %	Lines
.../controller/api/resources/PinotClusterConfigs.java	0.00%	24 Missing ⚠️
...egment/local/function/GroovyFunctionEvaluator.java	69.64%	17 Missing ⚠️
.../pinot/server/starter/helix/BaseServerStarter.java	0.00%	14 Missing ⚠️
...ent/local/function/GroovyStaticAnalyzerConfig.java	80.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14197      +/-   ##
============================================
+ Coverage     61.75%   63.79%   +2.04%     
- Complexity      207     1535    +1328     
============================================
  Files          2436     2624     +188     
  Lines        133233   144560   +11327     
  Branches      20636    22109    +1473     
============================================
+ Hits          82274    92224    +9950     
- Misses        44911    45541     +630     
- Partials       6048     6795     +747

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.74% <51.20%> (+2.03%)`	⬆️
java-21	`63.68% <51.20%> (+2.06%)`	⬆️
skip-bytebuffers-false	`63.78% <51.20%> (+2.04%)`	⬆️
skip-bytebuffers-true	`63.64% <51.20%> (+35.92%)`	⬆️
temurin	`63.79% <51.20%> (+2.04%)`	⬆️
unittests	`63.79% <51.20%> (+2.04%)`	⬆️
unittests1	`55.54% <73.56%> (+8.65%)`	⬆️
unittests2	`34.29% <8.80%> (+6.56%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jackie-Jiang · 2024-10-09T21:06:35Z

...gment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyMethodSanitizer.java

+import org.codehaus.groovy.classgen.BytecodeExpression;
+
+
+public class GroovyMethodSanitizer implements GroovyCodeVisitor {


This class can be significantly simplified if extending CodeVisitorSupport.

Why do we want to block on method names? Is this mostly for flexibility?

I'll replace this with this feature: https://docs.groovy-lang.org/latest/html/api/org/codehaus/groovy/control/customizers/SecureASTCustomizer.ExpressionChecker.html

There are methods like execute (which let's a user execute a string as a shell command) and invoke which need to be blocked but can't be blocked with the other rule sets. We want people to be able to use Strings and string operations but want to block execute, so adding this Visitor or adding an ExpressionChecker rule to block any call to methods in the disallow list is what I went with here.

Jackie-Jiang · 2024-10-09T21:07:03Z

pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java

+        "Groovy({return [\"bash\", \"-c\", \"env\"].execute().text})"
+    );
+
+    final GroovyStaticAnalyzerConfig config = new GroovyStaticAnalyzerConfig(


(minor, convention) We don't usually use final in local variable, same for other places

(format) This doesn't follow Pinot Style, please reformat the changes

Just working shopping how an immutable by default approach to Java would work, I'll get rid of it.

Jackie-Jiang · 2024-10-09T21:08:25Z

...-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java

+
+public class GroovyStaticAnalyzerConfig {
+  final boolean _enabled;
+  final private List<String> _allowedReceivers;


(convention) Put private before final

Jackie-Jiang · 2024-10-09T21:08:58Z

...-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java

+
+
+public class GroovyStaticAnalyzerConfig {
+  final boolean _enabled;


enabled is not really useful. Let's remove it and it should be enabled whenever the config exists

Jackie-Jiang · 2024-10-09T21:09:59Z

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java

+        manager.setConfig(config);
+      }
+    } catch (Exception _ex) {
+      LOGGER.error("Failed to read config from ZK. Loading Default configuration.");


Directly throw exception instead of logging error. We want to fail the server start if security is not guaranteed

Jackie-Jiang · 2024-10-09T21:10:56Z

...local/src/main/java/org/apache/pinot/segment/local/function/GroovySecurityConfigManager.java

+        .expireAfterWrite(5, TimeUnit.MINUTES)
+        .build(new CacheLoader<>() {
+          @Override
+          @Nonnull


(convention) We don't usually annotate @Nonnull, but only annotate @Nullable

This was added to match the CacheLoader interface, but will remove.

Jackie-Jiang · 2024-10-09T21:17:02Z

...-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java

+  }
+
+  public ZNRecord toZNRecord() throws JsonProcessingException {
+    ZNRecord record = new ZNRecord("groovySecurityConfiguration");


We probably don't want this config to be a separate ZNode. We can consider putting it as a field in the cluster config

Jackie-Jiang · 2024-10-09T21:26:31Z

I find this post very similar to what we wanted: https://melix.github.io/blog/2011/05/12/customizing_groovy_compilation_process.html
Are you following the steps in this post?

code clean up.

ege-st · 2024-10-11T13:57:43Z

Not quite sure what is happening with the failing tests, looks like it's some CLP tests that are failing.

npawar · 2024-10-13T00:35:43Z

pinot.server.query.groovy.analyzer.static

this covers both ingestion time and query time groovy i assume? having it start with "pinot.server.query" would be confusing

npawar · 2024-10-13T00:39:27Z

/cluster/groovy/analyzer/static/config API,

Is this the right convention to use for such an API? Only until cluster/config/groovy are we doing a hierarchy, so perhaps something like cluster/configs/groovystaticConfig is a more apt name?

ege-st · 2024-10-15T00:14:09Z

Good catch. Will fix.

…

On Sat, Oct 12, 2024 at 8:36 PM Neha Pawar ***@***.***> wrote: pinot.server.query.groovy.analyzer.static this covers both ingestion time and query time groovy i assume? having it start with "pinot.server.query" would be confusing — Reply to this email directly, view it on GitHub <#14197 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BAASDJ7SYAMR7XZOTGADAZTZ3G57LAVCNFSM6AAAAABPVKS7F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYG43DGNZUGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ege-st · 2024-10-15T00:15:54Z

/cluster/groovy/analyzer/static/config API,

Is this the right convention to use for such an API? Only until cluster/config/groovy are we doing a hierarchy, so perhaps something like cluster/configs/groovystaticConfig is a more apt name?

I initially had that, but then I thought we might add additional Groovy security features (such as executing in a sandbox) in the future, and grouping Groovy configs under cluster/configs/groovy/ would be more future proof.

perhaps /cluster/configs/groovy/staticConfig would be better.

… and ingestion, so "query" is too restrictive)

soumitra-st · 2024-10-15T18:02:43Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java

@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
      throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
    }
  }
+
+  @GET
+  @Path("/cluster/groovy/analyzer/static/default")


The path looks long, shall we use a shorter path? /cluster/config/groovy...

Do we need a separate API to the default config? IMO, https://github.com/apache/pinot/pull/14197/files#diff-510a3030bf6c520bbe87caa12b6eff67938c4299feef0c1433f913da810081a3R189 API should get the current active config.

Current design is that if the feature is disabled if there is no config and enabled if there is a config (per a comment on an older version of this PR). Hence adding a method to easily get a default config that can then be used to turn on this feature.

Alternatively, a query param could be added to the Set method which, when true, will set the configuration to the Default configuration and if false will use the config provided in the request body.

soumitra-st · 2024-10-15T18:03:22Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java

+  }
+
+  @GET
+  @Path("/cluster/groovy/analyzer/static/config")


Same here, use shorter path.

perhaps we can assume that anything under /cluster is a config and use cluster/groovy/staticAnalysis.

soumitra-st · 2024-10-15T18:05:39Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java

+      @ApiResponse(code = 500, message = "Internal server error")
+  })
+  public GroovyStaticAnalyzerConfig getDefaultGroovyStaticAnalysisConfig() {
+    return GroovyStaticAnalyzerConfig.createDefault();


What is the default security config, and is it backward compatible?

Starting here: https://github.com/apache/pinot/pull/14197/files#diff-106af3bbf7f8d5f13d3ccb902ed2cfc28edb30931dfeb0ac8d64a7a64c373cfeR97

Assuming backwards compatible means that it won't break exist (safe) scripts, then testing so far looks good, but I'm working with Minesh to see what functions current customers are running in Groovy scripts.

Jackie-Jiang · 2025-02-22T02:31:34Z

Close this one and prefer #14844

ege-st added 27 commits October 3, 2024 11:16

groovy static checker poc

db67ed3

Replace safety checker with the Groovy provided SecureASTCustomizer

c748e00

wip

5aaf3aa

Clean up

0b9b588

Add flag to enable analyzer

031ec3e

clean up

cc11e08

Add config for static analysis. Add a method sanitizer that can block…

000bc9a

… specific method names

Adding tests and a method sanitizer

058079e

Basic unit tests

9ccbcf8

Throw exceptions when scripts are malicious

1695a7b

Fix tests

32fa852

Adding Jackson annotations to config class.

3e9ead0

Add a singleton method for setting the configuration of the groovy an…

2fe6f7a

…alyzer

Config serialization tests

db351d4

clean up

08ea5e1

spotless

6c83de4

Code clean up and style fixes

bb72541

add note to self for expanding tests

fee6974

WIP: trying to figure out how to make a expiring/caching config manag…

e55aa81

…er for groovy executor

Convert config to a ZN Record

4204d90

Adding stub for configuration manager integration test

8706e16

code clean up

c9cdcb7

Licenses

1bc688f

Obfuscate error message

5011515

WIP: temp code to configure groovy checker at start up

6056db5

Initialize groovy config at start up

6b28e42

Load config from ZooKeeper. Add APIs to get and update the Groovy Sec…

301941d

…urity configuration

Jackie-Jiang reviewed Oct 9, 2024

View reviewed changes

ege-st added 12 commits October 10, 2024 10:18

Remove unused imports

6482acf

If config failed to load bubble exception up and crash server.

4accb9f

code clean up.

If config failed to load bubble exception up and crash server.

75db8e6

log the groovy config at start up

f3be556

Allow config to be updated instead of just set once

1960fd4

Removing the enabled property from the groovy config

6edf90c

updating the apis to have more restful paths

c2a80b5

Provide API for getting the default configuration

3a06a5d

Simplify the config system.

6e98510

Simplify the config system.

d58d6c2

removing finals

eb095d6

Merge branch 'master' into groovy_injection_protection_cache

caaae1a

ege-st marked this pull request as ready for review October 11, 2024 13:52

ege-st mentioned this pull request Oct 11, 2024

PoC for checking Groovy scripts for shell script injection attacks #14103

Closed

ege-st added 4 commits October 11, 2024 17:49

Add map to the allowed classes

731bcb5

Code clean up

43f06b3

Add a counter for the number of groovy violations

33da492

change what the synchronize is on

1813009

Remove "query" from the groovy config key name (it applies to queries…

e026c21

… and ingestion, so "query" is too restrictive)

soumitra-st reviewed Oct 15, 2024

View reviewed changes

Jackie-Jiang mentioned this pull request Feb 7, 2025

Support for groovy static analysis for groovy scripts #14844

Open

Jackie-Jiang closed this Feb 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure Excecution of Groovy Code #14197

Secure Excecution of Groovy Code #14197

ege-st commented Oct 9, 2024 •

edited

Loading

codecov-commenter commented Oct 9, 2024 •

edited

Loading

Jackie-Jiang Oct 9, 2024

ege-st Oct 9, 2024

Jackie-Jiang Oct 9, 2024

Jackie-Jiang Oct 9, 2024

ege-st Oct 9, 2024

Jackie-Jiang Oct 9, 2024

Jackie-Jiang Oct 9, 2024

ege-st Oct 9, 2024

Jackie-Jiang Oct 9, 2024

Jackie-Jiang Oct 9, 2024

ege-st Oct 9, 2024

Jackie-Jiang Oct 9, 2024

ege-st Oct 9, 2024

Jackie-Jiang commented Oct 9, 2024

ege-st commented Oct 11, 2024

npawar commented Oct 13, 2024

npawar commented Oct 13, 2024

ege-st commented Oct 15, 2024 via email

ege-st commented Oct 15, 2024

soumitra-st Oct 15, 2024

soumitra-st Oct 15, 2024

ege-st Oct 15, 2024

soumitra-st Oct 15, 2024

ege-st Oct 15, 2024

soumitra-st Oct 15, 2024

ege-st Oct 15, 2024

Jackie-Jiang commented Feb 22, 2025

		import org.codehaus.groovy.classgen.BytecodeExpression;


		public class GroovyMethodSanitizer implements GroovyCodeVisitor {



		public class GroovyStaticAnalyzerConfig {
		final boolean _enabled;

Secure Excecution of Groovy Code #14197

Secure Excecution of Groovy Code #14197

Conversation

ege-st commented Oct 9, 2024 • edited Loading

Description

Testing

codecov-commenter commented Oct 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang commented Oct 9, 2024

ege-st commented Oct 11, 2024

npawar commented Oct 13, 2024

npawar commented Oct 13, 2024

ege-st commented Oct 15, 2024 via email

ege-st commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang commented Feb 22, 2025

ege-st commented Oct 9, 2024 •

edited

Loading

codecov-commenter commented Oct 9, 2024 •

edited

Loading