This is an automated email from the ASF dual-hosted git repository. tballison pushed a commit to branch 3x-improve-grpc in repository https://gitbox.apache.org/repos/asf/tika.git
commit 57d8a7ef4c26fa4939443354f6f1a493ac0897b3 Author: tallison <[email protected]> AuthorDate: Wed Jul 8 08:30:46 2026 -0400 improve grpc --- CHANGES.txt | 8 + tika-grpc/README.md | 25 +++ .../tika/pipes/grpc/ExpiringFetcherStore.java | 20 +++ .../org/apache/tika/pipes/grpc/TikaGrpcConfig.java | 89 +++++++++++ .../org/apache/tika/pipes/grpc/TikaGrpcServer.java | 7 + .../apache/tika/pipes/grpc/TikaGrpcServerImpl.java | 131 +++++++++++++++- .../tika/pipes/grpc/TikaGrpcCapabilityTest.java | 169 +++++++++++++++++++++ .../src/test/resources/tika-pipes-test-config.xml | 6 + .../serialization/ParseContextDeserializer.java | 4 + .../tika/serialization/TikaJsonDeserializer.java | 36 +++++ .../TestParseContextComponentInjection.java | 164 ++++++++++++++++++++ 11 files changed, 656 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 17c5276bab..0842c32898 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,13 @@ Release 3.3.2 - (unreleased) + * A request-supplied ParseContext (tika-server /pipes and /async) may no longer + instantiate a Parser, Detector or EncodingDetector, closing a command-injection/SSRF + surface (TIKA-4763). + + * tika-grpc denies per-request config and runtime fetcher modifications by default; opt + in via allowPerRequestConfig/allowComponentModifications in the new <grpc> config + section. Fetchers declared in <fetchers> are now loaded at startup (TIKA-4764). + * tika-server now requires enableUnsecureFeatures=true when the /pipes, /async, or /status endpoints are selected; the server refuses to start otherwise. Previously listing the endpoint was treated as sufficient consent (TIKA-4760). diff --git a/tika-grpc/README.md b/tika-grpc/README.md index f2269b22bf..e376517744 100644 --- a/tika-grpc/README.md +++ b/tika-grpc/README.md @@ -11,3 +11,28 @@ This server will manage a pool of Tika Pipes clients. * Delete * Fetch + Parse a given Fetch Item +## Security + +tika-grpc binds to all interfaces with no application-level authentication, so restrict it to a +trusted network and/or enable TLS (`--secure`, with `--cert-chain`/`--private-key`/ +`--trust-cert-collection`/`--client-auth-required`). A startup warning is logged when running +without TLS. + +Dangerous capabilities are **denied by default** and must be opted into in the `<grpc>` section of +the tika-config: + +```xml +<properties> + <grpc> + <!-- allow SaveFetcher/DeleteFetcher (runtime Create/Update/Delete) --> + <allowComponentModifications>true</allowComponentModifications> + <!-- allow per-request config (additional_fetch_config_json) --> + <allowPerRequestConfig>true</allowPerRequestConfig> + </grpc> +</properties> +``` + +When `allowComponentModifications` is false, fetchers must be declared in the `<fetchers>` section +of the tika-config; these are loaded at startup. Read operations (GetFetcher/ListFetchers, +Fetch+Parse) are always available. + diff --git a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/ExpiringFetcherStore.java b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/ExpiringFetcherStore.java index d21f11b08f..f6222cd155 100644 --- a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/ExpiringFetcherStore.java +++ b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/ExpiringFetcherStore.java @@ -38,6 +38,9 @@ public class ExpiringFetcherStore implements AutoCloseable { private final Map<String, AbstractFetcher> fetchers = Collections.synchronizedMap(new HashMap<>()); private final Map<String, AbstractConfig> fetcherConfigs = Collections.synchronizedMap(new HashMap<>()); private final Map<String, Instant> fetcherLastAccessed = Collections.synchronizedMap(new HashMap<>()); + //fetchers declared in the tika-config <fetchers> section are loaded once at startup and must + //never be expired (with allowComponentModifications=false they cannot be re-registered). + private final Set<String> permanentFetchers = Collections.synchronizedSet(new HashSet<>()); private final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); @@ -45,6 +48,10 @@ public class ExpiringFetcherStore implements AutoCloseable { executorService.scheduleAtFixedRate(() -> { Set<String> expired = new HashSet<>(); for (String fetcherName : fetchers.keySet()) { + if (permanentFetchers.contains(fetcherName)) { + //config-declared fetchers never expire + continue; + } Instant lastAccessed = fetcherLastAccessed.get(fetcherName); if (lastAccessed == null) { LOG.error("Detected a fetcher with no last access time. FetcherName={}", fetcherName); @@ -68,6 +75,7 @@ public class ExpiringFetcherStore implements AutoCloseable { boolean success = fetchers.remove(fetcherName) != null; fetcherConfigs.remove(fetcherName); fetcherLastAccessed.remove(fetcherName); + permanentFetchers.remove(fetcherName); return success; } @@ -89,8 +97,20 @@ public class ExpiringFetcherStore implements AutoCloseable { } public <T extends AbstractFetcher, C extends AbstractConfig> void createFetcher(T fetcher, C config) { + createFetcher(fetcher, config, false); + } + + /** + * @param permanent if true, this fetcher is never expired by the stale-fetcher job. Used for + * fetchers declared in the tika-config {@code <fetchers>} section, which are + * loaded once at startup. + */ + public <T extends AbstractFetcher, C extends AbstractConfig> void createFetcher(T fetcher, C config, boolean permanent) { fetchers.put(fetcher.getName(), fetcher); fetcherConfigs.put(fetcher.getName(), config); + if (permanent) { + permanentFetchers.add(fetcher.getName()); + } getFetcherAndLogAccess(fetcher.getName()); } diff --git a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcConfig.java b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcConfig.java new file mode 100644 index 0000000000..dfc42e6dea --- /dev/null +++ b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcConfig.java @@ -0,0 +1,89 @@ +/* + * 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.tika.pipes.grpc; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.tika.config.ConfigBase; +import org.apache.tika.exception.TikaConfigException; + +/** + * grpc-specific configuration, read from the {@code <grpc>} section of the tika-config xml (the + * same {@link ConfigBase} mechanism {@code TikaServerConfig} uses for its {@code <server>} + * section). + * <p> + * tika-grpc binds to all interfaces with no application-level authentication, so both capabilities + * below are <em>denied by default</em>. An operator must explicitly opt in (after restricting + * network access and/or enabling TLS - see the {@code --secure} family of options on + * {@link TikaGrpcServer}). This mirrors the 4.x {@code TikaGrpcConfig} / {@code grpc} config + * section. + * + * <pre>{@code + * <properties> + * <grpc> + * <allowPerRequestConfig>true</allowPerRequestConfig> + * <allowComponentModifications>true</allowComponentModifications> + * </grpc> + * </properties> + * }</pre> + */ +public class TikaGrpcConfig extends ConfigBase { + + /** + * When false, a {@code FetchAndParse} request that carries + * {@code additional_fetch_config_json} is rejected with {@code PERMISSION_DENIED}. + */ + private boolean allowPerRequestConfig = false; + + /** + * When false, the runtime component-mutation RPCs ({@code SaveFetcher}, {@code DeleteFetcher}) + * are rejected with {@code PERMISSION_DENIED}. Fetchers declared in the {@code <fetchers>} + * section of the tika-config are still loaded at startup regardless of this flag. + */ + private boolean allowComponentModifications = false; + + /** + * Loads the {@code <grpc>} section from the tika-config at the given path. If there is no + * {@code <grpc>} section, the locked-down defaults (every capability denied) are returned. + */ + public static TikaGrpcConfig load(Path tikaConfigPath) throws IOException, TikaConfigException { + TikaGrpcConfig config = new TikaGrpcConfig(); + try (InputStream is = Files.newInputStream(tikaConfigPath)) { + config.configure("grpc", is); + } + return config; + } + + public boolean isAllowPerRequestConfig() { + return allowPerRequestConfig; + } + + public void setAllowPerRequestConfig(boolean allowPerRequestConfig) { + this.allowPerRequestConfig = allowPerRequestConfig; + } + + public boolean isAllowComponentModifications() { + return allowComponentModifications; + } + + public void setAllowComponentModifications(boolean allowComponentModifications) { + this.allowComponentModifications = allowComponentModifications; + } +} diff --git a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServer.java b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServer.java index 70e8bcb917..dd2024308e 100644 --- a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServer.java +++ b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServer.java @@ -88,6 +88,13 @@ public class TikaGrpcServer { creds = channelCredBuilder.build(); } else { creds = InsecureServerCredentials.create(); + LOGGER.warn("tika-grpc is starting with NO transport security (plaintext) and binds to " + + "all interfaces with no application-level authentication. Anyone who can reach " + + "port {} can drive this server. Restrict it to a trusted network and/or enable " + + "TLS with --secure (see --cert-chain/--private-key/--trust-cert-collection/" + + "--client-auth-required). Dangerous capabilities (per-request config, runtime " + + "fetcher modifications) are denied by default; enable them only in the <grpc> " + + "section of the tika-config if you understand the implications.", port); } if (tikaConfigXml == null) { // Create a default tika config diff --git a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServerImpl.java b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServerImpl.java index fccd1def5e..a614f27cb9 100644 --- a/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServerImpl.java +++ b/tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServerImpl.java @@ -23,6 +23,8 @@ import java.io.Writer; import java.lang.reflect.InvocationTargetException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -51,6 +53,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.w3c.dom.Document; import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; import org.xml.sax.SAXException; import org.apache.tika.DeleteFetcherReply; @@ -101,6 +105,9 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { String tikaConfigPath; + //deny-by-default grpc capabilities, read from the <grpc> section of the tika-config + private final TikaGrpcConfig tikaGrpcConfig; + TikaGrpcServerImpl(String tikaConfigPath) throws TikaConfigException, IOException, ParserConfigurationException, TransformerException, SAXException { @@ -120,6 +127,11 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { expiringFetcherStore = new ExpiringFetcherStore(pipesConfig.getStaleFetcherTimeoutSeconds(), pipesConfig.getStaleFetcherDelaySeconds()); this.tikaConfigPath = tikaConfigPath; + this.tikaGrpcConfig = TikaGrpcConfig.load(Paths.get(tikaConfigPath)); + //load any fetchers declared in the <fetchers> section into the store before we rewrite the + //config below; otherwise updateTikaConfig() would wipe them. These are the trusted, + //operator-declared fetchers and are loaded regardless of allowComponentModifications. + loadFetchersFromConfig(); try { updateTikaConfig(); } catch (TikaException e) { @@ -183,9 +195,110 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { } } + /** + * Loads fetchers declared in the {@code <fetchers>} section of the tika-config into the store + * at startup. These are trusted, operator-declared fetchers (the secure way to provide + * fetchers when {@code allowComponentModifications} is false) and are marked permanent so the + * stale-fetcher job never expires them. The reverse of {@link #populateFetcherConfigs}. + */ + private void loadFetchersFromConfig() + throws ParserConfigurationException, IOException, SAXException, TikaConfigException { + Document tikaConfigDoc = + DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(tikaConfigPath); + NodeList fetchersElements = tikaConfigDoc.getElementsByTagName("fetchers"); + if (fetchersElements.getLength() == 0) { + return; + } + Element fetchersElement = (Element) fetchersElements.item(0); + NodeList fetcherNodes = fetchersElement.getElementsByTagName("fetcher"); + for (int i = 0; i < fetcherNodes.getLength(); i++) { + Element fetcherEl = (Element) fetcherNodes.item(i); + String fetcherClass = fetcherEl.getAttribute("class"); + if (StringUtils.isBlank(fetcherClass)) { + throw new TikaConfigException("<fetcher> in <fetchers> must have a 'class' attribute"); + } + String name = null; + Map<String, Object> params = new LinkedHashMap<>(); + NodeList children = fetcherEl.getChildNodes(); + for (int j = 0; j < children.getLength(); j++) { + Node child = children.item(j); + if (child.getNodeType() != Node.ELEMENT_NODE) { + continue; + } + String localName = child.getNodeName(); + if ("name".equals(localName)) { + name = child.getTextContent(); + } else { + params.put(localName, readParamValue((Element) child)); + } + } + if (StringUtils.isBlank(name)) { + throw new TikaConfigException("<fetcher> in <fetchers> must have a <name>"); + } + saveFetcher(name, fetcherClass, params, createTikaParamMap(params), true); + LOG.info("Loaded fetcher '{}' ({}) from config", name, fetcherClass); + } + } + + /** + * Reads a single config element written by {@link #populateFetcherConfigs}: an element with + * child elements is read back as a {@link List} of their text contents; otherwise the element's + * text content is returned. + */ + private static Object readParamValue(Element element) { + List<String> list = new ArrayList<>(); + NodeList children = element.getChildNodes(); + for (int i = 0; i < children.getLength(); i++) { + Node child = children.item(i); + if (child.getNodeType() == Node.ELEMENT_NODE) { + list.add(child.getTextContent()); + } + } + return list.isEmpty() ? element.getTextContent() : list; + } + + /** + * @return true (and sends {@code PERMISSION_DENIED} to the client) if this request carries + * per-request configuration that is not allowed. Note: we use {@code onError} rather than + * throwing so the client receives {@code PERMISSION_DENIED} rather than {@code UNKNOWN}. + */ + private boolean denyPerRequestConfig(FetchAndParseRequest request, + StreamObserver<?> responseObserver) { + if (StringUtils.isBlank(request.getAdditionalFetchConfigJson()) + || tikaGrpcConfig.isAllowPerRequestConfig()) { + return false; + } + responseObserver.onError(io.grpc.Status.PERMISSION_DENIED + .withDescription("Per-request configuration (additional_fetch_config_json) is " + + "disabled. Set 'allowPerRequestConfig' to true in the <grpc> section of " + + "the tika-config to enable it.") + .asRuntimeException()); + return true; + } + + /** + * @return true (and sends {@code PERMISSION_DENIED} to the client) if runtime component + * modifications (SaveFetcher/DeleteFetcher) are not allowed. + */ + private boolean denyComponentModifications(StreamObserver<?> responseObserver) { + if (tikaGrpcConfig.isAllowComponentModifications()) { + return false; + } + responseObserver.onError(io.grpc.Status.PERMISSION_DENIED + .withDescription("Runtime component modifications (SaveFetcher/DeleteFetcher) are " + + "disabled. Set 'allowComponentModifications' to true in the <grpc> section " + + "of the tika-config to enable them. Fetchers may instead be declared in the " + + "<fetchers> section of the tika-config.") + .asRuntimeException()); + return true; + } + @Override public void fetchAndParseServerSideStreaming(FetchAndParseRequest request, StreamObserver<FetchAndParseReply> responseObserver) { + if (denyPerRequestConfig(request, responseObserver)) { + return; + } fetchAndParseImpl(request, responseObserver); } @@ -195,6 +308,9 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { return new StreamObserver<>() { @Override public void onNext(FetchAndParseRequest fetchAndParseRequest) { + if (denyPerRequestConfig(fetchAndParseRequest, responseObserver)) { + return; + } fetchAndParseImpl(fetchAndParseRequest, responseObserver); } @@ -213,6 +329,9 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { @Override public void fetchAndParse(FetchAndParseRequest request, StreamObserver<FetchAndParseReply> responseObserver) { + if (denyPerRequestConfig(request, responseObserver)) { + return; + } fetchAndParseImpl(request, responseObserver); responseObserver.onCompleted(); } @@ -271,12 +390,15 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { @Override public void saveFetcher(SaveFetcherRequest request, StreamObserver<SaveFetcherReply> responseObserver) { + if (denyComponentModifications(responseObserver)) { + return; + } SaveFetcherReply reply = SaveFetcherReply.newBuilder().setFetcherId(request.getFetcherId()).build(); try { Map<String, Object> fetcherConfigMap = OBJECT_MAPPER.readValue(request.getFetcherConfigJson(), new TypeReference<>() {}); Map<String, Param> tikaParamsMap = createTikaParamMap(fetcherConfigMap); - saveFetcher(request.getFetcherId(), request.getFetcherClass(), fetcherConfigMap, tikaParamsMap); + saveFetcher(request.getFetcherId(), request.getFetcherClass(), fetcherConfigMap, tikaParamsMap, false); updateTikaConfig(); } catch (Exception e) { throw new RuntimeException(e); @@ -285,7 +407,7 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { responseObserver.onCompleted(); } - private void saveFetcher(String name, String fetcherClassName, Map<String, Object> paramsMap, Map<String, Param> tikaParamsMap) { + private void saveFetcher(String name, String fetcherClassName, Map<String, Object> paramsMap, Map<String, Param> tikaParamsMap, boolean permanent) { try { if (paramsMap == null) { paramsMap = new LinkedHashMap<>(); @@ -310,7 +432,7 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { } else { LOG.info("Creating new fetcher {}", name); } - expiringFetcherStore.createFetcher(abstractFetcher, configObject); + expiringFetcherStore.createFetcher(abstractFetcher, configObject, permanent); } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException | TikaConfigException e) { throw new RuntimeException(e); @@ -394,6 +516,9 @@ class TikaGrpcServerImpl extends TikaGrpc.TikaImplBase { @Override public void deleteFetcher(DeleteFetcherRequest request, StreamObserver<DeleteFetcherReply> responseObserver) { + if (denyComponentModifications(responseObserver)) { + return; + } boolean successfulDelete = deleteFetcher(request.getFetcherId()); if (successfulDelete) { try { diff --git a/tika-grpc/src/test/java/org/apache/tika/pipes/grpc/TikaGrpcCapabilityTest.java b/tika-grpc/src/test/java/org/apache/tika/pipes/grpc/TikaGrpcCapabilityTest.java new file mode 100644 index 0000000000..f37c316d5c --- /dev/null +++ b/tika-grpc/src/test/java/org/apache/tika/pipes/grpc/TikaGrpcCapabilityTest.java @@ -0,0 +1,169 @@ +/* + * 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.tika.pipes.grpc; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.UUID; + +import com.asarkar.grpc.test.GrpcCleanupExtension; +import com.asarkar.grpc.test.Resources; +import io.grpc.ManagedChannel; +import io.grpc.Server; +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import io.grpc.inprocess.InProcessChannelBuilder; +import io.grpc.inprocess.InProcessServerBuilder; +import org.apache.commons.io.FileUtils; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import org.apache.tika.DeleteFetcherRequest; +import org.apache.tika.FetchAndParseRequest; +import org.apache.tika.GetFetcherReply; +import org.apache.tika.GetFetcherRequest; +import org.apache.tika.SaveFetcherReply; +import org.apache.tika.SaveFetcherRequest; +import org.apache.tika.TikaGrpc; +import org.apache.tika.pipes.fetcher.fs.FileSystemFetcher; + +/** + * Verifies the deny-by-default grpc capabilities (the {@code <grpc>} config section) and that + * fetchers declared in the {@code <fetchers>} section are loaded at startup even when runtime + * component modifications are denied. + */ +@ExtendWith(GrpcCleanupExtension.class) +public class TikaGrpcCapabilityTest { + + private static final String PIPES_SECTION = + " <pipes><params><numClients>1</numClients><timeoutMillis>60000</timeoutMillis>" + + "<maxForEmitBatchBytes>-1</maxForEmitBatchBytes></params></pipes>\n"; + + private static File writeConfig(String grpcSection, String fetchersSection) throws IOException { + File f = new File("target", "grpc-cap-" + UUID.randomUUID() + ".xml"); + String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<properties>\n" + + PIPES_SECTION + grpcSection + fetchersSection + "</properties>\n"; + FileUtils.writeStringToFile(f, xml, StandardCharsets.UTF_8); + f.deleteOnExit(); + return f; + } + + private static TikaGrpc.TikaBlockingStub startServer(Resources resources, File config) throws Exception { + String serverName = InProcessServerBuilder.generateName(); + Server server = InProcessServerBuilder + .forName(serverName) + .directExecutor() + .addService(new TikaGrpcServerImpl(config.getAbsolutePath())) + .build() + .start(); + resources.register(server, Duration.ofSeconds(10)); + ManagedChannel channel = InProcessChannelBuilder + .forName(serverName) + .directExecutor() + .build(); + resources.register(channel, Duration.ofSeconds(10)); + return TikaGrpc.newBlockingStub(channel); + } + + private static void assertPermissionDenied(Executable call) { + StatusRuntimeException ex = assertThrows(StatusRuntimeException.class, call::run); + assertEquals(Status.PERMISSION_DENIED.getCode(), ex.getStatus().getCode()); + } + + @FunctionalInterface + private interface Executable { + void run() throws Exception; + } + + @Test + public void lockedByDefaultDeniesSaveFetcher(Resources resources) throws Exception { + //no <grpc> section -> deny by default + TikaGrpc.TikaBlockingStub stub = startServer(resources, writeConfig("", "<fetchers></fetchers>\n")); + assertPermissionDenied(() -> stub.saveFetcher(SaveFetcherRequest + .newBuilder() + .setFetcherId("f") + .setFetcherClass(FileSystemFetcher.class.getName()) + .setFetcherConfigJson("{}") + .build())); + } + + @Test + public void lockedByDefaultDeniesDeleteFetcher(Resources resources) throws Exception { + TikaGrpc.TikaBlockingStub stub = startServer(resources, writeConfig("", "<fetchers></fetchers>\n")); + assertPermissionDenied(() -> stub.deleteFetcher(DeleteFetcherRequest + .newBuilder() + .setFetcherId("f") + .build())); + } + + @Test + public void lockedByDefaultDeniesPerRequestConfig(Resources resources) throws Exception { + //the per-request-config guard runs before the fetcher-existence check, so a non-blank + //additional_fetch_config_json is denied regardless of whether the fetcher exists + TikaGrpc.TikaBlockingStub stub = startServer(resources, writeConfig("", "<fetchers></fetchers>\n")); + assertPermissionDenied(() -> stub.fetchAndParse(FetchAndParseRequest + .newBuilder() + .setFetcherId("f") + .setFetchKey("k") + .setAdditionalFetchConfigJson("{\"basePath\":\"/etc\"}") + .build())); + } + + @Test + public void configDeclaredFetcherLoadsAndIsUsableWhenLocked(Resources resources) throws Exception { + String basePath = new File("target").getAbsolutePath(); + String fetchers = "<fetchers><fetcher class=\"" + FileSystemFetcher.class.getName() + "\">" + + "<name>fsf</name><basePath>" + basePath + "</basePath></fetcher></fetchers>\n"; + //no <grpc> section -> runtime modifications denied, but the config-declared fetcher must load + TikaGrpc.TikaBlockingStub stub = startServer(resources, writeConfig("", fetchers)); + + GetFetcherReply reply = stub.getFetcher(GetFetcherRequest + .newBuilder() + .setFetcherId("fsf") + .build()); + assertEquals("fsf", reply.getFetcherId()); + assertEquals(FileSystemFetcher.class.getName(), reply.getFetcherClass()); + + //...but runtime SaveFetcher is still denied + assertPermissionDenied(() -> stub.saveFetcher(SaveFetcherRequest + .newBuilder() + .setFetcherId("other") + .setFetcherClass(FileSystemFetcher.class.getName()) + .setFetcherConfigJson("{\"basePath\":\"" + basePath + "\"}") + .build())); + } + + @Test + public void unlockedAllowsSaveFetcher(Resources resources) throws Exception { + String grpc = "<grpc><allowComponentModifications>true</allowComponentModifications>" + + "<allowPerRequestConfig>true</allowPerRequestConfig></grpc>\n"; + TikaGrpc.TikaBlockingStub stub = startServer(resources, writeConfig(grpc, "<fetchers></fetchers>\n")); + String basePath = new File("target").getAbsolutePath(); + SaveFetcherReply reply = stub.saveFetcher(SaveFetcherRequest + .newBuilder() + .setFetcherId("f") + .setFetcherClass(FileSystemFetcher.class.getName()) + .setFetcherConfigJson("{\"basePath\":\"" + basePath + "\"}") + .build()); + assertEquals("f", reply.getFetcherId()); + } +} diff --git a/tika-grpc/src/test/resources/tika-pipes-test-config.xml b/tika-grpc/src/test/resources/tika-pipes-test-config.xml index e4006edb35..89b775e3e3 100644 --- a/tika-grpc/src/test/resources/tika-pipes-test-config.xml +++ b/tika-grpc/src/test/resources/tika-pipes-test-config.xml @@ -30,6 +30,12 @@ <maxForEmitBatchBytes>-1</maxForEmitBatchBytes> <!-- disable emit --> </params> </pipes> + <grpc> + <!-- these tests exercise the runtime SaveFetcher/DeleteFetcher RPCs and per-request config, + which are denied by default; opt in here --> + <allowComponentModifications>true</allowComponentModifications> + <allowPerRequestConfig>true</allowPerRequestConfig> + </grpc> <fetchers> </fetchers> </properties> diff --git a/tika-serialization/src/main/java/org/apache/tika/serialization/ParseContextDeserializer.java b/tika-serialization/src/main/java/org/apache/tika/serialization/ParseContextDeserializer.java index f37e2c07e5..e1059d6347 100644 --- a/tika-serialization/src/main/java/org/apache/tika/serialization/ParseContextDeserializer.java +++ b/tika-serialization/src/main/java/org/apache/tika/serialization/ParseContextDeserializer.java @@ -54,6 +54,10 @@ public class ParseContextDeserializer extends JsonDeserializer<ParseContext> { try { Class clazz = Class.forName(className); Class superClazz = className.equals(superClassName) ? clazz : Class.forName(superClassName); + //a request must never be able to submit a whole new Parser/Detector/EncodingDetector + //via config. Guard the ParseContext map-key too, since it is attacker-controlled + //independently of _class. + TikaJsonDeserializer.assertNotComponent(superClazz); parseContext.set(superClazz, TikaJsonDeserializer.deserialize(clazz, obj)); } catch (ReflectiveOperationException ex) { throw new IOException(ex); diff --git a/tika-serialization/src/main/java/org/apache/tika/serialization/TikaJsonDeserializer.java b/tika-serialization/src/main/java/org/apache/tika/serialization/TikaJsonDeserializer.java index 1b180afdcf..ed22d448ee 100644 --- a/tika-serialization/src/main/java/org/apache/tika/serialization/TikaJsonDeserializer.java +++ b/tika-serialization/src/main/java/org/apache/tika/serialization/TikaJsonDeserializer.java @@ -29,6 +29,10 @@ import java.util.Optional; import com.fasterxml.jackson.databind.JsonNode; +import org.apache.tika.detect.Detector; +import org.apache.tika.detect.EncodingDetector; +import org.apache.tika.parser.Parser; + /** * See the notes @link{TikaJsonSerializer}. * <p> @@ -36,6 +40,37 @@ import com.fasterxml.jackson.databind.JsonNode; */ public class TikaJsonDeserializer { + /** + * Pluggable components that must never be instantiated from a serialized + * {@link org.apache.tika.parser.ParseContext}. Submitting a whole new {@link Parser}, + * {@link Detector} or {@link EncodingDetector} via request-time configuration is a remote + * code-execution / SSRF surface (e.g. {@code ExternalParser.setCommandLine}, + * {@code ExternalParser.setCommand}, {@code TesseractOCRParser.setTesseractPath}). Configuring + * an already-loaded component is fine; instantiating a new one from a request is not. + */ + private static final Class<?>[] BLOCKED_COMPONENT_TYPES = + {Parser.class, Detector.class, EncodingDetector.class}; + + /** + * Security guard enforced at the single (recursive) instantiation chokepoint + * ({@link #deserialize(Class, JsonNode)}), so it also blocks a component smuggled into a setter + * that takes one (e.g. {@code ExternalParser.setOutputParser(Parser)}). + * + * @param clazz the class about to be instantiated from JSON + * @throws SecurityException if {@code clazz} is a {@link Parser}, {@link Detector} or + * {@link EncodingDetector} + */ + public static void assertNotComponent(Class<?> clazz) { + for (Class<?> blocked : BLOCKED_COMPONENT_TYPES) { + if (blocked.isAssignableFrom(clazz)) { + throw new SecurityException("Refusing to instantiate a " + blocked.getSimpleName() + + " (" + clazz.getName() + ") from a serialized ParseContext. " + + blocked.getSimpleName() + "s may not be submitted via request-time " + + "configuration."); + } + } + } + public static Optional deserializeObject(JsonNode root) { if (!root.isObject()) { throw new IllegalArgumentException("root needs to be an object"); @@ -55,6 +90,7 @@ public class TikaJsonDeserializer { } public static <T> T deserialize(Class<? extends T> clazz, JsonNode root) throws ReflectiveOperationException { + assertNotComponent(clazz); T obj = clazz .getDeclaredConstructor() .newInstance(); diff --git a/tika-serialization/src/test/java/org/apache/tika/serialization/TestParseContextComponentInjection.java b/tika-serialization/src/test/java/org/apache/tika/serialization/TestParseContextComponentInjection.java new file mode 100644 index 0000000000..eebae64092 --- /dev/null +++ b/tika-serialization/src/test/java/org/apache/tika/serialization/TestParseContextComponentInjection.java @@ -0,0 +1,164 @@ +/* + * 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.tika.serialization; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.Charset; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.Test; + +import org.apache.tika.detect.Detector; +import org.apache.tika.detect.EncodingDetector; +import org.apache.tika.metadata.Metadata; +import org.apache.tika.mime.MediaType; +import org.apache.tika.parser.ParseContext; +import org.apache.tika.parser.Parser; +import org.apache.tika.parser.external2.ExternalParser; + +/** + * A request must never be able to submit a whole new {@link Parser}, {@link Detector} or + * {@link EncodingDetector} via a serialized {@code ParseContext} (e.g. a tika-server + * {@code /pipes} or {@code /async} request). These are command-injection / SSRF surfaces + * (e.g. {@code ExternalParser.setCommandLine}, {@code TesseractOCRParser.setTesseractPath}). + * Configuration of benign, non-component objects must still work. + */ +public class TestParseContextComponentInjection { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final String CLASS_KEY = TikaJsonSerializer.INSTANTIATED_CLASS_KEY; + + /** A ParseContext "contents" node of the form { "<className>": { "_class": "<className>" } }. */ + private static JsonNode entry(String className) throws IOException { + return MAPPER.readTree("{\"" + className + "\":{\"" + CLASS_KEY + "\":\"" + className + "\"}}"); + } + + private static boolean hasSecurityCause(Throwable t) { + while (t != null) { + if (t instanceof SecurityException) { + return true; + } + t = t.getCause(); + } + return false; + } + + @Test + public void assertNotComponentRejectsParserDetectorEncodingDetector() { + assertThrows(SecurityException.class, () -> TikaJsonDeserializer.assertNotComponent(ExternalParser.class)); + assertThrows(SecurityException.class, () -> TikaJsonDeserializer.assertNotComponent(EvilDetector.class)); + assertThrows(SecurityException.class, () -> TikaJsonDeserializer.assertNotComponent(EvilEncodingDetector.class)); + } + + @Test + public void assertNotComponentAllowsBenignConfig() { + assertDoesNotThrow(() -> TikaJsonDeserializer.assertNotComponent(BenignConfig.class)); + } + + @Test + public void deserializeChokepointRejectsComponents() throws IOException { + //this is exactly what the nested/recursive deserialization path invokes + JsonNode empty = MAPPER.readTree("{}"); + assertThrows(SecurityException.class, () -> TikaJsonDeserializer.deserialize(ExternalParser.class, empty)); + assertThrows(SecurityException.class, () -> TikaJsonDeserializer.deserialize(EvilDetector.class, empty)); + assertThrows(SecurityException.class, () -> TikaJsonDeserializer.deserialize(EvilEncodingDetector.class, empty)); + } + + @Test + public void readParseContextRejectsTopLevelParser() throws IOException { + JsonNode node = entry(ExternalParser.class.getName()); + assertThrows(SecurityException.class, () -> ParseContextDeserializer.readParseContext(node)); + } + + @Test + public void readParseContextRejectsTopLevelDetector() throws IOException { + JsonNode node = entry(EvilDetector.class.getName()); + assertThrows(SecurityException.class, () -> ParseContextDeserializer.readParseContext(node)); + } + + @Test + public void readParseContextRejectsTopLevelEncodingDetector() throws IOException { + JsonNode node = entry(EvilEncodingDetector.class.getName()); + assertThrows(SecurityException.class, () -> ParseContextDeserializer.readParseContext(node)); + } + + @Test + public void readParseContextRejectsNestedParser() throws IOException { + //a benign holder whose Parser-typed setter is fed a nested Parser _class must be blocked + //by the recursive chokepoint, not just the top level. + String holder = Holder.class.getName(); + String evil = ExternalParser.class.getName(); + JsonNode node = MAPPER.readTree("{\"" + holder + "\":{\"" + CLASS_KEY + "\":\"" + holder + "\"," + + "\"outputParser\":{\"" + CLASS_KEY + "\":\"" + evil + "\"}}}"); + Exception ex = assertThrows(Exception.class, () -> ParseContextDeserializer.readParseContext(node)); + assertTrue(hasSecurityCause(ex), "expected a SecurityException in the cause chain, got: " + ex); + } + + @Test + public void readParseContextAllowsBenignConfig() throws IOException { + ParseContext pc = ParseContextDeserializer.readParseContext(entry(BenignConfig.class.getName())); + assertNotNull(pc.get(BenignConfig.class)); + } + + // ---- test helpers ---- + + public static class BenignConfig { + private String value; + + public String getValue() { + return value; + } + + public void setValue(String value) { + this.value = value; + } + } + + /** Non-component object with a {@link Parser}-typed setter, to exercise the nested/recursive guard. */ + public static class Holder { + private Parser outputParser; + + public Parser getOutputParser() { + return outputParser; + } + + public void setOutputParser(Parser outputParser) { + this.outputParser = outputParser; + } + } + + public static class EvilDetector implements Detector { + @Override + public MediaType detect(InputStream input, Metadata metadata) throws IOException { + return null; + } + } + + public static class EvilEncodingDetector implements EncodingDetector { + @Override + public Charset detect(InputStream input, Metadata metadata) throws IOException { + return null; + } + } +}
