Skip to content

refuse CR and LF in HttpFetcher request headers #3438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mrdevel2002-ui wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refuse CR and LF in HttpFetcher request headers #3438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions ...plugins/tika-pipes-http/src/main/java/org/apache/tika/pipes/fetcher/http/HttpFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Pattern;

import com.nimbusds.jose.JOSEException;
import org.apache.commons.io.IOUtils;
Expand Down Expand Up @@ -94,6 +95,9 @@ public static HttpFetcher build(ExtensionConfig pluginConfig) throws TikaConfigE
}


// RFC 7230 token
private static final Pattern HEADER_NAME = Pattern.compile("[!#$%&'*+.^_`|~0-9A-Za-z-]+");

private HttpFetcherConfig httpFetcherConfig = new HttpFetcherConfig();
private HttpClientFactory httpClientFactory = new HttpClientFactory();

Expand Down Expand Up @@ -175,7 +179,7 @@ private RequestConfig buildRequestConfig() {
.build();
}

private void setHttpRequestHeaders(Metadata metadata, HttpGet get) {
private void setHttpRequestHeaders(Metadata metadata, HttpGet get) throws TikaException {
if (!StringUtils.isBlank(userAgent)) {
get.setHeader(USER_AGENT, userAgent);
}
Expand All @@ -194,16 +198,34 @@ private void setHttpRequestHeaders(Metadata metadata, HttpGet get) {
}
}

private static void parseHeaderAndPutOnRequest(HttpGet get, String httpRequestHeader) {
private static void parseHeaderAndPutOnRequest(HttpGet get, String httpRequestHeader)
throws TikaException {
String[] parts = httpRequestHeader
.trim().split(":", 2);
if (parts.length >= 2) {
String key = parts[0].trim();
String value = parts[1].trim();
checkHeader(key, value);
get.setHeader(key, value);
}
}

/**
* httpcore folds CR and LF in a value into spaces but writes the name as given, so a
* name from per-request config that carries CRLF would inject request lines.
*/
private static void checkHeader(String name, String value) throws TikaException {
if (!HEADER_NAME.matcher(name).matches()) {
throw new TikaException("Invalid HTTP header name: " + name);
}
for (int i = 0; i < value.length(); i++) {
char c = value.charAt(i);
if ((c < 0x20 && c != '\t') || c == 0x7f) {
throw new TikaException("Invalid control character in HTTP header " + name);
}
}
}

private HttpFetcherRequestConfig getAdditionalHttpFetcherConfig(ParseContext parseContext)
throws TikaConfigException {
if (parseContext.hasJsonConfig("http-fetcher")) {
Expand Down Expand Up @@ -234,14 +256,18 @@ private void putAdditionalHeadersOnRequest(HttpFetcherRequestConfig additionalFe
httpGet.setHeader(USER_AGENT, httpFetcherConfig.getUserAgent());
}
if (additionalFetcherConfig != null && additionalFetcherConfig.getHttpRequestHeaders() != null) {
additionalFetcherConfig.getHttpRequestHeaders()
.getHeaders()
.forEach(httpGet::addHeader);
for (Map.Entry<String, String> e : additionalFetcherConfig.getHttpRequestHeaders()
.getHeaders().entries()) {
checkHeader(e.getKey(), e.getValue());
httpGet.addHeader(e.getKey(), e.getValue());
}
}
if (httpFetcherConfig.getHttpRequestHeaders() != null) {
httpFetcherConfig.getHttpRequestHeaders()
.getHeaders()
.forEach(httpGet::addHeader);
for (Map.Entry<String, String> e : httpFetcherConfig.getHttpRequestHeaders()
.getHeaders().entries()) {
checkHeader(e.getKey(), e.getValue());
httpGet.addHeader(e.getKey(), e.getValue());
}
}
if (jwtGenerator != null) {
try {
Expand All @@ -252,7 +278,8 @@ private void putAdditionalHeadersOnRequest(HttpFetcherRequestConfig additionalFe
}
}

private static void placeHeaderOnGetRequest(HttpGet get, String httpRequestHeader) {
private static void placeHeaderOnGetRequest(HttpGet get, String httpRequestHeader)
throws TikaException {
int idxOfEquals = httpRequestHeader.indexOf(':');
if (idxOfEquals == -1) {
return;
Expand All @@ -263,6 +290,7 @@ private static void placeHeaderOnGetRequest(HttpGet get, String httpRequestHeade
String headerValue = httpRequestHeader
.substring(idxOfEquals + 1)
.trim();
checkHeader(headerKey, headerValue);
get.setHeader(headerKey, headerValue);
}

Expand Down
36 changes: 36 additions & 0 deletions ...ins/tika-pipes-http/src/test/java/org/apache/tika/pipes/fetcher/http/HttpFetcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.zip.GZIPInputStream;

Expand Down Expand Up @@ -261,6 +262,41 @@ public String getReasonPhrase() {

}

/**
* httpcore writes a header name as given (only a value has CR and LF folded), so a
* per-request header name carrying CRLF would inject request lines.
*/
@Test
public void testHeadersWithCrLfAreRefused() throws Exception {
HttpResponse response = buildMockResponse(HttpStatus.SC_OK, IOUtils.toInputStream(CONTENT, Charset.defaultCharset()));
HttpClient httpClient = Mockito.mock(HttpClient.class);
Mockito.when(httpClient.execute(ArgumentMatchers.any(HttpUriRequest.class), ArgumentMatchers.any(HttpContext.class)))
.thenReturn(response);
httpFetcher.setHttpClient(httpClient);

ParseContext nameContext = new ParseContext();
nameContext.setJsonConfig("http-fetcher", OBJECT_MAPPER.writeValueAsString(
Map.of("httpRequestHeaders", Map.of("map", Map.of(
"X-Test\r\nInjected: yes\r\n\r\nPOST /evil HTTP/1.1\r\nHost: h",
List.of("v"))))));
assertThrows(TikaException.class,
() -> httpFetcher.fetch("http://localhost", new Metadata(), nameContext));

ParseContext valueContext = new ParseContext();
valueContext.setJsonConfig("http-fetcher", OBJECT_MAPPER.writeValueAsString(
Map.of("httpRequestHeaders", Map.of("map", Map.of(
"X-Test", List.of("v\r\nInjected: yes"))))));
assertThrows(TikaException.class,
() -> httpFetcher.fetch("http://localhost", new Metadata(), valueContext));

Metadata metadata = new Metadata();
metadata.set(Property.externalText("httpRequestHeaders"), "X-Test\r\nInjected: yes: v");
assertThrows(TikaException.class,
() -> httpFetcher.fetch("http://localhost", metadata, new ParseContext()));

Mockito.verifyNoInteractions(httpClient);
}

@Test
@Disabled("requires network connectivity")
public void testRedirect() throws Exception {
Expand Down