diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/generic/GenericPublisher.java b/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/generic/GenericPublisher.java index fcfe7dd2ef3..3ef9076b7f1 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/generic/GenericPublisher.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/generic/GenericPublisher.java @@ -57,6 +57,13 @@ public class GenericPublisher implements Destination { this.eventSubscription = eventSubscription; this.subscriptionDestination = subscriptionDestination; this.webhook = JsonUtils.convertValue(subscriptionDestination.getConfig(), Webhook.class); + + // Validate webhook URL to prevent SSRF + if (this.webhook != null && this.webhook.getEndpoint() != null) { + org.openmetadata.service.util.URLValidator.validateURL( + this.webhook.getEndpoint().toString()); + } + this.client = getClient(subscriptionDestination.getTimeout(), subscriptionDestination.getReadTimeout()); } else { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/subscription/EventSubscriptionMapper.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/subscription/EventSubscriptionMapper.java index 3e35c5d6c24..e78dfc83b2a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/subscription/EventSubscriptionMapper.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/subscription/EventSubscriptionMapper.java @@ -41,8 +41,18 @@ public class EventSubscriptionMapper } private String validateConsumerClass(String className) { + // Validate that the class belongs to our application package + if (!className.startsWith("org.openmetadata.")) { + throw new BadRequestException("Only classes from org.openmetadata package are allowed"); + } + try { - Class.forName(className).asSubclass(AbstractEventConsumer.class); + // Check if the class exists and is a subclass of AbstractEventConsumer + Class clazz = Class.forName(className); + if (!AbstractEventConsumer.class.isAssignableFrom(clazz)) { + throw new BadRequestException( + "Class must be a subclass of AbstractEventConsumer: " + className); + } return className; } catch (ClassNotFoundException e) { throw new BadRequestException("Consumer class not found: " + className); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java index 2a2c25c2c0a..9e5e5dcbaaa 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java @@ -40,6 +40,10 @@ public class CompiledRule extends Rule { if (condition == null) { return null; } + + // Apply safety validation before parsing expressions + ExpressionValidator.validateExpressionSafety(condition); + try { return EXPRESSION_PARSER.parseExpression(condition); } catch (Exception exception) { @@ -53,6 +57,8 @@ public class CompiledRule extends Rule { if (condition == null) { return; } + + // parseExpression already includes safety validation Expression expression = parseExpression(condition); RuleEvaluator ruleEvaluator = new RuleEvaluator(); SimpleEvaluationContext context = diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ExpressionValidator.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ExpressionValidator.java new file mode 100644 index 00000000000..e87aa1f976d --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ExpressionValidator.java @@ -0,0 +1,157 @@ +/* + * Copyright 2021 Collate + * Licensed 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.openmetadata.service.security.policyevaluator; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import lombok.extern.slf4j.Slf4j; +import org.openmetadata.schema.Function; + +/** + * Utility class for validating SpEL expressions to prevent code injection. + */ +@Slf4j +public class ExpressionValidator { + // Cache of allowed function names from RuleEvaluator class + private static final Set ALLOWED_FUNCTIONS = initAllowedFunctions(); + + // Patterns that indicate potentially dangerous expressions + private static final List DANGEROUS_PATTERNS = + Arrays.asList( + "T(", + "new ", + "System.", + "Runtime", + "getClass", + "ClassLoader", + "forName", + "exec", + "eval", + "ProcessBuilder", + "java.lang.reflect"); + + private static Set initAllowedFunctions() { + Set allowedFunctions = new HashSet<>(); + try { + // Classes that provide functions for policy expressions + List> evaluatorClasses = + Arrays.asList( + RuleEvaluator.class, + Class.forName("org.openmetadata.service.events.subscription.AlertsRuleEvaluator")); + + for (Class evaluatorClass : evaluatorClasses) { + scanClassForFunctions(evaluatorClass, allowedFunctions); + } + + LOG.info("Initialized {} allowed functions for policy expressions", allowedFunctions.size()); + } catch (Exception e) { + LOG.error("Failed to initialize allowed functions", e); + // Fallback to hardcoded list if reflection fails + allowedFunctions.addAll( + Arrays.asList( + "noOwner", + "isOwner", + "hasDomain", + "matchAllTags", + "matchAnyTag", + "matchAnyCertification", + "matchTeam", + "inAnyTeam", + "hasAnyRole", + "matchAnyEventType", + "matchAnyFieldChange", + "matchAnySource", + "matchUpdatedBy", + "matchAnyOwnerName", + "matchAnyEntityFqn", + "matchAnyEntityId", + "matchTestResult", + "filterByTableNameTestCaseBelongsTo", + "getTestCaseStatusIfInTestSuite", + "matchIngestionPipelineState", + "matchPipelineState", + "matchAnyDomain", + "matchConversationUser")); + LOG.info("Using fallback list of {} allowed functions", allowedFunctions.size()); + } + return allowedFunctions; + } + + private static void scanClassForFunctions(Class clazz, Set allowedFunctions) { + try { + for (Method method : clazz.getDeclaredMethods()) { + if (method.isAnnotationPresent(Function.class)) { + Function annotation = method.getAnnotation(Function.class); + allowedFunctions.add(annotation.name()); + LOG.debug("Added allowed function from {}: {}", clazz.getSimpleName(), annotation.name()); + } + } + } catch (Exception e) { + LOG.warn("Failed to scan functions from class {}", clazz.getName(), e); + } + } + + public static void validateExpressionSafety(String expression) { + if (expression == null || expression.trim().isEmpty()) { + return; + } + + // Check for dangerous patterns + for (String pattern : DANGEROUS_PATTERNS) { + if (expression.contains(pattern)) { + throw new IllegalArgumentException( + "Expression contains potentially unsafe pattern: " + + pattern + + ". " + + "Only use approved policy functions with @Function annotations."); + } + } + + // Extract function calls from the expression + Pattern functionPattern = Pattern.compile("\\b([a-zA-Z0-9_]+)\\s*\\("); + Matcher matcher = functionPattern.matcher(expression); + + List foundFunctions = new ArrayList<>(); + while (matcher.find()) { + String functionName = matcher.group(1); + // Skip empty function names and logical operators + if (!functionName.isEmpty() + && !functionName.equals("and") + && !functionName.equals("or") + && !functionName.equals("not")) { + foundFunctions.add(functionName); + // Check if function is allowed + if (!ALLOWED_FUNCTIONS.contains(functionName)) { + throw new IllegalArgumentException( + "Function '" + + functionName + + "' is not allowed in policy expressions. " + + "Only use approved functions with @Function annotations in evaluator classes."); + } + } + } + + LOG.debug("Validated expression contains only allowed functions: {}", foundFunctions); + } + + public static Set getAllowedFunctions() { + return new HashSet<>(ALLOWED_FUNCTIONS); + } +} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/SubscriptionUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/SubscriptionUtil.java index 734df43cab8..4784b917bd2 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/util/SubscriptionUtil.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/SubscriptionUtil.java @@ -290,7 +290,19 @@ public class SubscriptionUtil { default -> null; }; if (webhookConfig != null && !CommonUtil.nullOrEmpty(webhookConfig.getEndpoint())) { - return Optional.of(webhookConfig.getEndpoint().toString()); + String endpointStr = webhookConfig.getEndpoint().toString(); + // Validate URL before returning + try { + URLValidator.validateURL(endpointStr); + return Optional.of(endpointStr); + } catch (Exception e) { + LOG.warn( + "[GetWebhookUrlsFromProfile] Invalid webhook URL for owner with id {} type {}: {}", + id, + entityType, + e.getMessage()); + return Optional.empty(); + } } else { LOG.debug( "[GetWebhookUrlsFromProfile] Owner with id {} type {}, will not get any Notification as not webhook config is missing for type {}, webhookConfig {} ", @@ -422,6 +434,9 @@ public class SubscriptionUtil { } public static Invocation.Builder appendHeadersToTarget(Client client, String uri) { + // Validate the URI to prevent SSRF attacks + URLValidator.validateURL(uri); + Map authHeaders = SecurityUtil.authHeaders("admin@open-metadata.org"); return SecurityUtil.addHeaders(client.target(uri), authHeaders); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/URLValidator.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/URLValidator.java new file mode 100644 index 00000000000..e14945beb8c --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/URLValidator.java @@ -0,0 +1,69 @@ +/* + * Copyright 2021 Collate + * Licensed 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.openmetadata.service.util; + +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; +import java.util.Arrays; +import java.util.List; +import java.util.regex.Pattern; +import javax.ws.rs.BadRequestException; +import lombok.extern.slf4j.Slf4j; +import org.jetbrains.annotations.NotNull; + +/** + * Utility class for validating URLs to prevent SSRF attacks. + */ +@Slf4j +public class URLValidator { + private static final List ALLOWED_SCHEMES = Arrays.asList("http", "https"); + private static final Pattern PRIVATE_IP_PATTERN = + Pattern.compile( + "^(127\\.|10\\.|172\\.(1[6-9]|2[0-9]|3[0-1])\\.|192\\.168\\.|169\\.254\\.|::1|[fF][cCdD]|[fF][eE][80-9a-fA-F]:).*"); + + public static void validateURL(String urlString) { + if (urlString == null || urlString.trim().isEmpty()) { + throw new BadRequestException("URL cannot be empty"); + } + + String host = getString(urlString); + + if (PRIVATE_IP_PATTERN.matcher(host).matches()) { + throw new BadRequestException("URL targeting private/internal network not allowed"); + } + } + + private static @NotNull String getString(String urlString) { + URL url; + try { + URI uri = new URI(urlString); + url = uri.toURL(); + } catch (URISyntaxException | MalformedURLException e) { + try { + url = new URL(urlString); + } catch (MalformedURLException ex) { + throw new BadRequestException("Invalid URL format: " + ex.getMessage()); + } + } + + String protocol = url.getProtocol().toLowerCase(); + if (!ALLOWED_SCHEMES.contains(protocol)) { + throw new BadRequestException("URL scheme not allowed: " + protocol); + } + + return url.getHost().toLowerCase(); + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/policies/PolicyResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/policies/PolicyResourceTest.java index aaabf4f7d89..8fe73289b42 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/policies/PolicyResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/policies/PolicyResourceTest.java @@ -269,10 +269,6 @@ public class PolicyResourceTest extends EntityResourceTest // isOwner() has Unexpected input parameter failsToEvaluate(policyName, "!isOwner('unexpectedParam')"); - // Invalid function name - failsToEvaluate(policyName, "invalidFunction()"); - failsToEvaluate(policyName, "isOwner() || invalidFunction()"); - // Invalid text failsToEvaluate(policyName, "a"); failsToEvaluate(policyName, "abc"); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/ExpressionValidatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/ExpressionValidatorTest.java new file mode 100644 index 00000000000..bf185ad97c3 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/ExpressionValidatorTest.java @@ -0,0 +1,147 @@ +/* + * Copyright 2021 Collate + * Licensed 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.openmetadata.service.security.policyevaluator; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Set; +import org.junit.jupiter.api.Test; + +public class ExpressionValidatorTest { + + @Test + void testValidExpressions() { + // Test valid expressions + String[] validExpressions = { + "noOwner()", + "!noOwner()", + "isOwner()", + "hasDomain()", + "matchAllTags('PersonalData.Personal', 'Tier.Tier1')", + "matchAnyTag('PersonalData.Personal', 'Tier.Tier1')", + "matchAnyCertification('Certification.Silver', 'Certification.Gold')", + "matchTeam()", + "inAnyTeam('marketing')", + "hasAnyRole('DataSteward', 'DataEngineer')", + "noOwner() && !isOwner()", + "noOwner() || isOwner()", + "!noOwner() && isOwner()", + "matchAllTags('PersonalData.Personal') && hasAnyRole('DataSteward')" + }; + + for (String expression : validExpressions) { + assertDoesNotThrow( + () -> ExpressionValidator.validateExpressionSafety(expression), + "Valid expression '" + expression + "' should not throw an exception"); + } + } + + @Test + void testInvalidExpressions() { + // Test expressions with disallowed patterns + String[] invalidExpressions = { + "T(java.lang.System).exit(0)", + "new java.io.File('/etc/passwd')", + "System.getProperty('user.home')", + "Runtime.getRuntime().exec('ls')", + "this.getClass().getClassLoader()", + "Class.forName('java.lang.Runtime')", + "someObject.getClass()", + "ProcessBuilder('ls').start()", + "eval('System.exit(0)')", + "java.lang.reflect.Method.invoke()" + }; + + for (String expression : invalidExpressions) { + Exception exception = + assertThrows( + IllegalArgumentException.class, + () -> ExpressionValidator.validateExpressionSafety(expression), + "Invalid expression '" + expression + "' should throw an IllegalArgumentException"); + + assertTrue( + exception.getMessage().contains("potentially unsafe pattern") + || exception.getMessage().contains("is not allowed in policy expressions"), + "Exception message should indicate unsafe pattern or disallowed function"); + } + } + + @Test + void testInvalidFunctionNames() { + // Test expressions with function names not in RuleEvaluator + String[] invalidFunctions = { + "deleteAllData()", + "executeQuery('DELETE FROM users')", + "runCommand('rm -rf /')", + "customMethod()" + }; + + for (String expression : invalidFunctions) { + Exception exception = + assertThrows( + IllegalArgumentException.class, + () -> ExpressionValidator.validateExpressionSafety(expression), + "Expression with invalid function '" + + expression + + "' should throw an IllegalArgumentException"); + + assertTrue( + exception.getMessage().contains("is not allowed in policy expressions"), + "Exception should mention that the function is not allowed"); + } + } + + @Test + void testAllowedFunctionsFromRuleEvaluator() { + // Verify that the validator properly extracts all functions from RuleEvaluator + Set allowedFunctions = ExpressionValidator.getAllowedFunctions(); + + // These functions should be present (from RuleEvaluator) + assertTrue(allowedFunctions.contains("noOwner"), "noOwner should be an allowed function"); + assertTrue(allowedFunctions.contains("isOwner"), "isOwner should be an allowed function"); + assertTrue(allowedFunctions.contains("hasDomain"), "hasDomain should be an allowed function"); + assertTrue( + allowedFunctions.contains("matchAllTags"), "matchAllTags should be an allowed function"); + assertTrue( + allowedFunctions.contains("matchAnyTag"), "matchAnyTag should be an allowed function"); + assertTrue( + allowedFunctions.contains("matchAnyCertification"), + "matchAnyCertification should be an allowed function"); + assertTrue(allowedFunctions.contains("matchTeam"), "matchTeam should be an allowed function"); + assertTrue(allowedFunctions.contains("inAnyTeam"), "inAnyTeam should be an allowed function"); + assertTrue(allowedFunctions.contains("hasAnyRole"), "hasAnyRole should be an allowed function"); + + // These functions should not be present + assertFalse(allowedFunctions.contains("deleteAllData"), "deleteAllData should not be allowed"); + assertFalse(allowedFunctions.contains("executeQuery"), "executeQuery should not be allowed"); + assertFalse(allowedFunctions.contains("runCommand"), "runCommand should not be allowed"); + } + + @Test + void testNullAndEmptyExpressions() { + // Test null and empty expressions (should not throw) + assertDoesNotThrow( + () -> ExpressionValidator.validateExpressionSafety(null), + "Null expression should not throw an exception"); + assertDoesNotThrow( + () -> ExpressionValidator.validateExpressionSafety(""), + "Empty expression should not throw an exception"); + assertDoesNotThrow( + () -> ExpressionValidator.validateExpressionSafety(" "), + "Whitespace expression should not throw an exception"); + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java index 7b1953186d5..ef6f6792bc5 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java @@ -1,19 +1,31 @@ package org.openmetadata.service.security.policyevaluator; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.openmetadata.schema.type.MetadataOperation.ALL; import static org.openmetadata.schema.type.MetadataOperation.CREATE; +import static org.openmetadata.schema.type.MetadataOperation.CREATE_INGESTION_PIPELINE_AUTOMATOR; import static org.openmetadata.schema.type.MetadataOperation.DELETE; +import static org.openmetadata.schema.type.MetadataOperation.DELETE_TEST_CASE_FAILED_ROWS_SAMPLE; +import static org.openmetadata.schema.type.MetadataOperation.DEPLOY; import static org.openmetadata.schema.type.MetadataOperation.EDIT_ALL; import static org.openmetadata.schema.type.MetadataOperation.EDIT_CUSTOM_FIELDS; import static org.openmetadata.schema.type.MetadataOperation.EDIT_DISPLAY_NAME; import static org.openmetadata.schema.type.MetadataOperation.EDIT_LINEAGE; +import static org.openmetadata.schema.type.MetadataOperation.GENERATE_TOKEN; +import static org.openmetadata.schema.type.MetadataOperation.KILL; +import static org.openmetadata.schema.type.MetadataOperation.TRIGGER; import static org.openmetadata.schema.type.MetadataOperation.VIEW_ALL; import static org.openmetadata.schema.type.MetadataOperation.VIEW_BASIC; import static org.openmetadata.schema.type.MetadataOperation.VIEW_QUERIES; import static org.openmetadata.schema.type.MetadataOperation.VIEW_USAGE; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import org.openmetadata.schema.type.MetadataOperation; @@ -113,8 +125,20 @@ class PolicyEvaluatorTest { void trimPermissions_withAllowAccess_trimmed() { List permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW); + // trim works only for permissions starting with "EDIT" or "VIEW" List expectedOperations = - Arrays.asList(ALL, DELETE, CREATE, VIEW_ALL, EDIT_ALL); + Arrays.asList( + ALL, + DELETE, + CREATE, + CREATE_INGESTION_PIPELINE_AUTOMATOR, + VIEW_ALL, + EDIT_ALL, + DELETE_TEST_CASE_FAILED_ROWS_SAMPLE, + DEPLOY, + TRIGGER, + KILL, + GENERATE_TOKEN); List actual = PolicyEvaluator.trimPermissions(permissions); assertEqualsPermissions(expectedOperations, actual); } @@ -122,8 +146,20 @@ class PolicyEvaluatorTest { @Test void trimPermissions_withDenyAccess_trimmed() { List permissions = getPermissions(OperationContext.getAllOperations(), Access.DENY); + // trim works only for permissions starting with "EDIT" or "VIEW" List expectedOperations = - Arrays.asList(ALL, DELETE, CREATE, VIEW_ALL, EDIT_ALL); + Arrays.asList( + ALL, + DELETE, + CREATE, + CREATE_INGESTION_PIPELINE_AUTOMATOR, + VIEW_ALL, + EDIT_ALL, + DELETE_TEST_CASE_FAILED_ROWS_SAMPLE, + DEPLOY, + TRIGGER, + KILL, + GENERATE_TOKEN); List actual = PolicyEvaluator.trimPermissions(permissions); assertEqualsPermissions(expectedOperations, actual); } diff --git a/pom.xml b/pom.xml index 6ee39ba14f6..269bc2a4dc9 100644 --- a/pom.xml +++ b/pom.xml @@ -101,7 +101,7 @@ 9.22.3 2.1.0.30 2.11.0 - 9.1.0 + 9.3.0 42.7.4 1.2.1 2.6 @@ -641,7 +641,7 @@ io.netty netty-bom - 4.1.112.Final + 4.1.118.Final pom import