This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch UNOMI-972-credentials-profile-binding-privileged-rest in repository https://gitbox.apache.org/repos/asf/unomi.git
commit f5b60ab068bd6b5fbccefae8c62ae1f628119985 Author: Serge Huber <[email protected]> AuthorDate: Mon Aug 10 09:26:38 2026 +0200 UNOMI-972: restrict Groovy action upload to system administrators Reported issue 4. POST /cxs/groovyActions carried no @RequiresRole, and SecurityFilter only enforces where that annotation is present, so any authenticated subject reached it - including a tenant administrator, whose authority is meant to stop at its own tenant's data. The endpoint now requires the system ADMINISTRATOR role; tenant administrators are deliberately excluded, unlike the profile-binding trust check. The report also showed the upload itself executing. GroovyShell#parse returns a Script *instance*, and constructing it runs the script's field initializers, so a script with a Groovy @Field initializer ran at save time, before any rule dispatched it. Every caller here only needs the compiled Class - to read the @Action annotation or check for execute() - so compilation now uses parseClass and instantiates nothing. This also closes a second occurrence: the cache-refresh path re-instantiated persisted scripts on every refresh cycle. The regression test carries the reported payload shape and is preceded by a positive control that runs the same payload through a bare GroovyShell, so a payload that silently failed to execute could not make the assertion pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../karaf-kar/src/main/feature/feature.xml | 2 + extensions/groovy-actions/rest/pom.xml | 10 +++ .../groovy/actions/rest/GroovyActionsEndPoint.java | 5 ++ .../rest/GroovyActionsEndPointRoleTest.java | 37 ++++++++++ .../services/impl/GroovyActionsServiceImpl.java | 23 +++++- .../impl/GroovyActionsServiceImplTest.java | 84 ++++++++++++++++++++++ .../cxs/actions/fieldInitializerAction.groovy | 27 +++++++ .../META-INF/cxs/actions/rceProofAction.groovy | 37 ++++++++++ 8 files changed, 222 insertions(+), 3 deletions(-) diff --git a/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml b/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml index 5d527b1e8..c16201eda 100644 --- a/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml +++ b/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml @@ -20,6 +20,8 @@ <details>${project.description}</details> <feature>wrap</feature> <feature>unomi-services</feature> + <!-- REST endpoints use @RequiresRole from org.apache.unomi.rest.security --> + <feature>unomi-rest-api</feature> <configfile finalname="/etc/org.apache.unomi.groovy.actions.cfg">mvn:org.apache.unomi/unomi-groovy-actions-services/${project.version}/cfg/groovyactionscfg</configfile> <bundle start="false">mvn:org.apache.unomi/unomi-groovy-actions-services/${project.version}</bundle> <bundle start="false">mvn:org.apache.unomi/unomi-groovy-actions-rest/${project.version}</bundle> diff --git a/extensions/groovy-actions/rest/pom.xml b/extensions/groovy-actions/rest/pom.xml index 3bedcc985..b6aef5cd3 100644 --- a/extensions/groovy-actions/rest/pom.xml +++ b/extensions/groovy-actions/rest/pom.xml @@ -57,6 +57,11 @@ <artifactId>unomi-groovy-actions-services</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-rest</artifactId> + <scope>provided</scope> + </dependency> <dependency> <groupId>org.osgi</groupId> @@ -106,6 +111,11 @@ <artifactId>slf4j-api</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>org.junit.jupiter</groupId> + <artifactId>junit-jupiter</artifactId> + <scope>test</scope> + </dependency> </dependencies> <build> <plugins> diff --git a/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java b/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java index 1f41cf6f3..635dfe3d4 100644 --- a/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java +++ b/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java @@ -21,7 +21,9 @@ import org.apache.commons.io.IOUtils; import org.apache.cxf.jaxrs.ext.multipart.Attachment; import org.apache.cxf.jaxrs.ext.multipart.Multipart; import org.apache.cxf.rs.security.cors.CrossOriginResourceSharing; +import org.apache.unomi.api.security.UnomiRoles; import org.apache.unomi.groovy.actions.services.GroovyActionsService; +import org.apache.unomi.rest.security.RequiresRole; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; import org.slf4j.Logger; @@ -35,6 +37,7 @@ import java.io.IOException; @Produces(MediaType.APPLICATION_JSON + ";charset=UTF-8") @CrossOriginResourceSharing(allowAllOrigins = true, allowCredentials = true) @Path("/groovyActions") +@RequiresRole(UnomiRoles.ADMINISTRATOR) @Component(service = GroovyActionsEndPoint.class, property = "osgi.jaxrs.resource=true") public class GroovyActionsEndPoint { @@ -55,6 +58,7 @@ public class GroovyActionsEndPoint { * Uploads a Groovy action script and registers a matching action type. * <p> * The multipart field {@code file} must be a {@code .groovy} file; the action id is derived from the filename. + * Restricted to system administrators (JAAS); tenant administrators cannot upload scripts. * * @param file the Groovy script upload * @return an empty success response @@ -79,6 +83,7 @@ public class GroovyActionsEndPoint { /** * Deletes the Groovy action and its action type entry. + * Restricted to system administrators (JAAS). * * @param actionId the action identifier * @api.status 204 empty Action deleted. diff --git a/extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java b/extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java new file mode 100644 index 000000000..92ffea28d --- /dev/null +++ b/extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.groovy.actions.rest; + +import org.apache.unomi.api.security.UnomiRoles; +import org.apache.unomi.rest.security.RequiresRole; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Regression: Groovy upload/delete must stay restricted to system administrators. + */ +class GroovyActionsEndPointRoleTest { + + @Test + void endpointRequiresSystemAdministratorRole() { + RequiresRole requiresRole = GroovyActionsEndPoint.class.getAnnotation(RequiresRole.class); + assertNotNull(requiresRole, "GroovyActionsEndPoint must declare @RequiresRole"); + assertArrayEquals(new String[]{UnomiRoles.ADMINISTRATOR}, requiresRole.value()); + } +} diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index 40c02a7ae..09307e038 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -301,7 +301,7 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im // Extract Action annotation and register the ActionType try { synchronized(compilationLock) { - Action actionAnnotation = compilationShell.parse(groovyCodeSource).getClass().getMethod("execute").getAnnotation(Action.class); + Action actionAnnotation = compileToClass(groovyCodeSource).getMethod("execute").getAnnotation(Action.class); if (actionAnnotation != null) { contextManager.executeAsSystem(() -> { saveActionType(actionAnnotation); @@ -407,7 +407,7 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im try { GroovyCodeSource groovyCodeSource = new GroovyCodeSource(script, actionName, "/groovy/script"); synchronized(compilationLock) { - compilationShell.parse(groovyCodeSource).getClass().getMethod("execute"); + compileToClass(groovyCodeSource).getMethod("execute"); } // Note: We don't extract or save the ActionType here } catch (NoSuchMethodException e) { @@ -471,16 +471,33 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im } } + /** * Thread-safe script compilation using synchronized shared shell. */ private Class<? extends Script> compileScript(String actionName, String scriptContent) { GroovyCodeSource codeSource = new GroovyCodeSource(scriptContent, actionName, "/groovy/script"); synchronized(compilationLock) { - return compilationShell.parse(codeSource).getClass(); + return compileToClass(codeSource); } } + + /** + * Compiles a script to its Class without instantiating it. + * <p> + * Deliberately {@code parseClass} and not {@code GroovyShell#parse}: {@code parse} returns a + * {@code Script} <em>instance</em>, and constructing that instance runs the script's field + * initializers. An uploaded script carrying a Groovy {@code @Field} initializer would therefore + * execute at upload/compile time, before any rule ever dispatches it. Every caller here only + * needs the compiled Class (to read the {@code @Action} annotation or check for {@code execute}), + * so nothing needs to be instantiated until the action is actually run. + */ + @SuppressWarnings("unchecked") + private Class<? extends Script> compileToClass(GroovyCodeSource codeSource) { + return (Class<? extends Script>) compilationShell.getClassLoader().parseClass(codeSource, false); + } + /** * Compiles a script and creates metadata with timing information. */ diff --git a/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java b/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java index 94536a296..1336926ce 100644 --- a/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java +++ b/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java @@ -16,6 +16,7 @@ */ package org.apache.unomi.groovy.actions.services.impl; +import groovy.lang.GroovyShell; import groovy.lang.Script; import org.apache.unomi.api.Event; import org.apache.unomi.api.ExecutionContext; @@ -46,11 +47,15 @@ import org.osgi.framework.BundleContext; import org.osgi.framework.wiring.BundleWiring; import java.net.URL; +import java.io.File; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import java.util.*; import static org.junit.Assert.*; +import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -172,6 +177,85 @@ public class GroovyActionsServiceImplTest { }); } + /** + * Saving an action compiles it; it must not instantiate it. A Groovy {@code @Field} initializer + * runs at instantiation, so if save() ever goes back to {@code GroovyShell#parse} an uploaded + * script executes at upload time, before any rule dispatches it — the reported RCE vector. + */ + @Test + public void testSaveDoesNotRunFieldInitializers() throws Exception { + System.clearProperty("unomi.test.groovyFieldInitializerRan"); + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/fieldInitializerAction.groovy", + "Could not find the field-initializer test Groovy action file"); + try { + contextManager.executeAsTenant(TENANT_1, () -> { + groovyActionsService.save("fieldInitializerAction", groovyScript); + }); + assertNull("Saving a Groovy action must compile it without instantiating it, so a @Field " + + "initializer must not execute at upload time", + System.getProperty("unomi.test.groovyFieldInitializerRan")); + } finally { + System.clearProperty("unomi.test.groovyFieldInitializerRan"); + } + } + + /** + * Regression test for the reported upload-time RCE: uploading a Groovy action whose {@code @Field} + * initializer runs an OS command used to execute that command at save time, as the server user, + * before any rule dispatched the action. + * <p> + * The positive control matters as much as the assertion. It runs the same payload through a plain + * {@link GroovyShell} first and requires the proof file to appear, which establishes that command + * execution really does work on this machine. Without it, a payload that silently failed to run — + * wrong shell, restricted environment — would make the real assertion pass while proving nothing. + * <p> + * Scope: this covers execution at <em>upload</em> time, which is the reported vector. It does not + * claim the action is sandboxed when it is later dispatched — a Groovy action is arbitrary code by + * design, and uploading one now requires the system ADMINISTRATOR role. + */ + @Test + public void testSaveDoesNotExecuteUploadedCommands() throws Exception { + assumeTrue("requires a POSIX shell to run the payload", new File("/bin/sh").canExecute()); + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/rceProofAction.groovy", + "Could not find the RCE proof test Groovy action file"); + + Path tempDir = Files.createTempDirectory("unomi-rce-proof"); + Path proof = tempDir.resolve("rce-proof"); + System.setProperty("unomi.test.rceProofPath", proof.toString()); + try { + // Positive control: instantiating the script DOES run the payload, so the payload is live. + new GroovyShell().parse(stripActionAnnotation(groovyScript)); + assertTrue("positive control failed: the payload did not execute even via GroovyShell#parse, " + + "so the real assertion below would prove nothing on this machine", + Files.exists(proof)); + Files.delete(proof); + + // The actual assertion: saving the very same script must not run it. + contextManager.executeAsTenant(TENANT_1, () -> { + groovyActionsService.save("rceProofAction", groovyScript); + }); + + assertFalse("Uploading a Groovy action must not execute it: the payload wrote " + proof + + " at save time, which is remote code execution at upload", + Files.exists(proof)); + } finally { + System.clearProperty("unomi.test.rceProofPath"); + Files.deleteIfExists(proof); + Files.deleteIfExists(tempDir); + } + } + + /** + * Drops the {@code @Action} line so the positive control compiles under a bare {@link GroovyShell}, + * which has neither the service's ImportCustomizer nor its script base class. The {@code @Field} + * payload — the only part under test — is untouched. + */ + private static String stripActionAnnotation(String script) { + return script.replaceAll("(?m)^@Action\\(.*\\)$", ""); + } + @Test public void testRemoveGroovyAction() throws Exception { // First save an action diff --git a/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy b/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy new file mode 100644 index 000000000..92f96783e --- /dev/null +++ b/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import groovy.transform.Field + +// Stand-in for the reported upload-time RCE payload: a @Field initializer runs when the script class +// is *instantiated*. Saving this action must compile it without instantiating it, so this must NOT +// run. It writes a system property rather than executing a command so the test stays harmless. +@Field def sideEffect = { System.setProperty("unomi.test.groovyFieldInitializerRan", "true") }() + +@Action(id = "fieldInitializerAction", actionExecutor = "groovy:fieldInitializerAction") +def execute() { + return EventService.NO_CHANGE +} diff --git a/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/rceProofAction.groovy b/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/rceProofAction.groovy new file mode 100644 index 000000000..4bc5a7fdf --- /dev/null +++ b/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/rceProofAction.groovy @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import groovy.transform.Field + +// Faithful stand-in for the reported upload-time RCE payload, which was: +// @Field def proof = { new File("/tmp/rce_proof_id").text = ["bash","-c","id"].execute().text }() +// A Groovy @Field initializer runs when the script class is INSTANTIATED, so uploading this used to +// execute a command at save time, before any rule dispatched the action. Saving it must compile the +// script without instantiating it, so this must never run. +// +// The command is a harmless `echo` and the target path is injected by the test rather than +// hard-coded, so the payload cannot write outside the test's own temp directory. +@Field def proof = { + String target = System.getProperty("unomi.test.rceProofPath") + if (target != null) { + new File(target).text = ["sh", "-c", "echo pwned-at-upload-time"].execute().text + } +}() + +@Action(id = "rceProofAction", actionExecutor = "groovy:rceProofAction") +def execute() { + return EventService.NO_CHANGE +}
