diff --git a/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java b/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java index ad0549111..75b79924e 100644 --- a/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java +++ b/external/warc/src/main/java/org/apache/stormcrawler/warc/MetadataRecordFormat.java @@ -17,6 +17,7 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.List; import org.apache.commons.lang3.StringUtils; import org.apache.storm.tuple.Tuple; @@ -46,14 +47,26 @@ public class MetadataRecordFormat extends WARCRecordFormat { - private static final Logger LOG = LoggerFactory.getLogger(WARCRequestRecordFormat.class); + private static final Logger LOG = LoggerFactory.getLogger(MetadataRecordFormat.class); - private List metadataKeys; + private final List metadataKeys; public MetadataRecordFormat(List metadataKeys) { super(""); - this.metadataKeys = metadataKeys; - LOG.info("MetadataRecordFormat instantiated with {}", String.join(",", metadataKeys)); + // the keys are fixed configuration: validate them once here instead of + // for every record + final List validKeys = new ArrayList<>(metadataKeys.size()); + for (String key : metadataKeys) { + if (isValidWarcFieldName(key)) { + validKeys.add(key); + } else { + LOG.warn( + "Skipping invalid WARC field name configured in warc.metadata.keys: {}", + key); + } + } + this.metadataKeys = List.copyOf(validKeys); + LOG.info("MetadataRecordFormat instantiated with {}", String.join(",", this.metadataKeys)); } @Override @@ -81,7 +94,9 @@ public byte[] format(Tuple tuple) { if (StringUtils.isBlank(value)) { continue; } - payload.append(key).append(": ").append(value).append(CRLF); + // metadata values often originate from the parsed content: replace CR and + // LF so that the value cannot forge additional field lines + payload.append(key).append(": ").append(sanitizeWarcFieldValue(value)).append(CRLF); } } diff --git a/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java b/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java index cee46c8fb..ad8f2d46a 100644 --- a/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java +++ b/external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java @@ -89,6 +89,42 @@ public class WARCRecordFormat implements RecordFormat { protected static final Pattern HTTP_STATUS_CODE_PATTERN = Pattern.compile("^[0-9]{3}$"); protected static final String HTTP_VERSION_FALLBACK = "HTTP/1.1"; + /* + * Named fields (WARC header fields and the fields of an application/warc-fields payload) are + * terminated by CRLF: their names are limited to printable ASCII characters excluding the + * colon, cf. RFC 5322 section 2.2, and their values must not contain CR or LF, otherwise the + * remainder of the value would be read as additional field lines. + */ + private static final Pattern WARC_FIELD_NAME_PATTERN = Pattern.compile("[!-9;-~]+"); + + /** + * Check whether a string is a valid WARC field name, i.e. consists of printable ASCII + * characters without a colon, cf. RFC 5322 section 2.2. + * + * @param name field name candidate + * @return true if the name can safely be written as the name of a WARC field + */ + static boolean isValidWarcFieldName(String name) { + if (name == null) { + return false; + } + return WARC_FIELD_NAME_PATTERN.matcher(name).matches(); + } + + /** + * Replace CR and LF characters in a field value by spaces so that the value cannot forge + * additional field lines in a WARC header block or in an application/warc-fields payload. + * + * @param value field value to sanitise + * @return the value without CR and LF characters + */ + static String sanitizeWarcFieldValue(String value) { + if (value == null || (value.indexOf('\r') < 0 && value.indexOf('\n') < 0)) { + return value; + } + return value.replace('\r', ' ').replace('\n', ' '); + } + protected static final Pattern PROBLEMATIC_HEADERS = Pattern.compile("(?i)(?:Content-(?:Encoding|Length)|Transfer-Encoding)"); protected static final String X_HIDE_HEADER = "X-Crawler-"; @@ -445,7 +481,9 @@ public byte[] format(Tuple tuple) { if (StringUtils.isBlank(ct)) { ct = "application/octet-stream"; } - buffer.append("Content-Type: ").append(ct).append(CRLF); + // the content type is under the control of the remote server: replace CR and LF + // so that it cannot forge additional header lines in the WARC header block + buffer.append("Content-Type: ").append(sanitizeWarcFieldValue(ct)).append(CRLF); } String truncated = diff --git a/external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java b/external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java new file mode 100644 index 000000000..92430dc11 --- /dev/null +++ b/external/warc/src/test/java/org/apache/stormcrawler/warc/MetadataRecordFormatCRLFTest.java @@ -0,0 +1,119 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.stormcrawler.warc; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import org.apache.storm.tuple.Tuple; +import org.apache.stormcrawler.Metadata; +import org.junit.jupiter.api.Test; +import org.netpreserve.jwarc.MessageHeaders; +import org.netpreserve.jwarc.WarcMetadata; +import org.netpreserve.jwarc.WarcReader; +import org.netpreserve.jwarc.WarcRecord; + +/** + * Metadata values, e.g. feed descriptions or values extracted from the parsed content, may contain + * CR and LF characters. Written verbatim into the application/warc-fields payload of a WARC + * metadata record, a CR LF sequence would end the field line and the remainder of the value would + * look exactly like an additional field line generated by the crawler. + */ +class MetadataRecordFormatCRLFTest { + + private byte[] record(String key, String value) { + Metadata metadata = new Metadata(); + metadata.addValue(key, value); + Tuple tuple = mock(Tuple.class); + when(tuple.getStringByField("url")).thenReturn("https://www.example.org/"); + when(tuple.getValueByField("metadata")).thenReturn(metadata); + MetadataRecordFormat format = new MetadataRecordFormat(List.of(key)); + return format.format(tuple); + } + + private static List parseFields(byte[] warcBytes) { + List parsed = new ArrayList<>(); + try (WarcReader reader = new WarcReader(new ByteArrayInputStream(warcBytes))) { + for (WarcRecord rec : reader) { + assertTrue(rec instanceof WarcMetadata, "Can't parse as WARC metadata record"); + parsed.add(((WarcMetadata) rec).fields()); + } + } catch (IOException e) { + throw new RuntimeException(e); + } + return parsed; + } + + @Test + void valueWithCRLFDoesNotCreateExtraFields() { + byte[] warcBytes = record("feed.description", "some text\r\nhopsFromSeed: 1"); + String warcString = new String(warcBytes, StandardCharsets.UTF_8); + assertFalse( + warcString.contains("\r\nhopsFromSeed: 1\r\n"), + "a metadata value must not introduce a new warc-fields line"); + // CR and LF are replaced by spaces, the value remains on its own field line + assertTrue( + warcString.contains("feed.description: some text hopsFromSeed: 1\r\n"), + "the sanitised metadata value is expected on a single field line"); + + List records = parseFields(warcBytes); + assertEquals(1, records.size(), "expected a single WARC metadata record"); + MessageHeaders fields = records.get(0); + assertEquals( + List.of("some text hopsFromSeed: 1"), + fields.all("feed.description"), + "the sanitised metadata value must be kept on a single field line"); + assertFalse( + fields.contains("hopsFromSeed", "1"), + "parsers must not see a field the crawler did not write"); + } + + @Test + void valueWithBareLineBreaksDoesNotCreateExtraFields() { + for (String value : List.of("some text\nhopsFromSeed: 1", "some text\rhopsFromSeed: 1")) { + byte[] warcBytes = record("feed.description", value); + String warcString = new String(warcBytes, StandardCharsets.UTF_8); + assertFalse( + warcString.contains("\r\nhopsFromSeed: 1\r\n"), + "a metadata value must not introduce a new warc-fields line"); + for (MessageHeaders fields : parseFields(warcBytes)) { + assertFalse( + fields.contains("hopsFromSeed", "1"), + "parsers must not see a field the crawler did not write"); + } + } + } + + @Test + void invalidFieldNamesAreDropped() { + // a colon or a space makes the key unusable as WARC field name + for (String key : List.of("hopsFromSeed: 1", "hops FromSeed")) { + byte[] warcBytes = record(key, "any value"); + assertEquals( + 0, warcBytes.length, "a record with an invalid field name must not be written"); + } + } +} diff --git a/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java b/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java index 045579a89..d834d8ffd 100644 --- a/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java +++ b/external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -123,6 +124,30 @@ void testCanonicalizeIpAddress() { } } + @Test + void testWarcFieldNameValidation() { + assertFalse(WARCRecordFormat.isValidWarcFieldName(null), "null is not a field name"); + assertFalse(WARCRecordFormat.isValidWarcFieldName(""), "empty string is not a field name"); + assertFalse( + WARCRecordFormat.isValidWarcFieldName("hops FromSeed"), + "a space is not allowed in a field name"); + assertFalse( + WARCRecordFormat.isValidWarcFieldName("hopsFromSeed: 1"), + "a colon is not allowed in a field name"); + assertTrue(WARCRecordFormat.isValidWarcFieldName("via")); + assertTrue(WARCRecordFormat.isValidWarcFieldName("feed.description")); + assertTrue(WARCRecordFormat.isValidWarcFieldName("WARC-Truncated")); + } + + @Test + void testSanitizeWarcFieldValue() { + assertNull(WARCRecordFormat.sanitizeWarcFieldValue(null)); + assertEquals("unchanged", WARCRecordFormat.sanitizeWarcFieldValue("unchanged")); + assertEquals("a b", WARCRecordFormat.sanitizeWarcFieldValue("a\r\nb")); + assertEquals("a b", WARCRecordFormat.sanitizeWarcFieldValue("a\rb")); + assertEquals("a b", WARCRecordFormat.sanitizeWarcFieldValue("a\nb")); + } + @Test void testWarcRecord() { // test validity of WARC record @@ -331,6 +356,47 @@ void testWarcResourceRecord() { "WARC record: no or incorrect block, digest"); } + @Test + void testWarcResourceRecordContentTypeCRLFInjection() { + // test that a server-controlled Content-Type cannot forge additional WARC header + // lines in a resource record + String txt = "abcdef"; + byte[] content = txt.getBytes(StandardCharsets.UTF_8); + Metadata metadata = new Metadata(); + metadata.addValue( + protocolMDprefix + HttpHeaders.CONTENT_TYPE, + "text/html\r\nWARC-Truncated: length\r\n"); + Tuple tuple = mock(Tuple.class); + when(tuple.getBinaryByField("content")).thenReturn(content); + when(tuple.getStringByField("url")).thenReturn("https://www.example.org/"); + when(tuple.getValueByField("metadata")).thenReturn(metadata); + WARCRecordFormat format = new WARCRecordFormat(protocolMDprefix); + byte[] warcBytes = format.format(tuple); + String warcString = new String(warcBytes, StandardCharsets.UTF_8); + assertFalse( + warcString.contains("\r\nWARC-Truncated: length"), + "WARC record: Content-Type must not forge additional WARC header lines"); + // CR and LF are replaced by spaces, the content type remains on its header line + assertTrue( + warcString.contains("Content-Type: text/html WARC-Truncated: length"), + "WARC record: sanitised Content-Type expected on a single header line"); + + // try to read it with Jwarc + try (WarcReader reader = new WarcReader(new ByteArrayInputStream(warcBytes))) { + for (WarcRecord record : reader) { + assertFalse( + record.headers().contains("WARC-Truncated", "length"), + "WARC record: WARC header block must not contain a forged header"); + assertEquals( + 1, + record.headers().all("Content-Type").size(), + "WARC record: expected a single Content-Type header"); + } + } catch (IOException e) { + throw new RuntimeException(e); + } + } + @Test void testWarcMetadataRecord() { Metadata metadata = new Metadata();