From f8ff37ebd05da859396802b7d11ebdac0d7d803e Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Tue, 27 Oct 2026 15:50:33 +0100 Subject: [PATCH 1/3] Harden ORE harvest ingest against SSRF The OAI-ORE ingest crosswalk fetched every href in a remote resource map through URL.openStream(), guarded only by a scheme allow-list and an optional, off-by-default host-prefix check. A collection admin could point a harvest at a hostile OAI source and make the server fetch internal hosts such as 127.0.0.1 or the cloud metadata endpoint 169.254.169.254. Add a self-contained egress guard in org.dspace.harvest.ore: - BlockedAddressPredicate blocks loopback, private, link-local, CGNAT, reserved, documentation and cloud-metadata ranges on the raw address bytes, unwrapping IPv4-in-IPv6 and defeating decimal/hex/IPv6-embedding obfuscation. - OreUrlValidator fails closed: http/https only, no userinfo, resolvable host, host confined to the collection's oai_source (or a configured prefix), and every resolved address checked against the predicate. - SafeResourceFetcher follows redirects by hand, re-validating and re-resolving each hop, pins the socket to the validated address, blocks https->http downgrades, applies connect/read timeouts and a response size cap. Wire the crosswalk to the fetcher with a policy anchored on the oai_source (set from OAIHarvester.processRecord via HarvestPolicyAware); callers with no harvest context get the fail-closed policy. A rejected resource skips and rolls back that record so a bitstream-less item is not committed. Reconcile oai.cfg: drop the obsolete validateUrlPrefix key and document the new blockInternalAddresses / allowedInternalHosts / maxRedirects / timeout / maxBytes keys; allowedUrlPrefix remains the cross-host escape hatch. --- .../crosswalk/OREIngestionCrosswalk.java | 128 ++++---- .../java/org/dspace/harvest/OAIHarvester.java | 84 +++++- .../harvest/ore/BlockedAddressPredicate.java | 216 ++++++++++++++ .../harvest/ore/HarvestPolicyAware.java | 21 ++ .../dspace/harvest/ore/OreEgressPolicy.java | 139 +++++++++ .../ore/OreResourceRejectedException.java | 51 ++++ .../dspace/harvest/ore/OreUrlValidator.java | 242 ++++++++++++++++ .../dspace/harvest/ore/RejectionReason.java | 34 +++ .../harvest/ore/SafeResourceFetcher.java | 274 ++++++++++++++++++ .../ore/BlockedAddressPredicateTest.java | 152 ++++++++++ .../harvest/ore/OreUrlValidatorTest.java | 208 +++++++++++++ .../harvest/ore/SafeResourceFetcherTest.java | 206 +++++++++++++ dspace/config/modules/oai.cfg | 40 ++- 13 files changed, 1709 insertions(+), 86 deletions(-) create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/BlockedAddressPredicate.java create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/HarvestPolicyAware.java create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/OreEgressPolicy.java create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/OreResourceRejectedException.java create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/OreUrlValidator.java create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/RejectionReason.java create mode 100644 dspace-api/src/main/java/org/dspace/harvest/ore/SafeResourceFetcher.java create mode 100644 dspace-api/src/test/java/org/dspace/harvest/ore/BlockedAddressPredicateTest.java create mode 100644 dspace-api/src/test/java/org/dspace/harvest/ore/OreUrlValidatorTest.java create mode 100644 dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java diff --git a/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java b/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java index 9e890a6046fa..e90d7a1650fb 100644 --- a/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java +++ b/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java @@ -7,23 +7,17 @@ */ package org.dspace.content.crosswalk; -import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; -import java.net.ConnectException; -import java.net.URI; -import java.net.URISyntaxException; -import java.net.URL; import java.sql.SQLException; import java.text.NumberFormat; import java.util.Arrays; import java.util.Date; import java.util.HashSet; import java.util.List; -import java.util.Locale; -import java.util.Objects; import java.util.Set; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.logging.log4j.Logger; import org.dspace.authorize.AuthorizeException; import org.dspace.content.Bitstream; @@ -38,6 +32,12 @@ import org.dspace.content.service.ItemService; import org.dspace.core.Constants; import org.dspace.core.Context; +import org.dspace.harvest.ore.HarvestPolicyAware; +import org.dspace.harvest.ore.OreEgressPolicy; +import org.dspace.harvest.ore.OreResourceRejectedException; +import org.dspace.harvest.ore.OreUrlValidator; +import org.dspace.harvest.ore.RejectionReason; +import org.dspace.harvest.ore.SafeResourceFetcher; import org.dspace.services.ConfigurationService; import org.dspace.services.factory.DSpaceServicesFactory; import org.jdom2.Attribute; @@ -56,7 +56,7 @@ * @author Alexey Maslov */ public class OREIngestionCrosswalk - implements IngestionCrosswalk { + implements IngestionCrosswalk, HarvestPolicyAware { /** * log4j category */ @@ -84,6 +84,25 @@ public class OREIngestionCrosswalk protected ItemService itemService = ContentServiceFactory.getInstance().getItemService(); protected ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService(); + private final SafeResourceFetcher resourceFetcher = new SafeResourceFetcher(); + private OreEgressPolicy oreEgressPolicy; + + @Override + public void setOreEgressPolicy(OreEgressPolicy policy) { + this.oreEgressPolicy = policy; + } + + /** + * The packagers and the XSLT CLI reach this crosswalk with no harvest context, so they get the + * fail-closed policy rather than no policy at all. + */ + private OreEgressPolicy egressPolicy() { + if (oreEgressPolicy == null) { + oreEgressPolicy = OreEgressPolicy.strictest(configurationService); + } + return oreEgressPolicy; + } + @Override public void ingest(Context context, DSpaceObject dso, List metadata, boolean createMissingMetadataFields) @@ -174,23 +193,18 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea targetBundle = targetBundles.get(0); } - URL ARurl = null; InputStream in = null; if (href != null) { try { // Make sure the url string escapes all the oddball characters String processedURL = encodeForURL(href); - if (validResourceUri(entryId, processedURL)) { - // Generate a request for the aggregated resource - ARurl = new URL(processedURL); - in = ARurl.openStream(); - } else { - throw new FileNotFoundException("Failed to validate " + processedURL); - } - } catch (FileNotFoundException fe) { - log.error("The provided URI failed to return a resource: " + href); - } catch (ConnectException fe) { - log.error("The provided URI was invalid: " + href); + // The trust anchor of the egress policy is the collection's oai_source, not entryId: + // entryId is read from this remote document and is therefore attacker-controlled. + in = resourceFetcher.fetch(OreUrlValidator.parse(processedURL), egressPolicy()); + } catch (IOException ioe) { + // a transport failure has to drop this record only; escaping here would stop the whole + // collection, and after removeAllBundles it would commit the item without its files + throw transferFailed(href, ioe); } } else { throw new CrosswalkException("Entry did not contain link to resource: " + entryId); @@ -198,7 +212,14 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea // ingest and update if (in != null) { - Bitstream newBitstream = bitstreamService.create(context, targetBundle, in); + Bitstream newBitstream; + try { + newBitstream = bitstreamService.create(context, targetBundle, in); + } catch (IOException ioe) { + // the body is only read here, and the bitstore wraps everything it catches in a plain + // IOException, so both the size cap and a mid-transfer failure surface at this point + throw transferFailed(href, ioe); + } String bsName = resource.getAttributeValue("title"); newBitstream.setName(context, bsName); @@ -263,60 +284,19 @@ private String encodeForURL(String sourceString) { } /** - * Validate a resource URI against the host and scheme of the remote OAI endpoint, or a configured - * list of allowed prefixes. - * This still implicitly "trusts" the remote OAI server, but will reject resource URIs with a totally - * different hostname to avoid downloading malicious resources from a compromised endpoint. - * Even if the URL prefix validation is disabled, schemes will still be enforced to http(s) so file:/// and - * other unwanted schemes cannot be used - * @param entryUrl the entryId of the parent ORE resource - * @param resourceUrl the resource URL of the aggregated ORE resource - * @return result of the validation + * Turn an I/O failure while fetching or storing a file into a rejection of this one record. + * + * @param href the resource that could not be transferred + * @param cause the failure, kept so the real reason still reaches the log + * @return the rejection to throw */ - private boolean validResourceUri(String entryUrl, String resourceUrl) { - try { - Set allowedSchemes = Set.of("http", "https"); - URI entryUri = new URI(entryUrl).normalize(); - URI resourceUri = new URI(resourceUrl).normalize(); - String scheme = resourceUri.getScheme(); - - if (scheme == null || - !allowedSchemes.contains(scheme.toLowerCase(Locale.ROOT))) { - log.warn("Illegal scheme requested for ORE resource: {}", resourceUri); - return false; - } - - if (configurationService.getBooleanProperty("oai.harvester.ore.file.validateUrlPrefix", false)) { - for (String allowedPrefix : configurationService - .getArrayProperty("oai.harvester.ore.file.allowedUrlPrefix")) { - URI allowedUri = new URI(allowedPrefix).normalize(); - // Return true on the first allowed prefix match - if (Objects.equals(resourceUri.getScheme(), allowedUri.getScheme()) - && Objects.equals(resourceUri.getHost().toLowerCase(Locale.ROOT), - allowedUri.getHost().toLowerCase(Locale.ROOT))) { - return true; - } - } - - // If no allowed prefixes were matched, we require scheme + host to match the remote OAI server - if (!Objects.equals(entryUri.getScheme(), resourceUri.getScheme())) { - log.warn("Illegal scheme requested for ORE resource: {}", resourceUri); - return false; - } - if (!Objects.equals( - entryUri.getHost().toLowerCase(Locale.ROOT), - resourceUri.getHost().toLowerCase(Locale.ROOT))) { - log.warn("Illegal host requested for ORE resource: {}", resourceUri); - return false; - } - } - - return true; - - } catch (URISyntaxException e) { - log.warn("Could not validate ORE resource URI: {}", resourceUrl); - return false; - } + private OreResourceRejectedException transferFailed(String href, IOException cause) { + // the cap is enforced while the body streams, so it arrives wrapped rather than as itself + boolean tooLarge = + ExceptionUtils.indexOfType(cause, SafeResourceFetcher.ResponseTooLargeException.class) >= 0; + return new OreResourceRejectedException( + tooLarge ? RejectionReason.RESPONSE_TOO_LARGE : RejectionReason.FETCH_FAILED, href, + tooLarge ? "size cap exceeded" : "transfer failed", cause); } } diff --git a/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java b/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java index 1b8d0002ce2c..0b6ba51d11d5 100644 --- a/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java +++ b/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java @@ -64,6 +64,10 @@ import org.dspace.handle.factory.HandleServiceFactory; import org.dspace.handle.service.HandleService; import org.dspace.harvest.factory.HarvestServiceFactory; +import org.dspace.harvest.ore.HarvestPolicyAware; +import org.dspace.harvest.ore.OreEgressPolicy; +import org.dspace.harvest.ore.OreResourceRejectedException; +import org.dspace.harvest.ore.RejectionReason; import org.dspace.harvest.service.HarvestedCollectionService; import org.dspace.harvest.service.HarvestedItemService; import org.dspace.services.ConfigurationService; @@ -121,6 +125,12 @@ public class OAIHarvester { Collection targetCollection; HarvestedCollection harvestRow; + // Number of records dropped because the ORE egress policy refused one of their files + private int rejectedRecords; + + // Number of records dropped because one of their files could not be downloaded at all + private int failedRecords; + // our context Context ourContext; @@ -454,12 +464,33 @@ public void runHarvest() throws SQLException, IOException, AuthorizeException { Date finishTime = new Date(); long timeTaken = finishTime.getTime() - startTime.getTime(); harvestRow.setHarvestStartTime(startTime); - harvestRow.setHarvestMessage("Harvest from " + oaiSource + " successful"); - harvestRow.setHarvestStatus(HarvestedCollection.STATUS_READY); - harvestRow.setLastHarvested(startTime); + int skippedRecords = rejectedRecords + failedRecords; + if (skippedRecords > 0) { + // deliberately generic: the rejected URLs and the addresses they resolved to belong in the log only + StringBuilder message = new StringBuilder("Harvest from ").append(oaiSource) + .append(" completed, but ").append(skippedRecords).append(" record(s) were skipped"); + if (rejectedRecords > 0) { + message.append("; ").append(rejectedRecords) + .append(" because a linked file was not allowed to be downloaded"); + } + if (failedRecords > 0) { + message.append("; ").append(failedRecords) + .append(" because a linked file could not be downloaded"); + } + harvestRow.setHarvestMessage(message.toString()); + harvestRow.setHarvestStatus(HarvestedCollection.STATUS_OAI_ERROR); + // lastHarvested deliberately not advanced: the skipped records fall outside the next incremental + // window otherwise and are never offered again; records already imported are skipped by the + // datestamp check in processRecord + } else { + harvestRow.setHarvestMessage("Harvest from " + oaiSource + " successful"); + harvestRow.setHarvestStatus(HarvestedCollection.STATUS_READY); + harvestRow.setLastHarvested(startTime); + } log.info( - "Harvest from " + oaiSource + " successful. The process took " + timeTaken + " milliseconds. Harvested " - + currentRecord + " items."); + "Harvest from " + oaiSource + (skippedRecords > 0 + ? " completed with " + skippedRecords + " skipped record(s)." : " successful.") + + " The process took " + timeTaken + " milliseconds. Harvested " + currentRecord + " items."); harvestedCollectionService.update(ourContext, harvestRow); ourContext.setMode(originalMode); @@ -476,6 +507,29 @@ private void reloadRequiredEntities() throws SQLException { harvestRow = ourContext.reloadEntity(harvestRow); } + /** + * Drop a record whose ORE ingest was refused by the egress policy. Everything this record wrote is still + * uncommitted, so rolling back restores the item exactly as it was before the ingest started. + * + * @param itemOaiID the OAI identifier of the record being dropped + * @param ore the rejection + * @throws SQLException if the rollback fails + */ + protected void rejectRecord(String itemOaiID, OreResourceRejectedException ore) throws SQLException { + // a remote server that would not serve the file is a transport problem, not a policy decision, and the + // administrator has to be told which of the two it was + if (ore.getReason() == RejectionReason.FETCH_FAILED) { + failedRecords++; + } else { + rejectedRecords++; + } + log.warn("Skipping record {} for collection {}: ORE file {} harvested from {} was rejected ({})", + itemOaiID, targetCollection.getID(), ore.getUrl(), harvestRow.getOaiSource(), ore.getReason()); + ourContext.restoreAuthSystemState(); + ourContext.rollback(); + reloadRequiredEntities(); + } + /** * Process an individual PMH record, making (or updating) a corresponding DSpace Item. * @@ -532,6 +586,10 @@ protected void processRecord(Element record, String OREPrefix, final long curren if (harvestRow.getHarvestType() > 1) { oreREM = getMDrecord(harvestRow.getOaiSource(), itemOaiID, OREPrefix).get(0); ORExwalk = (IngestionCrosswalk) pluginService.getNamedPlugin(IngestionCrosswalk.class, this.ORESerialKey); + if (ORExwalk instanceof HarvestPolicyAware) { + ((HarvestPolicyAware) ORExwalk) + .setOreEgressPolicy(OreEgressPolicy.from(harvestRow, configurationService)); + } } // Ignore authorization @@ -570,7 +628,14 @@ protected void processRecord(Element record, String OREPrefix, final long curren if (harvestRow.getHarvestType() == 3) { log.info("Running ORE ingest on: " + item.getHandle()); itemService.removeAllBundles(ourContext, item); - ORExwalk.ingest(ourContext, item, oreREM, true); + try { + ORExwalk.ingest(ourContext, item, oreREM, true); + } catch (OreResourceRejectedException ore) { + // the metadata and the bundles are already gone at this point, so the whole record has to + // go back rather than leave the existing item stripped of its files + rejectRecord(itemOaiID, ore); + return; + } } } else { // NOTE: did not find, so we create (presumably, there will never be a case where an item already @@ -589,7 +654,12 @@ protected void processRecord(Element record, String OREPrefix, final long curren } if (harvestRow.getHarvestType() == 3) { - ORExwalk.ingest(ourContext, item, oreREM, true); + try { + ORExwalk.ingest(ourContext, item, oreREM, true); + } catch (OreResourceRejectedException ore) { + rejectRecord(itemOaiID, ore); + return; + } } // see if a handle can be extracted for the item diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/BlockedAddressPredicate.java b/dspace-api/src/main/java/org/dspace/harvest/ore/BlockedAddressPredicate.java new file mode 100644 index 000000000000..d78dbc58427f --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/BlockedAddressPredicate.java @@ -0,0 +1,216 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import java.net.InetAddress; +import java.net.UnknownHostException; +import java.util.Arrays; +import java.util.function.Predicate; + +/** + * Decides whether an address is one the server must never be talked into fetching from: loopback, private, + * link-local, CGNAT, reserved, documentation and cloud-metadata ranges. + *

