From 813203263b06dda18e2aed68ae92b34277f904b4 Mon Sep 17 00:00:00 2001 From: Othello Maurer Date: Tue, 6 Feb 2024 15:34:55 +0100 Subject: [PATCH] Restrict classes allowed for cluster config and event types (#18165) Add a new safe_classes configuration option to restrict the classes allowed to be used as cluster config and event types. The configuration option allows to specify a comma-separated set of prefixes matched against the fully qualified class name. For now, the default value for the configuration is org.graylog.,org.graylog2., which will allow all classes that Graylog maintains. This should work out of the box for almost all setups. Changing the default value might only be necessary if external plugins require cluster config or event types outside the "org.graylog." or "org.graylog2." namespaces. If that is the case, the configuration setting can be adjusted to cover this use case, e.b. by setting it to safe_classes = org.graylog.,org.graylog2.,custom.plugin.namespace. if said classes are located within the custom.plugin.namespace package. Refs: https://github.com/Graylog2/graylog2-server/security/advisories/GHSA-p6gg-5hf4-4rgj --- changelog/unreleased/pr-18165.toml | 4 + .../org/graylog/datanode/Configuration.java | 11 ++- .../main/java/org/graylog2/Configuration.java | 27 ++++++ .../cluster/ClusterConfigServiceImpl.java | 23 ++++- .../events/ClusterEventPeriodical.java | 33 +++++-- .../system/ClusterConfigResource.java | 11 ++- .../RestrictedChainingClassLoader.java | 60 ++++++++++++ .../org/graylog2/security/SafeClasses.java | 52 ++++++++++ .../UnsafeClassLoadingAttemptException.java | 29 ++++++ .../views/search/views/ViewServiceTest.java | 5 +- .../cluster/ClusterConfigServiceImplTest.java | 22 ++++- .../events/ClusterEventPeriodicalTest.java | 61 +++++++++++- .../indexset/MongoIndexSetServiceTest.java | 6 +- ...5163900_MoveIndexSetDefaultConfigTest.java | 10 +- ...50100_FixAlertConditionsMigrationTest.java | 7 +- .../system/ClusterConfigResourceTest.java | 94 +++++++++++++++++++ 16 files changed, 426 insertions(+), 29 deletions(-) create mode 100644 changelog/unreleased/pr-18165.toml create mode 100644 graylog2-server/src/main/java/org/graylog2/security/RestrictedChainingClassLoader.java create mode 100644 graylog2-server/src/main/java/org/graylog2/security/SafeClasses.java create mode 100644 graylog2-server/src/main/java/org/graylog2/security/UnsafeClassLoadingAttemptException.java create mode 100644 graylog2-server/src/test/java/org/graylog2/rest/resources/system/ClusterConfigResourceTest.java diff --git a/changelog/unreleased/pr-18165.toml b/changelog/unreleased/pr-18165.toml new file mode 100644 index 000000000000..5254c051e097 --- /dev/null +++ b/changelog/unreleased/pr-18165.toml @@ -0,0 +1,4 @@ +type = "security" +message = "Restrict classes allowed for cluster config and event types. [GHSA-p6gg-5hf4-4rgj](https://github.com/Graylog2/graylog2-server/security/advisories/GHSA-p6gg-5hf4-4rgj)" + +pulls = ["18165"] diff --git a/data-node/src/main/java/org/graylog/datanode/Configuration.java b/data-node/src/main/java/org/graylog/datanode/Configuration.java index 2c7601f5b0d9..34926091f724 100644 --- a/data-node/src/main/java/org/graylog/datanode/Configuration.java +++ b/data-node/src/main/java/org/graylog/datanode/Configuration.java @@ -23,6 +23,7 @@ import com.github.joschi.jadconfig.ValidatorMethod; import com.github.joschi.jadconfig.converters.IntegerConverter; import com.github.joschi.jadconfig.converters.StringListConverter; +import com.github.joschi.jadconfig.converters.StringSetConverter; import com.github.joschi.jadconfig.util.Duration; import com.github.joschi.jadconfig.validators.PositiveIntegerValidator; import com.github.joschi.jadconfig.validators.StringNotBlankValidator; @@ -31,6 +32,7 @@ import com.google.common.net.InetAddresses; import org.graylog.datanode.configuration.BaseConfiguration; import org.graylog.datanode.configuration.DatanodeDirectories; +import org.graylog2.Configuration.SafeClassesValidator; import org.graylog2.plugin.Tools; import org.graylog2.shared.SuppressForbidden; import org.joda.time.DateTimeZone; @@ -50,6 +52,7 @@ import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.Set; /** * Helper class to hold configuration of Graylog @@ -213,9 +216,15 @@ public class Configuration extends BaseConfiguration { @Parameter(value = "http_allow_embedding") private boolean httpAllowEmbedding = false; + /** + * Classes considered safe to load by name. A set of prefixes matched against the fully qualified class name. + */ + @Parameter(value = org.graylog2.Configuration.SAFE_CLASSES, converter = StringSetConverter.class, validators = SafeClassesValidator.class) + private Set safeClasses = Set.of("org.graylog.", "org.graylog2."); + @Parameter(value = "metrics_timestamp") private String metricsTimestamp = "timestamp"; - + @Parameter(value = "metrics_stream") private String metricsStream = "gl-datanode-metrics"; diff --git a/graylog2-server/src/main/java/org/graylog2/Configuration.java b/graylog2-server/src/main/java/org/graylog2/Configuration.java index 42a1ba3adb76..428fde6cd409 100644 --- a/graylog2-server/src/main/java/org/graylog2/Configuration.java +++ b/graylog2-server/src/main/java/org/graylog2/Configuration.java @@ -28,6 +28,7 @@ import com.github.joschi.jadconfig.validators.PositiveLongValidator; import com.github.joschi.jadconfig.validators.StringNotBlankValidator; import com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; import org.graylog.plugins.views.search.engine.suggestions.FieldValueSuggestionMode; import org.graylog.plugins.views.search.engine.suggestions.FieldValueSuggestionModeConverter; import org.graylog.security.certutil.CaConfiguration; @@ -50,11 +51,14 @@ import java.util.Collections; import java.util.Set; +import static org.graylog2.shared.utilities.StringUtils.f; + /** * Helper class to hold configuration of Graylog */ @SuppressWarnings("FieldMayBeFinal") public class Configuration extends CaConfiguration { + public static final String SAFE_CLASSES = "safe_classes"; /** * Deprecated! Use isLeader() instead. @@ -231,6 +235,12 @@ public class Configuration extends CaConfiguration { @Parameter(value = "minimum_auto_refresh_interval", required = true) private Period minimumAutoRefreshInterval = Period.seconds(1); + /** + * Classes considered safe to load by name. A set of prefixes matched against the fully qualified class name. + */ + @Parameter(value = SAFE_CLASSES, converter = StringSetConverter.class, validators = SafeClassesValidator.class) + private Set safeClasses = Set.of("org.graylog.", "org.graylog2."); + @Parameter(value = "field_value_suggestion_mode", required = true, converter = FieldValueSuggestionModeConverter.class) private FieldValueSuggestionMode fieldValueSuggestionMode = FieldValueSuggestionMode.ON; @@ -457,6 +467,10 @@ public Period getMinimumAutoRefreshInterval() { return minimumAutoRefreshInterval; } + public Set getSafeClasses() { + return safeClasses; + } + public FieldValueSuggestionMode getFieldValueSuggestionMode() { return fieldValueSuggestionMode; } @@ -573,6 +587,19 @@ public void validate(String name, String path) throws ValidationException { } } + public static class SafeClassesValidator implements Validator> { + @Override + public void validate(String name, Set set) throws ValidationException { + if (set.isEmpty()) { + throw new ValidationException(f("\"%s\" must not be empty. Please specify a comma-separated list of " + + "fully-qualified class name prefixes.", name)); + } + if (set.stream().anyMatch(StringUtils::isBlank)) { + throw new ValidationException(f("\"%s\" must only contain non-empty class name prefixes.", name)); + } + } + } + /** * Calculate the default number of output buffer processors as a linear function of available CPU cores. * The function is designed to yield predetermined values for the following select numbers of CPU cores that diff --git a/graylog2-server/src/main/java/org/graylog2/cluster/ClusterConfigServiceImpl.java b/graylog2-server/src/main/java/org/graylog2/cluster/ClusterConfigServiceImpl.java index 74c7d1fe935a..8734560da248 100644 --- a/graylog2-server/src/main/java/org/graylog2/cluster/ClusterConfigServiceImpl.java +++ b/graylog2-server/src/main/java/org/graylog2/cluster/ClusterConfigServiceImpl.java @@ -27,6 +27,9 @@ import org.graylog2.events.ClusterEventBus; import org.graylog2.plugin.cluster.ClusterConfigService; import org.graylog2.plugin.system.NodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; +import org.graylog2.security.UnsafeClassLoadingAttemptException; import org.graylog2.shared.plugins.ChainingClassLoader; import org.graylog2.shared.utilities.AutoValueUtils; import org.joda.time.DateTime; @@ -53,23 +56,33 @@ public class ClusterConfigServiceImpl implements ClusterConfigService { private final JacksonDBCollection dbCollection; private final NodeId nodeId; private final ObjectMapper objectMapper; - private final ChainingClassLoader chainingClassLoader; + private final RestrictedChainingClassLoader chainingClassLoader; private final EventBus clusterEventBus; @Inject public ClusterConfigServiceImpl(final MongoJackObjectMapperProvider mapperProvider, final MongoConnection mongoConnection, final NodeId nodeId, - final ChainingClassLoader chainingClassLoader, + final RestrictedChainingClassLoader chainingClassLoader, final ClusterEventBus clusterEventBus) { this(JacksonDBCollection.wrap(prepareCollection(mongoConnection), ClusterConfig.class, String.class, mapperProvider.get()), nodeId, mapperProvider.get(), chainingClassLoader, clusterEventBus); } + @Deprecated + public ClusterConfigServiceImpl(final MongoJackObjectMapperProvider mapperProvider, + final MongoConnection mongoConnection, + final NodeId nodeId, + final ChainingClassLoader chainingClassLoader, + final ClusterEventBus clusterEventBus) { + this(JacksonDBCollection.wrap(prepareCollection(mongoConnection), ClusterConfig.class, String.class, mapperProvider.get()), + nodeId, mapperProvider.get(), new RestrictedChainingClassLoader(chainingClassLoader, SafeClasses.allGraylogInternal()), clusterEventBus); + } + private ClusterConfigServiceImpl(final JacksonDBCollection dbCollection, final NodeId nodeId, final ObjectMapper objectMapper, - final ChainingClassLoader chainingClassLoader, + final RestrictedChainingClassLoader chainingClassLoader, final EventBus clusterEventBus) { this.nodeId = checkNotNull(nodeId); this.dbCollection = checkNotNull(dbCollection); @@ -175,10 +188,12 @@ public Set> list() { for (ClusterConfig clusterConfig : clusterConfigs) { final String type = clusterConfig.type(); try { - final Class cls = chainingClassLoader.loadClass(type); + final Class cls = chainingClassLoader.loadClassSafely(type); classes.add(cls); } catch (ClassNotFoundException e) { LOG.debug("Couldn't find configuration class \"{}\"", type, e); + } catch (UnsafeClassLoadingAttemptException e) { + LOG.warn("Couldn't load class <{}>.", type, e); } } } diff --git a/graylog2-server/src/main/java/org/graylog2/events/ClusterEventPeriodical.java b/graylog2-server/src/main/java/org/graylog2/events/ClusterEventPeriodical.java index 06bd5f8318c8..6dbc39fe4c81 100644 --- a/graylog2-server/src/main/java/org/graylog2/events/ClusterEventPeriodical.java +++ b/graylog2-server/src/main/java/org/graylog2/events/ClusterEventPeriodical.java @@ -28,10 +28,14 @@ import com.mongodb.DBObject; import com.mongodb.MongoException; import com.mongodb.WriteConcern; +import jakarta.inject.Inject; import org.graylog2.bindings.providers.MongoJackObjectMapperProvider; import org.graylog2.database.MongoConnection; import org.graylog2.plugin.periodical.Periodical; import org.graylog2.plugin.system.NodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; +import org.graylog2.security.UnsafeClassLoadingAttemptException; import org.graylog2.shared.plugins.ChainingClassLoader; import org.graylog2.shared.utilities.AutoValueUtils; import org.mongojack.DBCursor; @@ -41,8 +45,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import jakarta.inject.Inject; - import java.util.Collections; import static com.google.common.base.Preconditions.checkNotNull; @@ -57,23 +59,36 @@ public class ClusterEventPeriodical extends Periodical { private final NodeId nodeId; private final ObjectMapper objectMapper; private final EventBus serverEventBus; - private final ChainingClassLoader chainingClassLoader; + private final RestrictedChainingClassLoader chainingClassLoader; @Inject public ClusterEventPeriodical(final MongoJackObjectMapperProvider mapperProvider, final MongoConnection mongoConnection, final NodeId nodeId, - final ChainingClassLoader chainingClassLoader, + final RestrictedChainingClassLoader chainingClassLoader, final EventBus serverEventBus, final ClusterEventBus clusterEventBus) { this(JacksonDBCollection.wrap(prepareCollection(mongoConnection), ClusterEvent.class, String.class, mapperProvider.get()), nodeId, mapperProvider.get(), chainingClassLoader, serverEventBus, clusterEventBus); } + @Deprecated + public ClusterEventPeriodical(final MongoJackObjectMapperProvider mapperProvider, + final MongoConnection mongoConnection, + final NodeId nodeId, + final ChainingClassLoader chainingClassLoader, + final EventBus serverEventBus, + final ClusterEventBus clusterEventBus) { + this(JacksonDBCollection.wrap(prepareCollection(mongoConnection), ClusterEvent.class, String.class, + mapperProvider.get()), nodeId, mapperProvider.get(), + new RestrictedChainingClassLoader(chainingClassLoader, SafeClasses.allGraylogInternal()), + serverEventBus, clusterEventBus); + } + private ClusterEventPeriodical(final JacksonDBCollection dbCollection, final NodeId nodeId, final ObjectMapper objectMapper, - final ChainingClassLoader chainingClassLoader, + final RestrictedChainingClassLoader chainingClassLoader, final EventBus serverEventBus, final ClusterEventBus clusterEventBus) { this.nodeId = checkNotNull(nodeId); @@ -205,15 +220,15 @@ private void updateConsumers(final String eventId, final NodeId nodeId) { private Object extractPayload(Object payload, String eventClass) { try { - final Class clazz = chainingClassLoader.loadClass(eventClass); + final Class clazz = chainingClassLoader.loadClassSafely(eventClass); return objectMapper.convertValue(payload, clazz); } catch (ClassNotFoundException e) { LOG.debug("Couldn't load class <" + eventClass + "> for event", e); - return null; } catch (IllegalArgumentException e) { LOG.debug("Error while deserializing payload", e); - return null; - + } catch (UnsafeClassLoadingAttemptException e) { + LOG.warn("Couldn't load class <{}>.", eventClass, e); } + return null; } } diff --git a/graylog2-server/src/main/java/org/graylog2/rest/resources/system/ClusterConfigResource.java b/graylog2-server/src/main/java/org/graylog2/rest/resources/system/ClusterConfigResource.java index 69951910568b..a3107cd111cb 100644 --- a/graylog2-server/src/main/java/org/graylog2/rest/resources/system/ClusterConfigResource.java +++ b/graylog2-server/src/main/java/org/graylog2/rest/resources/system/ClusterConfigResource.java @@ -47,7 +47,8 @@ import org.graylog2.plugin.validate.ConfigValidationException; import org.graylog2.rest.MoreMediaTypes; import org.graylog2.rest.models.system.config.ClusterConfigList; -import org.graylog2.shared.plugins.ChainingClassLoader; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.UnsafeClassLoadingAttemptException; import org.graylog2.shared.rest.resources.RestResource; import org.graylog2.shared.security.RestPermissions; import org.slf4j.Logger; @@ -73,13 +74,13 @@ public class ClusterConfigResource extends RestResource { public static final String NO_CLASS_MSG = "Couldn't find configuration class '%s'"; private final ClusterConfigService clusterConfigService; - private final ChainingClassLoader chainingClassLoader; + private final RestrictedChainingClassLoader chainingClassLoader; private final ObjectMapper objectMapper; private final ClusterConfigValidatorService clusterConfigValidatorService; @Inject public ClusterConfigResource(ClusterConfigService clusterConfigService, - ChainingClassLoader chainingClassLoader, + RestrictedChainingClassLoader chainingClassLoader, ObjectMapper objectMapper, ClusterConfigValidatorService clusterConfigValidatorService) { this.clusterConfigService = requireNonNull(clusterConfigService); @@ -209,9 +210,11 @@ public JsonSchema schema(@ApiParam(name = "configClass", value = "The name of th @Nullable private Class classFromName(String className) { try { - return chainingClassLoader.loadClass(className); + return chainingClassLoader.loadClassSafely(className); } catch (ClassNotFoundException e) { return null; + } catch (UnsafeClassLoadingAttemptException e) { + throw new BadRequestException(e.getLocalizedMessage()); } } diff --git a/graylog2-server/src/main/java/org/graylog2/security/RestrictedChainingClassLoader.java b/graylog2-server/src/main/java/org/graylog2/security/RestrictedChainingClassLoader.java new file mode 100644 index 000000000000..752463ce4f08 --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog2/security/RestrictedChainingClassLoader.java @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.security; + +import jakarta.inject.Inject; +import org.graylog2.Configuration; +import org.graylog2.shared.plugins.ChainingClassLoader; + +import static org.graylog2.shared.utilities.StringUtils.f; + +/** + * A wrapper around the chaining class loader intended only for loading classes safely by considering an allow-list of + * class name prefixes. + */ +public class RestrictedChainingClassLoader { + private final ChainingClassLoader delegate; + private final SafeClasses safeClasses; + + @Inject + public RestrictedChainingClassLoader(ChainingClassLoader delegate, SafeClasses safeClasses) { + this.delegate = delegate; + this.safeClasses = safeClasses; + } + + /** + * Load the class only if the name passes the check of {@link SafeClasses#isSafeToLoad(String)}. If the class name + * passes the check, the call is delegated to {@link ChainingClassLoader#loadClass(String)}. If it doesn't pass the + * check, an {@link UnsafeClassLoadingAttemptException} is thrown. + * + * @return class as returned by the delegated call to {@link ChainingClassLoader#loadClass(String)} + * @throws ClassNotFoundException if the class was not found + * @throws UnsafeClassLoadingAttemptException if the class name didn't pass the safety check of + * {@link SafeClasses#isSafeToLoad(String)} + */ + public Class loadClassSafely(String name) throws ClassNotFoundException, UnsafeClassLoadingAttemptException { + if (safeClasses.isSafeToLoad(name)) { + return delegate.loadClass(name); + } else { + throw new UnsafeClassLoadingAttemptException( + f("Prevented loading of unsafe class \"%s\". Consider adjusting the configuration setting " + + "\"%s\", if you think that this is a mistake.", name, Configuration.SAFE_CLASSES) + ); + } + } + +} diff --git a/graylog2-server/src/main/java/org/graylog2/security/SafeClasses.java b/graylog2-server/src/main/java/org/graylog2/security/SafeClasses.java new file mode 100644 index 000000000000..bc1fd3148f8b --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog2/security/SafeClasses.java @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.security; + +import jakarta.inject.Inject; +import jakarta.inject.Named; +import jakarta.inject.Singleton; +import org.graylog2.Configuration; + +import javax.annotation.Nonnull; +import java.util.Objects; +import java.util.Set; + +/** + * Adds a safety net for class loading. + */ +@Singleton +public class SafeClasses { + private final Set prefixes; + + public static SafeClasses allGraylogInternal() { + return new SafeClasses(Set.of("org.graylog.", "org.graylog2.")); + } + + @Inject + public SafeClasses(@Named(Configuration.SAFE_CLASSES) @Nonnull Set prefixes) { + this.prefixes = Objects.requireNonNull(prefixes); + } + + /** + * Check if the class name is considered safe for loading by names from a potentially user-provided input. + * Classes are considered safe if their fully qualified class name starts with any of the prefixes configured in + * {@link Configuration#getSafeClasses()}. + */ + public boolean isSafeToLoad(String className) { + return prefixes.stream().anyMatch(className::startsWith); + } +} diff --git a/graylog2-server/src/main/java/org/graylog2/security/UnsafeClassLoadingAttemptException.java b/graylog2-server/src/main/java/org/graylog2/security/UnsafeClassLoadingAttemptException.java new file mode 100644 index 000000000000..0d73c707fd9a --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog2/security/UnsafeClassLoadingAttemptException.java @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.security; + +import org.graylog2.Configuration; + +/** + * Exception indicating an attempt to load a class that is not considered safe because it's fully qualified class name + * did not start with any of the prefixes configured in {@link Configuration#getSafeClasses()} + */ +public class UnsafeClassLoadingAttemptException extends Exception { + public UnsafeClassLoadingAttemptException(String message) { + super(message); + } +} diff --git a/graylog2-server/src/test/java/org/graylog/plugins/views/search/views/ViewServiceTest.java b/graylog2-server/src/test/java/org/graylog/plugins/views/search/views/ViewServiceTest.java index c1112df2d950..2d4776397f37 100644 --- a/graylog2-server/src/test/java/org/graylog/plugins/views/search/views/ViewServiceTest.java +++ b/graylog2-server/src/test/java/org/graylog/plugins/views/search/views/ViewServiceTest.java @@ -31,6 +31,8 @@ import org.graylog2.plugin.system.SimpleNodeId; import org.graylog2.search.SearchQueryField; import org.graylog2.search.SearchQueryParser; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; import org.graylog2.shared.bindings.providers.ObjectMapperProvider; import org.graylog2.shared.plugins.ChainingClassLoader; import org.junit.After; @@ -64,7 +66,8 @@ public void setUp() throws Exception { objectMapperProvider, mongodb.mongoConnection(), new SimpleNodeId("5ca1ab1e-0000-4000-a000-000000000000"), - new ChainingClassLoader(getClass().getClassLoader()), + new RestrictedChainingClassLoader( + new ChainingClassLoader(getClass().getClassLoader()), SafeClasses.allGraylogInternal()), new ClusterEventBus() ); this.dbService = new ViewService( diff --git a/graylog2-server/src/test/java/org/graylog2/cluster/ClusterConfigServiceImplTest.java b/graylog2-server/src/test/java/org/graylog2/cluster/ClusterConfigServiceImplTest.java index 0f1176d5406b..b89cbac91b22 100644 --- a/graylog2-server/src/test/java/org/graylog2/cluster/ClusterConfigServiceImplTest.java +++ b/graylog2-server/src/test/java/org/graylog2/cluster/ClusterConfigServiceImplTest.java @@ -30,6 +30,8 @@ import org.graylog2.plugin.cluster.ClusterConfigService; import org.graylog2.plugin.system.NodeId; import org.graylog2.plugin.system.SimpleNodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; import org.graylog2.shared.bindings.providers.ObjectMapperProvider; import org.graylog2.shared.plugins.ChainingClassLoader; import org.joda.time.DateTime; @@ -78,7 +80,8 @@ public void setUpService() throws Exception { provider, mongodb.mongoConnection(), nodeId, - new ChainingClassLoader(getClass().getClassLoader()), + new RestrictedChainingClassLoader(new ChainingClassLoader(getClass().getClassLoader()), + SafeClasses.allGraylogInternal()), clusterEventBus ); } @@ -401,7 +404,7 @@ public void listIgnoresInvalidClasses() throws Exception { .add("last_updated_by", "ID") .get()); collection.save(new BasicDBObjectBuilder() - .add("type", "invalid.ClassName") + .add("type", "org.graylog.invalid.ClassName") .add("payload", Collections.emptyMap()) .add("last_updated", TIME.toString()) .add("last_updated_by", "ID") @@ -413,6 +416,21 @@ public void listIgnoresInvalidClasses() throws Exception { .containsOnly(CustomConfig.class); } + @Test + public void listIgnoresUnsafeClasses() throws Exception { + @SuppressWarnings("deprecation") + final DBCollection collection = mongoConnection.getDatabase().getCollection(COLLECTION_NAME); + collection.save(new BasicDBObjectBuilder() + .add("type", "java.io.File") + .add("payload", "/etc/passwd") + .add("last_updated", TIME.toString()) + .add("last_updated_by", "ID") + .get()); + + assertThat(collection.count()).isOne(); + assertThat(clusterConfigService.list()).hasSize(0); + } + public static class ClusterConfigChangedEventHandler { public volatile ClusterConfigChangedEvent event; diff --git a/graylog2-server/src/test/java/org/graylog2/events/ClusterEventPeriodicalTest.java b/graylog2-server/src/test/java/org/graylog2/events/ClusterEventPeriodicalTest.java index 31705740479e..b84f42dee6bf 100644 --- a/graylog2-server/src/test/java/org/graylog2/events/ClusterEventPeriodicalTest.java +++ b/graylog2-server/src/test/java/org/graylog2/events/ClusterEventPeriodicalTest.java @@ -32,6 +32,8 @@ import org.graylog2.database.MongoConnection; import org.graylog2.plugin.system.NodeId; import org.graylog2.plugin.system.SimpleNodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; import org.graylog2.shared.bindings.providers.ObjectMapperProvider; import org.graylog2.shared.plugins.ChainingClassLoader; import org.graylog2.system.debug.DebugEvent; @@ -49,6 +51,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; @@ -87,7 +90,9 @@ public void setUpService() throws Exception { provider, mongodb.mongoConnection(), nodeId, - new ChainingClassLoader(getClass().getClassLoader()), + new RestrictedChainingClassLoader(new ChainingClassLoader(getClass().getClassLoader()), + new SafeClasses(Set.of( + SimpleEvent.class.getName(), DebugEvent.class.getName(), Safe.class.getName()))), serverEventBus, clusterEventBus ); @@ -311,7 +316,7 @@ public void localNodeIsMarkedAsHavingConsumedEvent() { DBObject dbObject = collection.findOne(); - assertThat(((BasicDBList)dbObject.get("consumers")).toArray()).isEqualTo(new String[] { nodeId.getNodeId() }); + assertThat(((BasicDBList) dbObject.get("consumers")).toArray()).isEqualTo(new String[]{nodeId.getNodeId()}); } @Test @@ -342,6 +347,58 @@ public void localEventIsNotProcessedFromDB() { verify(clusterEventBus, never()).post(any()); } + private static volatile String constructorArgument; + + public static class Unsafe { + public Unsafe(String param) { + constructorArgument = param; + } + } + + public static class Safe { + public Safe(String param) { + constructorArgument = param; + } + } + + @Test + public void testInstantiatesSafeEventClass() { + DBObject event = new BasicDBObjectBuilder() + .add("timestamp", TIME.getMillis()) + .add("producer", "TEST-PRODUCER") + .add("consumers", Collections.emptyList()) + .add("event_class", "org.graylog2.events.ClusterEventPeriodicalTest$Safe") + .add("payload", "this-is-safe") + .get(); + + @SuppressWarnings("deprecation") + final DBCollection collection = mongoConnection.getDatabase().getCollection(ClusterEventPeriodical.COLLECTION_NAME); + collection.save(event); + + constructorArgument = null; + clusterEventPeriodical.run(); + assertThat(constructorArgument).isEqualTo("this-is-safe"); + } + + @Test + public void testIgnoresUnsafeEventClass() { + DBObject event = new BasicDBObjectBuilder() + .add("timestamp", TIME.getMillis()) + .add("producer", "TEST-PRODUCER") + .add("consumers", Collections.emptyList()) + .add("event_class", "org.graylog2.events.ClusterEventPeriodicalTest$Unsafe") + .add("payload", "this-is-unsafe") + .get(); + + @SuppressWarnings("deprecation") + final DBCollection collection = mongoConnection.getDatabase().getCollection(ClusterEventPeriodical.COLLECTION_NAME); + collection.save(event); + + constructorArgument = null; + clusterEventPeriodical.run(); + assertThat(constructorArgument).isNull(); + } + public static class SimpleEventHandler { final AtomicInteger invocations = new AtomicInteger(); diff --git a/graylog2-server/src/test/java/org/graylog2/indexer/indexset/MongoIndexSetServiceTest.java b/graylog2-server/src/test/java/org/graylog2/indexer/indexset/MongoIndexSetServiceTest.java index 3894dbe0e899..9ed939b01183 100644 --- a/graylog2-server/src/test/java/org/graylog2/indexer/indexset/MongoIndexSetServiceTest.java +++ b/graylog2-server/src/test/java/org/graylog2/indexer/indexset/MongoIndexSetServiceTest.java @@ -34,6 +34,8 @@ import org.graylog2.plugin.cluster.ClusterConfigService; import org.graylog2.plugin.system.NodeId; import org.graylog2.plugin.system.SimpleNodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; import org.graylog2.shared.bindings.providers.ObjectMapperProvider; import org.graylog2.shared.plugins.ChainingClassLoader; import org.graylog2.streams.StreamService; @@ -78,7 +80,9 @@ public class MongoIndexSetServiceTest { public void setUp() throws Exception { clusterEventBus = new ClusterEventBus(); clusterConfigService = new ClusterConfigServiceImpl(objectMapperProvider, mongodb.mongoConnection(), - nodeId, new ChainingClassLoader(getClass().getClassLoader()), clusterEventBus); + nodeId, new RestrictedChainingClassLoader( + new ChainingClassLoader(getClass().getClassLoader()), SafeClasses.allGraylogInternal()), + clusterEventBus); indexSetService = new MongoIndexSetService(mongodb.mongoConnection(), objectMapperProvider, streamService, clusterConfigService, clusterEventBus); } diff --git a/graylog2-server/src/test/java/org/graylog2/migrations/V20161215163900_MoveIndexSetDefaultConfigTest.java b/graylog2-server/src/test/java/org/graylog2/migrations/V20161215163900_MoveIndexSetDefaultConfigTest.java index 79c1fc15ae2f..6a066d3b5630 100644 --- a/graylog2-server/src/test/java/org/graylog2/migrations/V20161215163900_MoveIndexSetDefaultConfigTest.java +++ b/graylog2-server/src/test/java/org/graylog2/migrations/V20161215163900_MoveIndexSetDefaultConfigTest.java @@ -29,13 +29,14 @@ import org.graylog2.migrations.V20161215163900_MoveIndexSetDefaultConfig.MigrationCompleted; import org.graylog2.plugin.system.NodeId; import org.graylog2.plugin.system.SimpleNodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; import org.graylog2.shared.bindings.providers.ObjectMapperProvider; import org.graylog2.shared.plugins.ChainingClassLoader; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; @@ -70,8 +71,11 @@ public class V20161215163900_MoveIndexSetDefaultConfigTest { @Before public void setUp() throws Exception { this.clusterConfigService = spy(new ClusterConfigServiceImpl(objectMapperProvider, - mongodb.mongoConnection(), nodeId, - new ChainingClassLoader(getClass().getClassLoader()), new ClusterEventBus())); + mongodb.mongoConnection(), + nodeId, + new RestrictedChainingClassLoader( + new ChainingClassLoader(getClass().getClassLoader()), SafeClasses.allGraylogInternal()), + new ClusterEventBus())); this.collection = mongodb.mongoConnection().getMongoDatabase().getCollection("index_sets"); diff --git a/graylog2-server/src/test/java/org/graylog2/migrations/V20170110150100_FixAlertConditionsMigrationTest.java b/graylog2-server/src/test/java/org/graylog2/migrations/V20170110150100_FixAlertConditionsMigrationTest.java index 661cec23f001..ab2f44af5a36 100644 --- a/graylog2-server/src/test/java/org/graylog2/migrations/V20170110150100_FixAlertConditionsMigrationTest.java +++ b/graylog2-server/src/test/java/org/graylog2/migrations/V20170110150100_FixAlertConditionsMigrationTest.java @@ -32,12 +32,13 @@ import org.graylog2.migrations.V20170110150100_FixAlertConditionsMigration.MigrationCompleted; import org.graylog2.plugin.system.NodeId; import org.graylog2.plugin.system.SimpleNodeId; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; import org.graylog2.shared.bindings.providers.ObjectMapperProvider; import org.graylog2.shared.plugins.ChainingClassLoader; import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; @@ -74,7 +75,9 @@ public class V20170110150100_FixAlertConditionsMigrationTest { public void setUp() throws Exception { this.clusterConfigService = spy(new ClusterConfigServiceImpl(objectMapperProvider, mongodb.mongoConnection(), nodeId, - new ChainingClassLoader(getClass().getClassLoader()), new ClusterEventBus())); + new RestrictedChainingClassLoader( + new ChainingClassLoader(getClass().getClassLoader()), SafeClasses.allGraylogInternal()), + new ClusterEventBus())); final MongoConnection mongoConnection = spy(mongodb.mongoConnection()); final MongoDatabase mongoDatabase = spy(mongoConnection.getMongoDatabase()); diff --git a/graylog2-server/src/test/java/org/graylog2/rest/resources/system/ClusterConfigResourceTest.java b/graylog2-server/src/test/java/org/graylog2/rest/resources/system/ClusterConfigResourceTest.java new file mode 100644 index 000000000000..af01de619b46 --- /dev/null +++ b/graylog2-server/src/test/java/org/graylog2/rest/resources/system/ClusterConfigResourceTest.java @@ -0,0 +1,94 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.rest.resources.system; + +import jakarta.ws.rs.BadRequestException; +import org.glassfish.jersey.message.internal.FileProvider; +import org.graylog2.plugin.cluster.ClusterConfigService; +import org.graylog2.plugin.validate.ClusterConfigValidatorService; +import org.graylog2.security.RestrictedChainingClassLoader; +import org.graylog2.security.SafeClasses; +import org.graylog2.shared.bindings.providers.ObjectMapperProvider; +import org.graylog2.shared.plugins.ChainingClassLoader; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.graylog2.shared.utilities.StringUtils.f; + +@ExtendWith(MockitoExtension.class) +class ClusterConfigResourceTest { + @Mock + private ClusterConfigService clusterConfigService; + + @Mock + private ClusterConfigValidatorService clusterConfigValidatorService; + + @Test + void putClassConsideredUnsafe(@TempDir Path tmpDir) throws IOException { + final Path file = tmpDir.resolve("secrets.txt"); + Files.writeString(file, "secret content"); + + final ClusterConfigResource resource = new ClusterConfigResource(clusterConfigService, + new RestrictedChainingClassLoader(new ChainingClassLoader(this.getClass().getClassLoader()), + SafeClasses.allGraylogInternal()), + new ObjectMapperProvider().get(), + clusterConfigValidatorService + ); + + assertThatThrownBy(() -> resource.update("java.io.File", + new ByteArrayInputStream(f("\"%s\"", file.toAbsolutePath()).getBytes(StandardCharsets.UTF_8)))) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("Prevented loading of unsafe class"); + } + + /** + * Proof of concept to show what would happen if we'd allow problematic classes + */ + @Test + void putClassConsideredSafe(@TempDir Path tmpDir) throws IOException { + final Path file = tmpDir.resolve("secrets.txt"); + Files.writeString(file, "secret content"); + + final ClusterConfigResource resource = new ClusterConfigResource(clusterConfigService, + new RestrictedChainingClassLoader(new ChainingClassLoader(this.getClass().getClassLoader()), + new SafeClasses(Set.of("java.io.File"))), + new ObjectMapperProvider().get(), + clusterConfigValidatorService); + + // Simulate how jersey would serialize a File object + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + new FileProvider().writeTo((File) resource.update("java.io.File", + new ByteArrayInputStream(f("\"%s\"", file.toAbsolutePath()).getBytes(StandardCharsets.UTF_8))).getEntity(), null, null, null, null, null, out); + final String content = out.toString(StandardCharsets.UTF_8); + + assertThat(content).isEqualTo("secret content"); + } +}