Escape CR and LF in WARC metadata records and resource Content-Type

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<String> metadataKeys;
+    private final List<String> metadataKeys;
 
     public MetadataRecordFormat(List<String> 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<String> 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);
             }
         }
 

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 =

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<MessageHeaders> parseFields(byte[] warcBytes) {
+        List<MessageHeaders> 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<MessageHeaders> 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");
+        }
+    }
+}

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();