+ * Matching is done on the raw address bytes, never on the textual form, and IPv6 addresses that embed an + * IPv4 address are unwrapped and re-checked recursively. {@code true} means BLOCKED. + */ +public class BlockedAddressPredicate implements Predicate { + + /** Ranges the java.net helpers below do not cover. */ + private static final Range[] BLOCKED_RANGES = { + // IPv4 + range(4, 8, "0.0.0.0/8 (this network)", 0), + range(4, 10, "100.64.0.0/10 (CGNAT)", 100, 64), + range(4, 24, "192.0.0.0/24 (IETF protocol assignments)", 192, 0, 0), + range(4, 24, "192.0.2.0/24 (TEST-NET-1)", 192, 0, 2), + range(4, 24, "198.51.100.0/24 (TEST-NET-2)", 198, 51, 100), + range(4, 24, "203.0.113.0/24 (TEST-NET-3)", 203, 0, 113), + range(4, 24, "192.88.99.0/24 (6to4 relay anycast)", 192, 88, 99), + range(4, 15, "198.18.0.0/15 (benchmarking)", 198, 18), + range(4, 4, "240.0.0.0/4 (reserved)", 240), + // IPv6 + range(16, 7, "fc00::/7 (unique local)", 0xfc), + range(16, 128, "::/128 (unspecified)", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), + range(16, 96, "::/96 (IPv4-compatible)", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), + range(16, 96, "::ffff:0:0:0/96 (IPv4-translated)", 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0, 0), + range(16, 96, "64:ff9b::/96 (NAT64)", 0x00, 0x64, 0xff, 0x9b, 0, 0, 0, 0, 0, 0, 0, 0), + range(16, 64, "100::/64 (discard-only)", 0x01, 0x00, 0, 0, 0, 0, 0, 0), + range(16, 32, "2001::/32 (Teredo)", 0x20, 0x01, 0x00, 0x00), + range(16, 32, "2001:db8::/32 (documentation)", 0x20, 0x01, 0x0d, 0xb8), + range(16, 16, "2002::/16 (6to4)", 0x20, 0x02) + }; + + @Override + public boolean test(InetAddress address) { + return matchedRange(address) != null; + } + + /** + * Describe why an address is blocked, for logging. + * + * @param address the address to check + * @return a description such as "100.64.0.0/10 (CGNAT)", or null if the address is acceptable + */ + public String matchedRange(InetAddress address) { + if (address == null) { + return "unresolved address"; + } + + String category = wellKnownCategory(address); + if (category != null) { + return category; + } + + byte[] raw = address.getAddress(); + for (Range blocked : BLOCKED_RANGES) { + if (blocked.contains(raw)) { + return blocked.label; + } + } + + // an IPv6 address can carry an IPv4 address that has to be judged on its own merits + InetAddress embedded = embeddedIPv4(raw); + if (embedded != null) { + String inner = matchedRange(embedded); + if (inner != null) { + return inner + " embedded in IPv6"; + } + } + + return null; + } + + /** + * Compare the leading {@code bits} of two equally sized raw addresses. + * + * @param address the address bytes + * @param network the network bytes + * @param bits the prefix length + * @return true when the address falls inside the network + */ + static boolean prefixMatches(byte[] address, byte[] network, int bits) { + if (address.length != network.length || bits < 0 || bits > address.length * 8) { + return false; + } + int wholeBytes = bits / 8; + for (int i = 0; i < wholeBytes; i++) { + if (address[i] != network[i]) { + return false; + } + } + int remainder = bits % 8; + if (remainder == 0) { + return true; + } + int mask = (0xff << (8 - remainder)) & 0xff; + return (address[wholeBytes] & mask) == (network[wholeBytes] & mask); + } + + private static String wellKnownCategory(InetAddress address) { + if (address.isAnyLocalAddress()) { + return "wildcard address"; + } + if (address.isLoopbackAddress()) { + return "loopback address"; + } + if (address.isLinkLocalAddress()) { + return "link-local address"; + } + if (address.isSiteLocalAddress()) { + return "site-local address"; + } + if (address.isMulticastAddress()) { + return "multicast address"; + } + return null; + } + + /** + * Extract the IPv4 address tunnelled inside an IPv6 address, if there is one. Without this + * 2002:7f00:0001:: would reach 127.0.0.1. + */ + private static InetAddress embeddedIPv4(byte[] raw) { + if (raw.length != 16) { + return null; + } + + byte[] extracted = null; + if (isZero(raw, 10) && unsigned(raw[10]) == 0xff && unsigned(raw[11]) == 0xff) { + extracted = slice(raw, 12); // ::ffff:0:0/96, IPv4-mapped + } else if (isZero(raw, 8) && unsigned(raw[8]) == 0xff && unsigned(raw[9]) == 0xff + && raw[10] == 0 && raw[11] == 0) { + extracted = slice(raw, 12); // ::ffff:0:0:0/96, IPv4-translated + } else if (isZero(raw, 12)) { + extracted = slice(raw, 12); // ::/96, IPv4-compatible + } else if (unsigned(raw[0]) == 0x20 && unsigned(raw[1]) == 0x02) { + extracted = slice(raw, 2); // 2002::/16, 6to4 + } else if (unsigned(raw[0]) == 0x20 && unsigned(raw[1]) == 0x01 && raw[2] == 0 && raw[3] == 0) { + extracted = invert(slice(raw, 12)); // 2001::/32, Teredo stores it inverted + } else if (raw[0] == 0 && unsigned(raw[1]) == 0x64 && unsigned(raw[2]) == 0xff && unsigned(raw[3]) == 0x9b) { + extracted = slice(raw, 12); // 64:ff9b::/96, NAT64 + } + + if (extracted == null) { + return null; + } + try { + return InetAddress.getByAddress(extracted); + } catch (UnknownHostException e) { + return null; // unreachable: the array is always four bytes long + } + } + + private static boolean isZero(byte[] raw, int length) { + for (int i = 0; i < length; i++) { + if (raw[i] != 0) { + return false; + } + } + return true; + } + + private static byte[] slice(byte[] raw, int from) { + return Arrays.copyOfRange(raw, from, from + 4); + } + + private static byte[] invert(byte[] raw) { + for (int i = 0; i < raw.length; i++) { + raw[i] = (byte) ~raw[i]; + } + return raw; + } + + private static int unsigned(byte value) { + return value & 0xff; + } + + private static Range range(int addressLength, int bits, String label, int... network) { + byte[] bytes = new byte[addressLength]; + for (int i = 0; i < network.length; i++) { + bytes[i] = (byte) network[i]; + } + return new Range(bytes, bits, label); + } + + /** A network prefix held as raw bytes; only the leading {@code bits} are ever compared. */ + private static final class Range { + private final byte[] network; + private final int bits; + private final String label; + + private Range(byte[] network, int bits, String label) { + this.network = network; + this.bits = bits; + this.label = label; + } + + private boolean contains(byte[] address) { + return prefixMatches(address, network, bits); + } + } +} diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/HarvestPolicyAware.java b/dspace-api/src/main/java/org/dspace/harvest/ore/HarvestPolicyAware.java new file mode 100644 index 000000000000..d64c8b133097 --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/HarvestPolicyAware.java @@ -0,0 +1,21 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +/** + * Implemented by crosswalks that fetch remote content and therefore need the egress rules of the collection + * being harvested. A crosswalk that is never given a policy must fall back to + * {@link OreEgressPolicy#strictest(org.dspace.services.ConfigurationService)}. + */ +public interface HarvestPolicyAware { + + /** + * @param policy the rules to apply to remote fetches during this harvest + */ + void setOreEgressPolicy(OreEgressPolicy policy); +} diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/OreEgressPolicy.java b/dspace-api/src/main/java/org/dspace/harvest/ore/OreEgressPolicy.java new file mode 100644 index 000000000000..32db1ccc4c28 --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/OreEgressPolicy.java @@ -0,0 +1,139 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import org.apache.commons.lang3.StringUtils; +import org.dspace.harvest.HarvestedCollection; +import org.dspace.services.ConfigurationService; + +/** + * Immutable set of rules applied when the ORE ingest crosswalk fetches an aggregated resource. + *

+ * An aggregated file is confined to the host of the collection's oai_source, or one of the configured + * allowed prefixes; internal, private and reserved addresses stay blocked in either case. + */ +public final class OreEgressPolicy { + + static final String CONFIG_PREFIX = "oai.harvester.ore.file."; + + private final URI anchor; + private final boolean blockInternalAddresses; + private final List allowedUrlPrefix; + private final List allowedInternalHosts; + private final int maxRedirects; + private final int connectTimeoutMs; + private final int readTimeoutMs; + private final long maxBytes; + + private OreEgressPolicy(URI anchor, ConfigurationService configurationService) { + this.anchor = anchor; + this.blockInternalAddresses = + configurationService.getBooleanProperty(CONFIG_PREFIX + "blockInternalAddresses", true); + this.allowedUrlPrefix = trimmedList(configurationService.getArrayProperty(CONFIG_PREFIX + "allowedUrlPrefix")); + this.allowedInternalHosts = + trimmedList(configurationService.getArrayProperty(CONFIG_PREFIX + "allowedInternalHosts")); + this.maxRedirects = configurationService.getIntProperty(CONFIG_PREFIX + "maxRedirects", 3); + this.connectTimeoutMs = configurationService.getIntProperty(CONFIG_PREFIX + "connectTimeout", 10000); + this.readTimeoutMs = configurationService.getIntProperty(CONFIG_PREFIX + "readTimeout", 30000); + this.maxBytes = configurationService.getLongProperty(CONFIG_PREFIX + "maxBytes", 2147483648L); + } + + /** + * Policy for callers that have no harvest context, such as the AIP/METS/SWORD packagers and the XSLT CLI: + * no anchor, so only an explicit allowedUrlPrefix can satisfy the host check. + * + * @param configurationService the DSpace configuration + * @return the fail-closed policy + */ + public static OreEgressPolicy strictest(ConfigurationService configurationService) { + return new OreEgressPolicy(null, configurationService); + } + + /** + * Policy for a harvested collection. The trust anchor is the administrator-supplied oai_source, never a + * value read out of the harvested document. + * + * @param harvestRow the collection being harvested, may be null + * @param configurationService the DSpace configuration + * @return the policy to apply to that collection's ORE ingest + */ + public static OreEgressPolicy from(HarvestedCollection harvestRow, ConfigurationService configurationService) { + if (harvestRow == null) { + return strictest(configurationService); + } + return new OreEgressPolicy(toUri(harvestRow.getOaiSource()), configurationService); + } + + /** + * @return the oai_source this collection harvests from, or null when there is no harvest context or the + * configured value is not a usable absolute URL + */ + public URI getAnchor() { + return anchor; + } + + public boolean isBlockInternalAddresses() { + return blockInternalAddresses; + } + + public List getAllowedUrlPrefix() { + return allowedUrlPrefix; + } + + public List getAllowedInternalHosts() { + return allowedInternalHosts; + } + + public int getMaxRedirects() { + return maxRedirects; + } + + public int getConnectTimeoutMs() { + return connectTimeoutMs; + } + + public int getReadTimeoutMs() { + return readTimeoutMs; + } + + /** + * @return the response size cap in bytes, or a negative value for unlimited + */ + public long getMaxBytes() { + return maxBytes; + } + + private static URI toUri(String value) { + if (StringUtils.isBlank(value)) { + return null; + } + try { + URI uri = new URI(value.trim()); + return uri.isAbsolute() && uri.getHost() != null ? uri : null; + } catch (URISyntaxException e) { + return null; + } + } + + private static List trimmedList(String[] values) { + if (values == null || values.length == 0) { + return Collections.emptyList(); + } + return Arrays.stream(values) + .filter(StringUtils::isNotBlank) + .map(String::trim) + .collect(Collectors.toUnmodifiableList()); + } +} diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/OreResourceRejectedException.java b/dspace-api/src/main/java/org/dspace/harvest/ore/OreResourceRejectedException.java new file mode 100644 index 000000000000..25e16e439e5e --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/OreResourceRejectedException.java @@ -0,0 +1,51 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import org.dspace.content.crosswalk.CrosswalkException; + +/** + * Thrown when an ORE aggregated resource may not be fetched. It extends {@link CrosswalkException} so that + * it travels through the existing crosswalk signatures without changing them. + */ +public class OreResourceRejectedException extends CrosswalkException { + + private final RejectionReason reason; + private final String url; + + public OreResourceRejectedException(RejectionReason reason, String url, String detail) { + super(buildMessage(reason, url, detail)); + this.reason = reason; + this.url = url; + } + + public OreResourceRejectedException(RejectionReason reason, String url, String detail, Throwable cause) { + super(buildMessage(reason, url, detail), cause); + this.reason = reason; + this.url = url; + } + + public RejectionReason getReason() { + return reason; + } + + /** + * @return the URL that was refused; for a redirect chain this is the offending hop, not the original URL + */ + public String getUrl() { + return url; + } + + private static String buildMessage(RejectionReason reason, String url, String detail) { + StringBuilder message = new StringBuilder("ORE resource rejected [").append(reason).append("]: ").append(url); + if (detail != null && !detail.isEmpty()) { + message.append(" (").append(detail).append(')'); + } + return message.toString(); + } +} diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/OreUrlValidator.java b/dspace-api/src/main/java/org/dspace/harvest/ore/OreUrlValidator.java new file mode 100644 index 000000000000..07af92ee4ffd --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/OreUrlValidator.java @@ -0,0 +1,242 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import java.net.InetAddress; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.UnknownHostException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Locale; + +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +/** + * Decides whether a single ORE aggregated resource URL may be fetched. Checks are applied in a fixed order + * and the first violation ends the check, so an unexpected input always fails closed. + */ +public class OreUrlValidator { + + private static final Logger log = LogManager.getLogger(); + + private static final String HTTP = "http"; + private static final String HTTPS = "https"; + + private final BlockedAddressPredicate blockedAddresses = new BlockedAddressPredicate(); + + /** + * Turn an href taken from an ORE document into a URI, refusing anything that is not usable. + * + * @param url the raw href + * @return the parsed URI + * @throws OreResourceRejectedException if the value is not an absolute, parseable URL + */ + public static URI parse(String url) throws OreResourceRejectedException { + try { + URI uri = new URI(url); + if (!uri.isAbsolute()) { + throw new OreResourceRejectedException(RejectionReason.SCHEME_NOT_ALLOWED, url, "not absolute"); + } + return uri; + } catch (URISyntaxException e) { + throw new OreResourceRejectedException(RejectionReason.MALFORMED_AUTHORITY, url, "unparseable URL", e); + } + } + + /** + * Validate a URL against the egress policy. + * + * @param uri the URL to fetch, or a single redirect hop of it + * @param policy the rules to apply + * @return the decision, carrying the resolved addresses when the URL is allowed + */ + public Decision validate(URI uri, OreEgressPolicy policy) { + if (uri == null) { + return Decision.reject(null, RejectionReason.MALFORMED_AUTHORITY, "no URL"); + } + + // 1. only http(s); this is what keeps file:, jar:, ftp: and friends out + String scheme = lower(uri.getScheme()); + if (!HTTP.equals(scheme) && !HTTPS.equals(scheme)) { + return Decision.reject(uri, RejectionReason.SCHEME_NOT_ALLOWED, "scheme " + uri.getScheme()); + } + + // 2. no credentials in the authority + String authority = uri.getRawAuthority(); + if (uri.getRawUserInfo() != null || (authority != null && authority.indexOf('@') >= 0)) { + return Decision.reject(uri, RejectionReason.USERINFO_PRESENT, "credentials in authority"); + } + + // 3. a host we can actually resolve; upstream dereferences this without checking + String host = uri.getHost(); + if (StringUtils.isBlank(host) || host.indexOf('%') >= 0) { + return Decision.reject(uri, RejectionReason.MALFORMED_AUTHORITY, "authority " + authority); + } + + // 4. host confinement: the resource must share scheme+host with the oai_source or an allowed prefix + if (!isHostConfined(uri, scheme, host, policy)) { + return Decision.reject(uri, RejectionReason.HOST_NOT_ALLOWED, "host " + host); + } + + // 5. resolve now, so the addresses we vet are the ones we connect to + List addresses; + try { + addresses = Arrays.asList(InetAddress.getAllByName(host)); + } catch (UnknownHostException e) { + return Decision.reject(uri, RejectionReason.HOST_UNRESOLVABLE, "host " + host); + } + if (addresses.isEmpty()) { + return Decision.reject(uri, RejectionReason.HOST_UNRESOLVABLE, "host " + host); + } + + // 6. every returned address must pass unless it is explicitly exempted + if (policy.isBlockInternalAddresses() && !isHostExempt(host, policy)) { + for (InetAddress address : addresses) { + String range = blockedAddresses.matchedRange(address); + if (range != null && !isAddressExempt(address, policy)) { + return Decision.reject(uri, RejectionReason.ADDRESS_BLOCKED, range); + } + } + } + + return Decision.allow(uri, addresses); + } + + /** + * The resource must share scheme and host with the collection's oai_source, or match a globally allowed + * prefix. The port is deliberately not compared, so same-host-different-port setups keep working. + */ + private boolean isHostConfined(URI uri, String scheme, String host, OreEgressPolicy policy) { + URI anchor = policy.getAnchor(); + if (anchor != null && host.equalsIgnoreCase(anchor.getHost()) && scheme.equals(lower(anchor.getScheme()))) { + return true; + } + for (String prefix : policy.getAllowedUrlPrefix()) { + if (matchesAllowedPrefix(uri, host, prefix)) { + return true; + } + } + return false; + } + + private boolean matchesAllowedPrefix(URI uri, String host, String prefix) { + if (!prefix.contains("://")) { + return host.equalsIgnoreCase(prefix); // bare hostname, the form documented in oai.cfg + } + URI allowed; + try { + allowed = new URI(prefix); + } catch (URISyntaxException e) { + log.warn("Ignoring unparseable {}allowedUrlPrefix entry: {}", OreEgressPolicy.CONFIG_PREFIX, prefix); + return false; + } + if (allowed.getHost() == null || !host.equalsIgnoreCase(allowed.getHost()) + || !lower(uri.getScheme()).equals(lower(allowed.getScheme()))) { + return false; + } + // the host was compared exactly above, so the remaining prefix test cannot be widened by a lookalike host + String allowedPath = StringUtils.defaultString(allowed.getRawPath()); + return allowedPath.isEmpty() || "/".equals(allowedPath) + || StringUtils.defaultString(uri.getRawPath()).startsWith(allowedPath); + } + + private boolean isHostExempt(String host, OreEgressPolicy policy) { + for (String entry : policy.getAllowedInternalHosts()) { + if (entry.indexOf('/') < 0 && entry.equalsIgnoreCase(host)) { + return true; + } + } + return false; + } + + private boolean isAddressExempt(InetAddress address, OreEgressPolicy policy) { + for (String entry : policy.getAllowedInternalHosts()) { + if (entry.indexOf('/') >= 0 && cidrContains(entry, address)) { + return true; + } + } + return false; + } + + private boolean cidrContains(String cidr, InetAddress address) { + int slash = cidr.lastIndexOf('/'); + try { + // the network part comes from configuration, not from the harvested document + InetAddress network = InetAddress.getByName(cidr.substring(0, slash).trim()); + int bits = Integer.parseInt(cidr.substring(slash + 1).trim()); + return BlockedAddressPredicate.prefixMatches(address.getAddress(), network.getAddress(), bits); + } catch (UnknownHostException | NumberFormatException e) { + log.warn("Ignoring unparseable {}allowedInternalHosts entry: {}", OreEgressPolicy.CONFIG_PREFIX, cidr); + return false; + } + } + + private static String lower(String value) { + return value == null ? "" : value.toLowerCase(Locale.ROOT); + } + + /** + * The outcome of validating one URL. + */ + public static final class Decision { + + private final boolean allowed; + private final RejectionReason reason; + private final URI uri; + private final List addresses; + private final String detail; + + private Decision(boolean allowed, RejectionReason reason, URI uri, List addresses, + String detail) { + this.allowed = allowed; + this.reason = reason; + this.uri = uri; + this.addresses = addresses; + this.detail = detail; + } + + static Decision allow(URI uri, List addresses) { + return new Decision(true, null, uri, Collections.unmodifiableList(addresses), null); + } + + static Decision reject(URI uri, RejectionReason reason, String detail) { + return new Decision(false, reason, uri, Collections.emptyList(), detail); + } + + public boolean isAllowed() { + return allowed; + } + + public RejectionReason getReason() { + return reason; + } + + public URI getUri() { + return uri; + } + + /** + * @return every address the host resolved to, empty when the URL was rejected + */ + public List getAddresses() { + return addresses; + } + + /** + * @return a short explanation for the log; may name the blocked range, so keep it out of user-visible + * messages + */ + public String getDetail() { + return detail; + } + } +} diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/RejectionReason.java b/dspace-api/src/main/java/org/dspace/harvest/ore/RejectionReason.java new file mode 100644 index 000000000000..d135253ac046 --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/RejectionReason.java @@ -0,0 +1,34 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +/** + * Why an ORE aggregated resource was refused by the egress policy. + */ +public enum RejectionReason { + /** The URL scheme was neither http nor https. */ + SCHEME_NOT_ALLOWED, + /** The authority carried credentials. */ + USERINFO_PRESENT, + /** The authority could not be parsed into a usable host. */ + MALFORMED_AUTHORITY, + /** The host is not the one hosting the OAI source and is not explicitly allowed. */ + HOST_NOT_ALLOWED, + /** The host did not resolve to any address. */ + HOST_UNRESOLVABLE, + /** The host resolved to an internal, private or otherwise reserved address. */ + ADDRESS_BLOCKED, + /** The redirect chain exceeded the configured hop cap. */ + TOO_MANY_REDIRECTS, + /** A redirect moved the request from https to http. */ + REDIRECT_DOWNGRADE, + /** The response was larger than the configured byte cap. */ + RESPONSE_TOO_LARGE, + /** The remote server did not return a usable response. */ + FETCH_FAILED +} diff --git a/dspace-api/src/main/java/org/dspace/harvest/ore/SafeResourceFetcher.java b/dspace-api/src/main/java/org/dspace/harvest/ore/SafeResourceFetcher.java new file mode 100644 index 000000000000..93320b68e108 --- /dev/null +++ b/dspace-api/src/main/java/org/dspace/harvest/ore/SafeResourceFetcher.java @@ -0,0 +1,274 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.InetAddress; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.List; +import java.util.Locale; + +import org.apache.commons.lang3.StringUtils; +import org.apache.http.Header; +import org.apache.http.HttpEntity; +import org.apache.http.HttpHost; +import org.apache.http.HttpStatus; +import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.conn.ConnectTimeoutException; +import org.apache.http.conn.HttpHostConnectException; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.protocol.HTTP; +import org.dspace.app.client.DSpaceHttpClientFactory; + +/** + * Fetches an ORE aggregated resource under an {@link OreEgressPolicy}: redirects are followed by hand so + * that every hop is validated again with a fresh DNS lookup, the socket is pinned to the address that was + * validated, and the body is capped. + */ +public class SafeResourceFetcher { + + private static final String HTTPS = "https"; + + /** HttpClient 4.5 has no HttpStatus constant for 308. */ + private static final int SC_PERMANENT_REDIRECT = 308; + + private final OreUrlValidator validator = new OreUrlValidator(); + + /** + * Fetch a resource. + * + * @param url the resource to fetch + * @param policy the rules to apply, to this URL and to every redirect hop + * @return the response body; closing it releases the connection + * @throws OreResourceRejectedException if the URL, a redirect hop or the response is not acceptable + * @throws IOException if the transfer itself fails + */ + public InputStream fetch(URI url, OreEgressPolicy policy) throws OreResourceRejectedException, IOException { + CloseableHttpClient client = buildClient(policy); + boolean streamReturned = false; + try { + URI current = url; + int hops = 0; + while (true) { + OreUrlValidator.Decision decision = validator.validate(current, policy); + if (!decision.isAllowed()) { + throw new OreResourceRejectedException(decision.getReason(), current.toString(), + decision.getDetail()); + } + + CloseableHttpResponse response = execute(client, current, decision.getAddresses()); + boolean responseReturned = false; + try { + int status = response.getStatusLine().getStatusCode(); + if (isRedirect(status)) { + current = nextHop(current, response, policy, hops++); + continue; + } + InputStream body = openBody(current, response, policy); + responseReturned = true; + streamReturned = true; + return new CappedStream(body, response, client, policy.getMaxBytes()); + } finally { + if (!responseReturned) { + response.close(); + } + } + } + } finally { + if (!streamReturned) { + client.close(); + } + } + } + + private InputStream openBody(URI current, CloseableHttpResponse response, OreEgressPolicy policy) + throws OreResourceRejectedException, IOException { + int status = response.getStatusLine().getStatusCode(); + if (status != HttpStatus.SC_OK) { + throw new OreResourceRejectedException(RejectionReason.FETCH_FAILED, current.toString(), + "HTTP status " + status); + } + HttpEntity entity = response.getEntity(); + if (entity == null) { + throw new OreResourceRejectedException(RejectionReason.FETCH_FAILED, current.toString(), + "no response body"); + } + if (policy.getMaxBytes() >= 0 && entity.getContentLength() > policy.getMaxBytes()) { + throw new OreResourceRejectedException(RejectionReason.RESPONSE_TOO_LARGE, current.toString(), + "declared " + entity.getContentLength() + " bytes"); + } + return entity.getContent(); + } + + private CloseableHttpClient buildClient(OreEgressPolicy policy) { + RequestConfig requestConfig = RequestConfig.custom() + .setRedirectsEnabled(false) + .setConnectTimeout(policy.getConnectTimeoutMs()) + .setConnectionRequestTimeout(policy.getConnectTimeoutMs()) + .setSocketTimeout(policy.getReadTimeoutMs()) + .build(); + // compression is off so that a compressed body cannot expand past the byte cap; builder(true) keeps the + // configured egress proxy, which then owns the connect and therefore the address pinning + return DSpaceHttpClientFactory.getInstance().builder(true) + .setDefaultRequestConfig(requestConfig) + .disableContentCompression() + .disableAutomaticRetries() + .build(); + } + + /** + * Try the validated addresses in turn. The validator vetted every one of them, so failing over is + * security-neutral, and it keeps a round-robin DNS entry with one dead address working as it did before. + */ + private CloseableHttpResponse execute(CloseableHttpClient client, URI uri, List addresses) + throws IOException { + IOException lastFailure = null; + for (InetAddress address : addresses) { + try { + return client.execute(pinnedHost(uri, address), newRequest(uri)); + } catch (ConnectTimeoutException | HttpHostConnectException e) { + lastFailure = e; + } + } + // the list is never empty: the validator rejects a host that resolves to nothing + throw lastFailure != null ? lastFailure : new IOException("no address to connect to for " + uri.getHost()); + } + + private HttpGet newRequest(URI uri) { + HttpGet request = new HttpGet(requestTarget(uri)); + if (uri.getPort() <= 0) { + // pinning forces an explicit port onto the HttpHost, but the default port must stay out of Host: + // or presigned S3 and signed-CDN URLs fail their signature check + request.setHeader(HTTP.TARGET_HOST, uri.getHost()); + } + return request; + } + + /** + * Connect to the address that was validated, but keep the hostname so Host: and SNI still carry the name. + */ + private HttpHost pinnedHost(URI uri, InetAddress address) { + String scheme = lower(uri.getScheme()); + // the port has to be explicit: DefaultRoutePlanner rebuilds a portless HttpHost by name and would + // throw the pinned address away, leaving the connection to a second, unvalidated DNS lookup + int port = uri.getPort() > 0 ? uri.getPort() : (HTTPS.equals(scheme) ? 443 : 80); + return new HttpHost(address, uri.getHost(), port, scheme); + } + + private String requestTarget(URI uri) { + String path = StringUtils.defaultIfEmpty(uri.getRawPath(), "/"); + return uri.getRawQuery() == null ? path : path + "?" + uri.getRawQuery(); + } + + private URI nextHop(URI current, CloseableHttpResponse response, OreEgressPolicy policy, int hop) + throws OreResourceRejectedException { + if (hop >= policy.getMaxRedirects()) { + throw new OreResourceRejectedException(RejectionReason.TOO_MANY_REDIRECTS, current.toString(), + "more than " + policy.getMaxRedirects() + " hops"); + } + Header location = response.getFirstHeader("Location"); + if (location == null || StringUtils.isBlank(location.getValue())) { + throw new OreResourceRejectedException(RejectionReason.FETCH_FAILED, current.toString(), + "redirect without a Location header"); + } + URI next; + try { + next = current.resolve(new URI(location.getValue().trim())); + } catch (URISyntaxException | IllegalArgumentException e) { + throw new OreResourceRejectedException(RejectionReason.MALFORMED_AUTHORITY, current.toString(), + "unparseable Location header", e); + } + if (HTTPS.equals(lower(current.getScheme())) && !HTTPS.equals(lower(next.getScheme()))) { + throw new OreResourceRejectedException(RejectionReason.REDIRECT_DOWNGRADE, next.toString(), + "https redirected to " + next.getScheme()); + } + return next; + } + + private static boolean isRedirect(int status) { + return status == HttpStatus.SC_MOVED_PERMANENTLY || status == HttpStatus.SC_MOVED_TEMPORARILY + || status == HttpStatus.SC_SEE_OTHER || status == HttpStatus.SC_TEMPORARY_REDIRECT + || status == SC_PERMANENT_REDIRECT; + } + + private static String lower(String value) { + return value == null ? "" : value.toLowerCase(Locale.ROOT); + } + + /** + * Signals that the response outgrew the configured cap while it was being read. It has to be an + * {@link IOException} to be throwable from {@link InputStream#read()}. + */ + public static class ResponseTooLargeException extends IOException { + public ResponseTooLargeException(long maxBytes) { + super("ORE resource exceeded the configured maximum of " + maxBytes + " bytes"); + } + } + + /** + * Counts what is actually read rather than trusting Content-Length, and releases the connection and the + * client when it is closed. + */ + private static class CappedStream extends FilterInputStream { + + private final CloseableHttpResponse response; + private final CloseableHttpClient client; + private final long maxBytes; + private long consumed; + + CappedStream(InputStream in, CloseableHttpResponse response, CloseableHttpClient client, long maxBytes) { + super(in); + this.response = response; + this.client = client; + this.maxBytes = maxBytes; + } + + @Override + public int read() throws IOException { + int read = super.read(); + if (read >= 0) { + count(1); + } + return read; + } + + @Override + public int read(byte[] buffer, int offset, int length) throws IOException { + int read = super.read(buffer, offset, length); + if (read > 0) { + count(read); + } + return read; + } + + private void count(long bytes) throws IOException { + consumed += bytes; + if (maxBytes >= 0 && consumed > maxBytes) { + throw new ResponseTooLargeException(maxBytes); + } + } + + @Override + public void close() throws IOException { + try { + super.close(); + } finally { + try { + response.close(); + } finally { + client.close(); + } + } + } + } +} diff --git a/dspace-api/src/test/java/org/dspace/harvest/ore/BlockedAddressPredicateTest.java b/dspace-api/src/test/java/org/dspace/harvest/ore/BlockedAddressPredicateTest.java new file mode 100644 index 000000000000..85b06f94525a --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/harvest/ore/BlockedAddressPredicateTest.java @@ -0,0 +1,152 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.net.InetAddress; +import java.net.UnknownHostException; + +import org.junit.Test; + +/** + * Unit tests for {@link BlockedAddressPredicate}. Every address here is an IP literal, so no test performs a + * DNS lookup. + */ +public class BlockedAddressPredicateTest { + + private final BlockedAddressPredicate predicate = new BlockedAddressPredicate(); + + @Test + public void blocksLoopbackAndWildcardAddresses() throws Exception { + assertBlocked("127.0.0.1"); + assertBlocked("127.99.1.2"); + assertBlocked("0.0.0.0"); + assertBlocked("0.1.2.3"); + assertBlocked("::1"); + assertBlocked("::"); + } + + @Test + public void blocksPrivateIPv4Addresses() throws Exception { + assertBlocked("10.0.0.5"); + assertBlocked("172.16.0.9"); + assertBlocked("192.168.1.1"); + } + + @Test + public void blocksCloudMetadataAddresses() throws Exception { + // AWS, GCP and Azure all publish their instance metadata here + assertBlocked(InetAddress.getByAddress(bytes(169, 254, 169, 254))); + // Alibaba puts it in CGNAT space, which none of the java.net helpers report + assertBlocked("100.100.100.200"); + assertBlocked("100.64.0.1"); + } + + @Test + public void blocksReservedIPv4Ranges() throws Exception { + assertBlocked("192.0.0.170"); + assertBlocked("192.0.2.1"); + assertBlocked("198.51.100.1"); + assertBlocked("203.0.113.1"); + assertBlocked("192.88.99.1"); + assertBlocked("198.18.0.1"); + assertBlocked("240.0.0.1"); + assertBlocked("255.255.255.255"); + } + + @Test + public void blocksMulticastAddresses() throws Exception { + assertBlocked("224.0.0.1"); + assertBlocked("ff02::1"); + } + + @Test + public void blocksUniqueLocalIPv6Addresses() throws Exception { + InetAddress uniqueLocal = InetAddress.getByName("fd12:3456::1"); + // the gap that matters most: the JDK does not consider fc00::/7 site-local + assertFalse(uniqueLocal.isSiteLocalAddress()); + assertBlocked(uniqueLocal); + assertBlocked("fc00::1"); + } + + @Test + public void blocksReservedIPv6Ranges() throws Exception { + assertBlocked("2001:db8::1"); + assertBlocked("100::1"); + } + + @Test + public void blocksIPv4AddressesTunnelledInsideIPv6() throws Exception { + assertBlocked("::ffff:127.0.0.1"); + assertBlocked("2002:7f00:0001::"); + assertBlocked("64:ff9b::7f00:1"); + assertBlocked("2001:0:4136:e378:8000:63bf:3fff:fdd2"); + // ::ffff:0:0:0/96 (RFC 6052 IPv4-translated) carries the IPv4 address two bytes further along + assertBlocked("::ffff:0:7f00:1"); + assertBlocked("::ffff:0:a9fe:a9fe"); + } + + @Test + public void allowsPublicAddresses() throws Exception { + assertAllowed("8.8.8.8"); + assertAllowed("93.184.216.34"); + assertAllowed(InetAddress.getByAddress(bytes(1, 1, 1, 1))); + assertAllowed("2606:4700:4700::1111"); + assertAllowed("2a00:1450:4001:80e::200e"); + } + + @Test + public void namesTheRangeThatMatched() throws Exception { + assertNull(predicate.matchedRange(InetAddress.getByName("8.8.8.8"))); + assertNotNull(predicate.matchedRange(InetAddress.getByName("100.100.100.200"))); + assertNotNull("an unresolved address must fail closed", predicate.matchedRange(null)); + } + + @Test + public void comparesOnlyTheLeadingBitsOfAPrefix() throws Exception { + byte[] network = InetAddress.getByName("10.0.0.0").getAddress(); + assertTrue(BlockedAddressPredicate.prefixMatches(address("10.255.3.4"), network, 8)); + assertFalse(BlockedAddressPredicate.prefixMatches(address("11.0.0.1"), network, 8)); + assertTrue(BlockedAddressPredicate.prefixMatches(address("10.0.0.1"), network, 0)); + // a prefix can never match across address families + assertFalse(BlockedAddressPredicate.prefixMatches(address("::1"), network, 8)); + assertFalse(BlockedAddressPredicate.prefixMatches(address("10.255.3.4"), network, 99)); + } + + private void assertBlocked(String literal) throws UnknownHostException { + assertBlocked(InetAddress.getByName(literal)); + } + + private void assertBlocked(InetAddress address) { + assertTrue(address.getHostAddress() + " must be blocked", predicate.test(address)); + } + + private void assertAllowed(String literal) throws UnknownHostException { + assertAllowed(InetAddress.getByName(literal)); + } + + private void assertAllowed(InetAddress address) { + assertFalse(address.getHostAddress() + " must be allowed", predicate.test(address)); + } + + private byte[] address(String literal) throws UnknownHostException { + return InetAddress.getByName(literal).getAddress(); + } + + private static byte[] bytes(int... values) { + byte[] raw = new byte[values.length]; + for (int i = 0; i < values.length; i++) { + raw[i] = (byte) values[i]; + } + return raw; + } +} diff --git a/dspace-api/src/test/java/org/dspace/harvest/ore/OreUrlValidatorTest.java b/dspace-api/src/test/java/org/dspace/harvest/ore/OreUrlValidatorTest.java new file mode 100644 index 000000000000..d19e2cc8835f --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/harvest/ore/OreUrlValidatorTest.java @@ -0,0 +1,208 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.net.URI; +import java.util.HashMap; +import java.util.Map; + +import org.dspace.harvest.HarvestedCollection; +import org.dspace.services.ConfigurationService; +import org.junit.Test; + +/** + * Unit tests for {@link OreUrlValidator}. The policies are built by hand and every host is an IP literal, so + * no test needs Spring or a DNS lookup. + */ +public class OreUrlValidatorTest { + + private static final String PREFIX = "oai.harvester.ore.file."; + + /** A public host, standing in for the repository the collection harvests from. */ + private static final String OAI_SOURCE = "http://8.8.8.8/oai/request"; + + /** A different public host, standing in for the one an ORE record points at. */ + private static final String OTHER_HOST = "93.184.216.34"; + + private final OreUrlValidator validator = new OreUrlValidator(); + + private final Map config = new HashMap<>(); + + @Test + public void rejectsNonHttpSchemes() { + OreEgressPolicy policy = policy(OAI_SOURCE); + assertRejected(RejectionReason.SCHEME_NOT_ALLOWED, "file:///etc/passwd", policy); + assertRejected(RejectionReason.SCHEME_NOT_ALLOWED, "jar:file:///opt/dspace/lib/api.jar!/dspace.cfg", policy); + assertRejected(RejectionReason.SCHEME_NOT_ALLOWED, "ftp://8.8.8.8/private", policy); + assertRejected(RejectionReason.SCHEME_NOT_ALLOWED, "netdoc:///etc/passwd", policy); + } + + @Test + public void rejectsAnAuthorityWithoutAHost() { + // upstream dereferences this host without checking it and throws NPE + assertRejected(RejectionReason.MALFORMED_AUTHORITY, "http:///etc/passwd", policy(OAI_SOURCE)); + } + + @Test + public void rejectsCredentialsInTheAuthority() { + OreEgressPolicy policy = policy(OAI_SOURCE); + assertRejected(RejectionReason.USERINFO_PRESENT, "http://user:secret@8.8.8.8/x", policy); + // the "real" host of this one is the metadata service, not the one that reads like a hostname + assertRejected(RejectionReason.USERINFO_PRESENT, "http://8.8.8.8@169.254.169.254/latest/", policy); + } + + @Test + public void rejectsObfuscatedAddressForms() { + // the decimal and IPv6-embedded forms resolve to an internal address and must be caught there, so + // the policy is anchored on each host to let the address check be what rejects it + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://2130706433/", anchoredOn("2130706433")); + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://[::ffff:7f00:1]/", anchoredOn("[::ffff:7f00:1]")); + // these never parse into a usable host in the first place + assertRejected(RejectionReason.MALFORMED_AUTHORITY, "http://127.1/", anchoredOn("127.1")); + assertRejected(RejectionReason.MALFORMED_AUTHORITY, "http://0x7f.0x0.0x0.0x1/", anchoredOn("0x7f.0x0.0x0.0x1")); + } + + @Test + public void blocksADifferentHost() { + assertRejected(RejectionReason.HOST_NOT_ALLOWED, "http://" + OTHER_HOST + "/file.pdf", policy(OAI_SOURCE)); + } + + @Test + public void allowsTheOaiSourceHost() { + OreEgressPolicy policy = policy(OAI_SOURCE); + assertAllowed("http://8.8.8.8/bitstream/123/1/file.pdf", policy); + // the port is deliberately not part of the comparison + assertAllowed("http://8.8.8.8:8080/bitstream/123/1/file.pdf", policy); + // the scheme is compared lower-cased + assertAllowed("HTTP://8.8.8.8/bitstream/123/1/file.pdf", policy); + } + + @Test + public void blocksASchemeMismatchWithTheOaiSource() { + assertRejected(RejectionReason.HOST_NOT_ALLOWED, "http://8.8.8.8/file.pdf", + policy("https://8.8.8.8/oai/request")); + } + + @Test + public void allowsAHostListedInAllowedUrlPrefix() { + config.put(PREFIX + "allowedUrlPrefix", new String[] {"http://" + OTHER_HOST + "/files"}); + OreEgressPolicy policy = policy(null); + assertAllowed("http://" + OTHER_HOST + "/files/a.pdf", policy); + assertRejected(RejectionReason.HOST_NOT_ALLOWED, "http://" + OTHER_HOST + "/elsewhere/a.pdf", policy); + assertRejected(RejectionReason.HOST_NOT_ALLOWED, "https://" + OTHER_HOST + "/files/a.pdf", policy); + + // the bare hostname form documented in oai.cfg + config.put(PREFIX + "allowedUrlPrefix", new String[] {OTHER_HOST}); + assertAllowed("https://" + OTHER_HOST + "/anything", policy(null)); + } + + @Test + public void blocksInternalAddresses() { + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://169.254.169.254/latest/meta-data/iam/", + anchoredOn("169.254.169.254")); + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://100.100.100.200/latest/meta-data/", + anchoredOn("100.100.100.200")); + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://127.0.0.1:8983/solr/statistics/select?q=*:*", + anchoredOn("127.0.0.1")); + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://10.1.2.3/internal", anchoredOn("10.1.2.3")); + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://[fd12:3456::1]/internal", anchoredOn("[fd12:3456::1]")); + } + + @Test + public void allowsAnInternalHostOnlyWhenItIsListed() { + String solr = "http://127.0.0.1:8983/solr/statistics/select"; + assertRejected(RejectionReason.ADDRESS_BLOCKED, solr, anchoredOn("127.0.0.1")); + + config.put(PREFIX + "allowedInternalHosts", new String[] {"127.0.0.1"}); + assertAllowed(solr, anchoredOn("127.0.0.1")); + + // CIDR entries are matched against the resolved address instead of the host name + config.put(PREFIX + "allowedInternalHosts", new String[] {"10.0.0.0/8"}); + assertAllowed("http://10.1.2.3/internal", anchoredOn("10.1.2.3")); + assertRejected(RejectionReason.ADDRESS_BLOCKED, "http://192.168.1.1/internal", anchoredOn("192.168.1.1")); + } + + @Test + public void strictestPolicyRejectsAnExternalHost() { + assertRejected(RejectionReason.HOST_NOT_ALLOWED, "http://" + OTHER_HOST + "/file.pdf", + OreEgressPolicy.strictest(configuration())); + // a null harvest row must land on the same fail-closed policy + assertRejected(RejectionReason.HOST_NOT_ALLOWED, "http://" + OTHER_HOST + "/file.pdf", + OreEgressPolicy.from(null, configuration())); + } + + @Test + public void rejectsAMissingUrl() { + OreUrlValidator.Decision decision = validator.validate(null, policy(OAI_SOURCE)); + assertFalse(decision.isAllowed()); + assertEquals(RejectionReason.MALFORMED_AUTHORITY, decision.getReason()); + } + + @Test + public void parseRejectsRelativeAndUnparseableUrls() throws Exception { + assertEquals(URI.create("http://8.8.8.8/x"), OreUrlValidator.parse("http://8.8.8.8/x")); + + OreResourceRejectedException relative = + assertThrows(OreResourceRejectedException.class, () -> OreUrlValidator.parse("/bitstream/1")); + assertEquals(RejectionReason.SCHEME_NOT_ALLOWED, relative.getReason()); + + OreResourceRejectedException unparseable = + assertThrows(OreResourceRejectedException.class, () -> OreUrlValidator.parse("http://8.8.8.8/a b")); + assertEquals(RejectionReason.MALFORMED_AUTHORITY, unparseable.getReason()); + } + + private void assertRejected(RejectionReason expected, String url, OreEgressPolicy policy) { + OreUrlValidator.Decision decision = validator.validate(URI.create(url), policy); + assertFalse(url + " must be rejected", decision.isAllowed()); + assertEquals(url, expected, decision.getReason()); + } + + private void assertAllowed(String url, OreEgressPolicy policy) { + OreUrlValidator.Decision decision = validator.validate(URI.create(url), policy); + assertTrue(url + " must be allowed, was rejected as " + decision.getReason(), decision.isAllowed()); + assertFalse("an allowed URL must carry the addresses it was validated against", + decision.getAddresses().isEmpty()); + } + + /** A policy confined to the given oai_source. */ + private OreEgressPolicy policy(String oaiSource) { + HarvestedCollection harvestRow = mock(HarvestedCollection.class); + when(harvestRow.getOaiSource()).thenReturn(oaiSource); + return OreEgressPolicy.from(harvestRow, configuration()); + } + + /** A policy whose oai_source confines to exactly {@code host}, so the address checks are what a test exercises. */ + private OreEgressPolicy anchoredOn(String host) { + return policy("http://" + host + "/oai/request"); + } + + /** + * Answers with whatever default the caller passes in, so the tests run against the shipped defaults and + * only the entries a test puts in {@link #config} differ. + */ + private ConfigurationService configuration() { + return mock(ConfigurationService.class, invocation -> { + Object[] arguments = invocation.getArguments(); + if (arguments.length == 0) { + return null; + } + Object value = config.get(arguments[0]); + if (value != null) { + return value; + } + return arguments.length > 1 ? arguments[1] : null; + }); + } +} diff --git a/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java b/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java new file mode 100644 index 000000000000..5e543dd086d7 --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java @@ -0,0 +1,206 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.harvest.ore; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; + +import java.io.IOException; +import java.io.InputStream; +import java.net.InetAddress; +import java.net.URI; +import java.util.HashMap; +import java.util.Map; + +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import org.apache.http.config.Registry; +import org.apache.http.config.RegistryBuilder; +import org.apache.http.conn.socket.ConnectionSocketFactory; +import org.apache.http.conn.socket.PlainConnectionSocketFactory; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.dspace.app.client.DSpaceHttpClientFactory; +import org.dspace.services.ConfigurationService; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.MockedStatic; + +/** + * Unit tests for {@link SafeResourceFetcher}, driven against a {@link MockWebServer}. + */ +public class SafeResourceFetcherTest { + + private static final String PREFIX = "oai.harvester.ore.file."; + + private final SafeResourceFetcher fetcher = new SafeResourceFetcher(); + + private final Map config = new HashMap<>(); + + private MockWebServer server; + + private MockedStatic httpClientFactory; + + @Before + public void setUp() throws IOException { + // the host confinement is always on, so the test hosts have to be allowed explicitly for the address + // and redirect checks to be what the tests actually exercise + config.put(PREFIX + "allowedUrlPrefix", new String[] {"127.0.0.1", "169.254.169.254"}); + // MockWebServer answers on 127.0.0.1, which the predicate blocks, so the tests exempt it explicitly + config.put(PREFIX + "allowedInternalHosts", new String[] {"127.0.0.1"}); + server = new MockWebServer(); + server.start(InetAddress.getByName("127.0.0.1"), 0); + httpClientFactory = mockStatic(DSpaceHttpClientFactory.class); + httpClientFactory.when(() -> DSpaceHttpClientFactory.getInstance()).thenReturn(new PlaintextClientFactory()); + } + + @After + public void tearDown() throws IOException { + httpClientFactory.close(); + server.shutdown(); + } + + @Test + public void returnsTheBodyOfAnAllowedUrl() throws Exception { + server.enqueue(new MockResponse().setBody("aggregated bitstream")); + try (InputStream body = fetcher.fetch(url("/bitstreams/1"), policy())) { + assertEquals("aggregated bitstream", new String(body.readAllBytes(), UTF_8)); + } + RecordedRequest request = server.takeRequest(); + assertEquals("/bitstreams/1", request.getPath()); + } + + @Test + public void rejectsTheServerAddressWhenItIsNotExempted() { + config.remove(PREFIX + "allowedInternalHosts"); + OreResourceRejectedException rejected = + assertThrows(OreResourceRejectedException.class, () -> fetcher.fetch(url("/bitstreams/1"), policy())); + assertEquals(RejectionReason.ADDRESS_BLOCKED, rejected.getReason()); + assertEquals(0, server.getRequestCount()); + } + + @Test + public void rejectsARedirectToTheMetadataService() { + String metadata = "http://169.254.169.254/latest/meta-data/iam/security-credentials/"; + server.enqueue(new MockResponse().setResponseCode(302).addHeader("Location", metadata)); + OreResourceRejectedException rejected = + assertThrows(OreResourceRejectedException.class, () -> fetcher.fetch(url("/ore"), policy())); + assertEquals(RejectionReason.ADDRESS_BLOCKED, rejected.getReason()); + assertEquals(metadata, rejected.getUrl()); + // the hop is judged before it is fetched, so nothing was ever sent to the metadata service + assertEquals(1, server.getRequestCount()); + } + + @Test + public void rejectsARedirectChainOverTheCap() { + config.put(PREFIX + "maxRedirects", 2); + for (int hop = 0; hop < 3; hop++) { + server.enqueue(new MockResponse().setResponseCode(302).addHeader("Location", "/hop" + hop)); + } + OreResourceRejectedException rejected = + assertThrows(OreResourceRejectedException.class, () -> fetcher.fetch(url("/ore"), policy())); + assertEquals(RejectionReason.TOO_MANY_REDIRECTS, rejected.getReason()); + assertEquals(3, server.getRequestCount()); + } + + @Test + public void refusesAnHttpsToHttpDowngrade() { + String downgraded = "http://127.0.0.1:" + server.getPort() + "/plain"; + server.enqueue(new MockResponse().setResponseCode(301).addHeader("Location", downgraded)); + URI secure = URI.create("https://127.0.0.1:" + server.getPort() + "/secure"); + OreResourceRejectedException rejected = + assertThrows(OreResourceRejectedException.class, () -> fetcher.fetch(secure, policy())); + assertEquals(RejectionReason.REDIRECT_DOWNGRADE, rejected.getReason()); + assertEquals(downgraded, rejected.getUrl()); + assertEquals(1, server.getRequestCount()); + } + + @Test + public void rejectsADeclaredLengthOverTheCap() { + config.put(PREFIX + "maxBytes", 16L); + server.enqueue(new MockResponse().setBody(body(4096))); + OreResourceRejectedException rejected = + assertThrows(OreResourceRejectedException.class, () -> fetcher.fetch(url("/big"), policy())); + assertEquals(RejectionReason.RESPONSE_TOO_LARGE, rejected.getReason()); + } + + @Test + public void rejectsABodyThatOutgrowsTheCapWhileStreaming() throws Exception { + config.put(PREFIX + "maxBytes", 16L); + server.enqueue(new MockResponse().setChunkedBody(body(4096), 512)); + try (InputStream body = fetcher.fetch(url("/big"), policy())) { + assertThrows(SafeResourceFetcher.ResponseTooLargeException.class, () -> body.readAllBytes()); + } + } + + @Test + public void rejectsABodyThatLiesAboutItsLength() throws Exception { + config.put(PREFIX + "maxBytes", 16L); + // chunked framing wins over the header, so the declared length never gets the chance to be trusted + server.enqueue(new MockResponse().setChunkedBody(body(4096), 512).setHeader("Content-Length", "4")); + try (InputStream body = fetcher.fetch(url("/liar"), policy())) { + assertThrows(SafeResourceFetcher.ResponseTooLargeException.class, () -> body.readAllBytes()); + } + } + + private URI url(String path) { + return URI.create("http://127.0.0.1:" + server.getPort() + path); + } + + private static String body(int length) { + return "x".repeat(length); + } + + /** + * No harvest context, so the address checks have to hold on their own; the test hosts are permitted + * through allowedUrlPrefix in {@link #setUp()}. + */ + private OreEgressPolicy policy() { + return OreEgressPolicy.strictest(configuration()); + } + + /** + * Answers with whatever default the caller passes in, so the tests run against the shipped defaults and + * only the entries a test puts in {@link #config} differ. + */ + private ConfigurationService configuration() { + return mock(ConfigurationService.class, invocation -> { + Object[] arguments = invocation.getArguments(); + if (arguments.length == 0) { + return null; + } + Object value = config.get(arguments[0]); + if (value != null) { + return value; + } + return arguments.length > 1 ? arguments[1] : null; + }); + } + + /** + * Stands in for the Spring-managed factory. It also serves https over a plaintext socket, so the redirect + * rules can be exercised against MockWebServer without a test certificate authority. + */ + private static class PlaintextClientFactory extends DSpaceHttpClientFactory { + + @Override + public HttpClientBuilder builder(boolean setProxy) { + Registry plaintext = RegistryBuilder.create() + .register("http", PlainConnectionSocketFactory.getSocketFactory()) + .register("https", PlainConnectionSocketFactory.getSocketFactory()) + .build(); + return HttpClients.custom().setConnectionManager(new PoolingHttpClientConnectionManager(plaintext)); + } + } +} diff --git a/dspace/config/modules/oai.cfg b/dspace/config/modules/oai.cfg index 10409cd10e82..ad8e828ac077 100644 --- a/dspace/config/modules/oai.cfg +++ b/dspace/config/modules/oai.cfg @@ -153,9 +153,39 @@ oai.harvester.unknownSchema = fail # this config parameter, a new handle will be minted instead. Default value: 123456789. #oai.harvester.rejectedHandlePrefix = 123456789, myTestHandle -# If ingesting files with ORE, only files with URLs that match the base URL of the remote -# OAI endpoint's domain name are accepted, or a list of other URL prefixes defined below -#oai.harvester.ore.file.validateUrlPrefix = true -# Prefixes that are allowed globally (for any endpoint) are below +# If ingesting files with ORE (harvest type "metadata and bitstreams"), each aggregated file URL +# must share scheme+host with the collection's remote OAI endpoint (its oai_source), or match one +# of the allowedUrlPrefix values below. This host confinement is always enforced and http(s) is the +# only permitted scheme. A prefix is either a bare hostname (any path on that host over any scheme) +# or a scheme://host/path prefix. Repeatable. Default: empty. #oai.harvester.ore.file.allowedUrlPrefix = dspace.myinstitution.edu -#oai.harvester.ore.file.allowedUrlPrefix = files.myinstitution.edu +#oai.harvester.ore.file.allowedUrlPrefix = https://files.myinstitution.edu/bitstreams +# The METS/SWORD packagers and the XSLT CLI have no collection and therefore no oai_source, so +# allowedUrlPrefix above is the ONLY way to let them fetch ORE files, and it ships empty. This bites +# only for a package that declares OTHERMDTYPE="ore"; DSpace's own AIP disseminator does not emit one. + +# Block URLs resolving to loopback, private, link-local, CGNAT, reserved or cloud-metadata +# addresses. Checked at every redirect hop. Default: true. +# Setting this to false restores the server-side request forgery this check exists to prevent: a +# remote OAI source can then make DSpace fetch any internal host and republish the response. +#oai.harvester.ore.file.blockInternalAddresses = true + +# Hostnames or CIDRs exempted from the check above - the only supported way to harvest files +# from an internal repository. Repeatable. Default: empty. +#oai.harvester.ore.file.allowedInternalHosts = repository.internal.myinstitution.edu +#oai.harvester.ore.file.allowedInternalHosts = 10.1.2.0/24 + +# NOTE: with http.proxy.host configured (dspace.cfg) these ORE fetches go through the proxy, which +# resolves the hostname itself and connects on our behalf. The address checks are then advisory +# only - they describe what DSpace resolved, not what was connected to - so the proxy has to +# enforce its own egress rules. + +# Maximum number of redirect hops; each hop is re-validated in full. Default: 3. +#oai.harvester.ore.file.maxRedirects = 3 + +# Connect and read timeouts for fetching an ORE file, in milliseconds. Defaults: 10000 / 30000. +#oai.harvester.ore.file.connectTimeout = 10000 +#oai.harvester.ore.file.readTimeout = 30000 + +# Maximum size of a single fetched ORE file in bytes; -1 means unlimited. Default: 2 GiB. +#oai.harvester.ore.file.maxBytes = 2147483648 From e6b3d3292580234a08adc2ca1c551585a4944d2f Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Tue, 27 Oct 2026 16:04:11 +0100 Subject: [PATCH 2/3] Add DNS-pinning test and redirect note for ORE egress guard Prove the ORE fetch connects to the address the validator vetted and does not resolve the host a second time: a resolver wired into the client's connection manager records every lookup, and the fetch must reach the server with no lookup recorded. This guards the socket pinning (an explicit address and port on the HttpHost) that defeats DNS rebinding between check and connect. Also note in oai.cfg that redirects are re-confined per hop, so a file URL that redirects to a different host must have that host in allowedUrlPrefix. --- .../harvest/ore/SafeResourceFetcherTest.java | 69 +++++++++++++++++++ dspace/config/modules/oai.cfg | 2 + 2 files changed, 71 insertions(+) diff --git a/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java b/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java index 5e543dd086d7..e494995d2762 100644 --- a/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java +++ b/dspace-api/src/test/java/org/dspace/harvest/ore/SafeResourceFetcherTest.java @@ -10,6 +10,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; @@ -17,14 +18,20 @@ import java.io.InputStream; import java.net.InetAddress; import java.net.URI; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; import org.apache.http.config.Registry; import org.apache.http.config.RegistryBuilder; +import org.apache.http.conn.DnsResolver; import org.apache.http.conn.socket.ConnectionSocketFactory; import org.apache.http.conn.socket.PlainConnectionSocketFactory; import org.apache.http.impl.client.HttpClientBuilder; @@ -154,6 +161,25 @@ public void rejectsABodyThatLiesAboutItsLength() throws Exception { } } + @Test + public void pinsTheConnectionToTheValidatedAddressAndDoesNotReResolve() throws Exception { + // the validator vets the host once; the connection must then target that exact address rather than + // look the host up a second time, which is what defeats DNS rebinding between check and connect + RecordingDnsResolver dns = new RecordingDnsResolver(); + httpClientFactory.when(() -> DSpaceHttpClientFactory.getInstance()) + .thenReturn(new PinningProbeClientFactory(dns)); + server.enqueue(new MockResponse().setBody("pinned")); + + try (InputStream body = fetcher.fetch(url("/pin"), policy())) { + assertEquals("pinned", new String(body.readAllBytes(), UTF_8)); + } + + // a regression such as dropping the explicit port in pinnedHost makes DefaultRoutePlanner rebuild the + // target by name, which sends the connect back through the resolver; a pinned address never does + assertTrue("the connection must not re-resolve the pinned host, but it looked up " + dns.resolvedHosts(), + dns.resolvedHosts().isEmpty()); + } + private URI url(String path) { return URI.create("http://127.0.0.1:" + server.getPort() + path); } @@ -203,4 +229,47 @@ public HttpClientBuilder builder(boolean setProxy) { return HttpClients.custom().setConnectionManager(new PoolingHttpClientConnectionManager(plaintext)); } } + + /** + * Like {@link PlaintextClientFactory}, but wires a resolver into the connection manager that records every + * host the client asks it to look up, so a test can prove the connect bypassed it. + */ + private static class PinningProbeClientFactory extends DSpaceHttpClientFactory { + + private final DnsResolver dnsResolver; + + PinningProbeClientFactory(DnsResolver dnsResolver) { + this.dnsResolver = dnsResolver; + } + + @Override + public HttpClientBuilder builder(boolean setProxy) { + Registry plaintext = RegistryBuilder.create() + .register("http", PlainConnectionSocketFactory.getSocketFactory()) + .register("https", PlainConnectionSocketFactory.getSocketFactory()) + .build(); + PoolingHttpClientConnectionManager manager = + new PoolingHttpClientConnectionManager(plaintext, null, null, dnsResolver, -1, TimeUnit.MILLISECONDS); + return HttpClients.custom().setConnectionManager(manager); + } + } + + /** + * Records the hosts it is asked to resolve. It still answers with loopback so that, if a regression does + * route a connect through here, the test fails on the recorded lookup rather than on a connection error. + */ + private static class RecordingDnsResolver implements DnsResolver { + + private final List hosts = Collections.synchronizedList(new ArrayList<>()); + + @Override + public InetAddress[] resolve(String host) throws UnknownHostException { + hosts.add(host); + return new InetAddress[] {InetAddress.getByName("127.0.0.1")}; + } + + List resolvedHosts() { + return hosts; + } + } } diff --git a/dspace/config/modules/oai.cfg b/dspace/config/modules/oai.cfg index ad8e828ac077..ac7ab2ffb011 100644 --- a/dspace/config/modules/oai.cfg +++ b/dspace/config/modules/oai.cfg @@ -158,6 +158,8 @@ oai.harvester.unknownSchema = fail # of the allowedUrlPrefix values below. This host confinement is always enforced and http(s) is the # only permitted scheme. A prefix is either a bare hostname (any path on that host over any scheme) # or a scheme://host/path prefix. Repeatable. Default: empty. +# Redirects are re-confined at every hop, so a file URL that redirects to a different host (a CDN or a +# presigned S3 bucket, say) is blocked unless that host is also listed here. #oai.harvester.ore.file.allowedUrlPrefix = dspace.myinstitution.edu #oai.harvester.ore.file.allowedUrlPrefix = https://files.myinstitution.edu/bitstreams # The METS/SWORD packagers and the XSLT CLI have no collection and therefore no oai_source, so From 6eb063381cfcaf15e2379ee9898c2b4fe3cc6d95 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 28 Oct 2026 12:02:56 +0100 Subject: [PATCH 3/3] Address review: redact rejected URLs, close fetched stream, pre-validate ORE resources - Redact the query string and userinfo from a rejected ORE URL before logging it in rejectRecord: a rejected presigned S3/CDN URL can carry reusable credentials there. - Close the fetched stream (and the HTTP connection/client it holds) on every path via try-with-resources, including when bitstream creation fails part way through. - Validate every aggregated resource URL up front, before any bitstream is created, so a URL rejected by the egress policy cannot leave the bitstreams of earlier resources in the same record orphaned in the assetstore after the record is rolled back. --- .../crosswalk/OREIngestionCrosswalk.java | 86 ++++++++++--------- .../java/org/dspace/harvest/OAIHarvester.java | 38 +++++++- 2 files changed, 81 insertions(+), 43 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java b/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java index e90d7a1650fb..d997ddf932d5 100644 --- a/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java +++ b/dspace-api/src/main/java/org/dspace/content/crosswalk/OREIngestionCrosswalk.java @@ -154,6 +154,23 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea Filters.attribute(), null, ATOM_NS); entryId = xpathAltHref.evaluateFirst(doc).getValue(); + // Validate every aggregated resource URL up front, before any bitstream is created, so a URL rejected + // by the egress policy cannot leave the bitstreams of earlier resources orphaned in the assetstore + // when the record is rolled back. The fetch below still re-validates each URL (and every redirect hop) + // to guard against TOCTOU. + OreUrlValidator preValidator = new OreUrlValidator(); + for (Element resource : aggregatedResources) { + String href = resource.getAttributeValue("href"); + if (href == null) { + throw new CrosswalkException("Entry did not contain link to resource: " + entryId); + } + OreUrlValidator.Decision decision = + preValidator.validate(OreUrlValidator.parse(encodeForURL(href)), egressPolicy()); + if (!decision.isAllowed()) { + throw new OreResourceRejectedException(decision.getReason(), href, decision.getDetail()); + } + } + // Next for each resource, create a bitstream NumberFormat nf = NumberFormat.getInstance(); nf.setGroupingUsed(false); @@ -193,51 +210,36 @@ public void ingest(Context context, DSpaceObject dso, Element root, boolean crea targetBundle = targetBundles.get(0); } - InputStream in = null; - if (href != null) { - try { - // Make sure the url string escapes all the oddball characters - String processedURL = encodeForURL(href); - // The trust anchor of the egress policy is the collection's oai_source, not entryId: - // entryId is read from this remote document and is therefore attacker-controlled. - in = resourceFetcher.fetch(OreUrlValidator.parse(processedURL), egressPolicy()); - } catch (IOException ioe) { - // a transport failure has to drop this record only; escaping here would stop the whole - // collection, and after removeAllBundles it would commit the item without its files - throw transferFailed(href, ioe); - } - } else { - throw new CrosswalkException("Entry did not contain link to resource: " + entryId); + // Make sure the url string escapes all the oddball characters + String processedURL = encodeForURL(href); + Bitstream newBitstream; + // try-with-resources so the fetched stream - and the underlying HTTP connection/client it holds - + // is always closed, including when bitstream creation fails part way through. + // The trust anchor of the egress policy is the collection's oai_source, not entryId: entryId is + // read from this remote document and is therefore attacker-controlled. + try (InputStream in = resourceFetcher.fetch(OreUrlValidator.parse(processedURL), egressPolicy())) { + newBitstream = bitstreamService.create(context, targetBundle, in); + } catch (IOException ioe) { + // a transport failure - or the size cap, which the bitstore surfaces here wrapped in an + // IOException - has to drop this record only; escaping would stop the whole collection and, + // after removeAllBundles, commit the item without its files + throw transferFailed(href, ioe); } - // ingest and update - if (in != null) { - Bitstream newBitstream; - try { - newBitstream = bitstreamService.create(context, targetBundle, in); - } catch (IOException ioe) { - // the body is only read here, and the bitstore wraps everything it catches in a plain - // IOException, so both the size cap and a mid-transfer failure surface at this point - throw transferFailed(href, ioe); - } - - String bsName = resource.getAttributeValue("title"); - newBitstream.setName(context, bsName); - - // Identify the format - String mimeString = resource.getAttributeValue("type"); - BitstreamFormat bsFormat = bitstreamFormatService.findByMIMEType(context, mimeString); - if (bsFormat == null) { - bsFormat = bitstreamFormatService.guessFormat(context, newBitstream); - } - newBitstream.setFormat(context, bsFormat); - bitstreamService.update(context, newBitstream); - - bundleService.addBitstream(context, targetBundle, newBitstream); - bundleService.update(context, targetBundle); - } else { - throw new CrosswalkException("Could not retrieve bitstream: " + entryId); + String bsName = resource.getAttributeValue("title"); + newBitstream.setName(context, bsName); + + // Identify the format + String mimeString = resource.getAttributeValue("type"); + BitstreamFormat bsFormat = bitstreamFormatService.findByMIMEType(context, mimeString); + if (bsFormat == null) { + bsFormat = bitstreamFormatService.guessFormat(context, newBitstream); } + newBitstream.setFormat(context, bsFormat); + bitstreamService.update(context, newBitstream); + + bundleService.addBitstream(context, targetBundle, newBitstream); + bundleService.update(context, targetBundle); } log.info( diff --git a/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java b/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java index 0b6ba51d11d5..5945fd307927 100644 --- a/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java +++ b/dspace-api/src/main/java/org/dspace/harvest/OAIHarvester.java @@ -13,6 +13,8 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.net.ConnectException; +import java.net.URI; +import java.net.URISyntaxException; import java.sql.SQLException; import java.text.SimpleDateFormat; import java.util.ArrayList; @@ -524,12 +526,46 @@ protected void rejectRecord(String itemOaiID, OreResourceRejectedException ore) rejectedRecords++; } log.warn("Skipping record {} for collection {}: ORE file {} harvested from {} was rejected ({})", - itemOaiID, targetCollection.getID(), ore.getUrl(), harvestRow.getOaiSource(), ore.getReason()); + itemOaiID, targetCollection.getID(), redactUrl(ore.getUrl()), harvestRow.getOaiSource(), + ore.getReason()); ourContext.restoreAuthSystemState(); ourContext.rollback(); reloadRequiredEntities(); } + /** + * Strip the query string and any userinfo from a rejected URL before logging it: a rejected presigned + * S3/CDN URL can carry reusable credentials in those parts, which must not leak into the logs. + * + * @param url the rejected URL + * @return scheme, host, port and path only, or a placeholder when the URL cannot be parsed + */ + private static String redactUrl(String url) { + if (url == null) { + return ""; + } + try { + URI uri = new URI(url); + if (uri.getHost() != null) { + StringBuilder redacted = new StringBuilder(); + if (uri.getScheme() != null) { + redacted.append(uri.getScheme()).append("://"); + } + redacted.append(uri.getHost()); + if (uri.getPort() > 0) { + redacted.append(':').append(uri.getPort()); + } + if (uri.getRawPath() != null) { + redacted.append(uri.getRawPath()); + } + return redacted.toString(); + } + } catch (URISyntaxException e) { + // fall through to the placeholder + } + return ""; + } + /** * Process an individual PMH record, making (or updating) a corresponding DSpace Item. *