Skip to content

Commit ee7f3cc

Browse files
vandonrvandonr
authored
security fix for private headers that might be copied to a different origin on redirect in Apache (#11671)
fix for APMSP-3274 add empty marker header to prevent future blind copy catch potential exception Merge branch 'master' into vandonr/fix2 Co-authored-by: raphael.vandon <raphael.vandon@datadoghq.com>
1 parent 98481ff commit ee7f3cc

4 files changed

Lines changed: 196 additions & 18 deletions

File tree

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpasyncclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientModule.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ public String[] helperClassNames() {
2222
packageName + ".DelegatingRequestProducer",
2323
packageName + ".TraceContinuedFutureCallback",
2424
packageName + ".ApacheHttpAsyncClientDecorator",
25-
packageName + ".HostAndRequestAsHttpUriRequest"
25+
packageName + ".HostAndRequestAsHttpUriRequest",
26+
packageName + ".RedirectHelper"
2627
};
2728
}
2829

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpasyncclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpClientRedirectInstrumentation.java‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public void methodAdvice(MethodTransformer transformer) {
4646

4747
public static class ClientRedirectAdvice {
4848
@Advice.OnMethodExit(suppress = Throwable.class)
49-
private static void onAfterExecute(
49+
static void onAfterExecute(
5050
@Advice.Argument(value = 2) final HttpContext context,
5151
@Advice.Return(typing = Assigner.Typing.DYNAMIC) final HttpRequest redirect) {
5252
if (redirect == null) {
@@ -58,31 +58,31 @@ private static void onAfterExecute(
5858
}
5959
HttpRequest original = (HttpRequest) originalRequest;
6060

61-
// Apache HttpClient 4.0.1+ copies headers from original to redirect only
62-
// if redirect headers are empty. Because we add headers
63-
// "x-datadog-" and "x-b3-" to redirect: it means redirect headers never
64-
// will be empty. So in case if not-instrumented redirect had no headers,
65-
// we just copy all not set headers from original to redirect (doing same
66-
// thing as apache httpclient does).
67-
if (!redirect.headerIterator().hasNext()) {
68-
// redirect didn't have other headers besides tracing, so we need to do copy
69-
// (same work as Apache HttpClient 4.0.1+ does w/o instrumentation)
70-
if (original instanceof HttpRequestWrapper) {
71-
// We should use the initial request because the wrapped one might contain more headers
72-
// (i.e. Host) we do not want to copy
73-
// if we cannot access the original request we cannot safely copy.
74-
// At this point we break the propagation not to corrupt the customer request
75-
redirect.setHeaders(((HttpRequestWrapper) original).getOriginal().getAllHeaders());
76-
}
61+
// Apache HttpClient 4.0.1+ copies headers from the original request to the redirect only when
62+
// the redirect request has no headers. Because tracing injects propagation headers before
63+
// redirect handling completes, an otherwise empty redirect request may no longer look empty
64+
// to HttpClient. Preserve that header-copy behavior only for same-origin redirects;
65+
// cross-origin redirects must not receive application headers such as Authorization or
66+
// Cookie.
67+
boolean emptyRedirect = !redirect.headerIterator().hasNext();
68+
if (emptyRedirect && RedirectHelper.isSameOrigin(context, original, redirect)) {
69+
redirect.setHeaders(((HttpRequestWrapper) original).getOriginal().getAllHeaders());
7770
} else {
71+
boolean copiedPropagationHeader = false;
7872
for (final Header header : original.getAllHeaders()) {
7973
if (PropagationUtils.KNOWN_PROPAGATION_HEADERS.contains(
8074
header.getName().toLowerCase(Locale.ROOT))) {
8175
if (!redirect.containsHeader(header.getName())) {
8276
redirect.setHeader(header.getName(), header.getValue());
77+
copiedPropagationHeader = true;
8378
}
8479
}
8580
}
81+
if (emptyRedirect && !copiedPropagationHeader) {
82+
// When there are no propagation headers to copy, add a harmless header to keep HttpClient
83+
// from treating the redirect as empty and copying application headers later.
84+
redirect.setHeader("x-datadog-redirect", "true");
85+
}
8686
}
8787
}
8888
}

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpasyncclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpasyncclient/RedirectHelper.java‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package datadog.trace.instrumentation.apachehttpasyncclient;
2+
3+
import java.net.URI;
4+
import org.apache.http.HttpHost;
5+
import org.apache.http.HttpRequest;
6+
import org.apache.http.client.methods.HttpRequestWrapper;
7+
import org.apache.http.client.methods.HttpUriRequest;
8+
import org.apache.http.protocol.HttpContext;
9+
10+
public final class RedirectHelper {
11+
private RedirectHelper() {}
12+
13+
public static boolean isSameOrigin(
14+
final HttpContext context, final HttpRequest original, final HttpRequest redirect) {
15+
if (!(original instanceof HttpRequestWrapper) || !(redirect instanceof HttpUriRequest)) {
16+
return false;
17+
}
18+
HttpRequest unwrappedOriginal = ((HttpRequestWrapper) original).getOriginal();
19+
if (!(unwrappedOriginal instanceof HttpUriRequest)) {
20+
return false;
21+
}
22+
URI originalUri = ((HttpUriRequest) unwrappedOriginal).getURI();
23+
URI redirectUri = ((HttpUriRequest) redirect).getURI();
24+
if (originalUri == null || redirectUri == null) {
25+
return false;
26+
}
27+
28+
originalUri = resolveOriginalUri(context, originalUri);
29+
if (!redirectUri.isAbsolute()) {
30+
redirectUri = originalUri.resolve(redirectUri);
31+
}
32+
if (originalUri.getScheme() == null || redirectUri.getScheme() == null) {
33+
return false;
34+
}
35+
36+
String originalHost = originalUri.getHost();
37+
String redirectHost = redirectUri.getHost();
38+
if (originalHost == null || redirectHost == null) {
39+
return false;
40+
}
41+
42+
return originalUri.getScheme().equalsIgnoreCase(redirectUri.getScheme())
43+
&& originalHost.equalsIgnoreCase(redirectHost)
44+
&& effectivePort(originalUri) == effectivePort(redirectUri);
45+
}
46+
47+
private static URI resolveOriginalUri(final HttpContext context, final URI originalUri) {
48+
if (originalUri.isAbsolute()) {
49+
return originalUri;
50+
}
51+
Object targetHost = context.getAttribute("http.target_host");
52+
if (!(targetHost instanceof HttpHost)) {
53+
return originalUri;
54+
}
55+
HttpHost host = (HttpHost) targetHost;
56+
try {
57+
return URI.create(host.toURI()).resolve(originalUri);
58+
} catch (IllegalArgumentException e) {
59+
return originalUri;
60+
}
61+
}
62+
63+
private static int effectivePort(final URI uri) {
64+
if (uri.getPort() != -1) {
65+
return uri.getPort();
66+
}
67+
String scheme = uri.getScheme();
68+
if ("http".equalsIgnoreCase(scheme)) {
69+
return 80;
70+
}
71+
if ("https".equalsIgnoreCase(scheme)) {
72+
return 443;
73+
}
74+
return -1;
75+
}
76+
}

‎dd-java-agent/instrumentation/apache-httpclient/apache-httpasyncclient-4.0/src/test/java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpClientRedirectInstrumentationTest.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package datadog.trace.instrumentation.apachehttpasyncclient;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertFalse;
5+
6+
import org.apache.http.HttpHost;
7+
import org.apache.http.client.methods.HttpGet;
8+
import org.apache.http.client.methods.HttpRequestWrapper;
9+
import org.apache.http.protocol.BasicHttpContext;
10+
import org.apache.http.protocol.HttpContext;
11+
import org.junit.jupiter.api.Test;
12+
13+
class ApacheHttpClientRedirectInstrumentationTest {
14+
15+
@Test
16+
void doesNotCopyApplicationHeadersToCrossOriginRedirects() throws Exception {
17+
HttpGet original = originalRequest("http://example.com/request");
18+
original.addHeader("Authorization", "Bearer secret");
19+
original.addHeader("Cookie", "session=secret");
20+
original.addHeader("X-Api-Key", "secret");
21+
original.addHeader("x-datadog-trace-id", "123");
22+
23+
HttpGet redirect = new HttpGet("http://attacker.example/redirect");
24+
25+
ApacheHttpClientRedirectInstrumentation.ClientRedirectAdvice.onAfterExecute(
26+
contextWith(original), redirect);
27+
28+
assertFalse(redirect.containsHeader("Authorization"));
29+
assertFalse(redirect.containsHeader("Cookie"));
30+
assertFalse(redirect.containsHeader("X-Api-Key"));
31+
assertEquals("123", redirect.getFirstHeader("x-datadog-trace-id").getValue());
32+
}
33+
34+
@Test
35+
void preventsApacheHeaderCopyWhenCrossOriginRedirectHasNoPropagationHeaders() throws Exception {
36+
HttpGet original = originalRequest("http://example.com/request");
37+
original.addHeader("Authorization", "Bearer secret");
38+
original.addHeader("Cookie", "session=secret");
39+
40+
HttpGet redirect = new HttpGet("http://attacker.example/redirect");
41+
42+
ApacheHttpClientRedirectInstrumentation.ClientRedirectAdvice.onAfterExecute(
43+
contextWith(original), redirect);
44+
45+
assertFalse(redirect.containsHeader("Authorization"));
46+
assertFalse(redirect.containsHeader("Cookie"));
47+
assertEquals("true", redirect.getFirstHeader("x-datadog-redirect").getValue());
48+
}
49+
50+
@Test
51+
void copiesApplicationHeadersToSameOriginRedirects() throws Exception {
52+
HttpGet original = originalRequest("https://example.com/request");
53+
original.addHeader("Authorization", "Bearer secret");
54+
original.addHeader("x-datadog-trace-id", "123");
55+
56+
HttpGet redirect = new HttpGet("https://example.com/redirect");
57+
58+
ApacheHttpClientRedirectInstrumentation.ClientRedirectAdvice.onAfterExecute(
59+
contextWith(original), redirect);
60+
61+
assertEquals("Bearer secret", redirect.getFirstHeader("Authorization").getValue());
62+
assertEquals("123", redirect.getFirstHeader("x-datadog-trace-id").getValue());
63+
}
64+
65+
@Test
66+
void treatsDefaultPortsAsSameOrigin() throws Exception {
67+
HttpGet original = originalRequest("https://example.com:443/request");
68+
original.addHeader("Authorization", "Bearer secret");
69+
70+
HttpGet redirect = new HttpGet("https://example.com/redirect");
71+
72+
ApacheHttpClientRedirectInstrumentation.ClientRedirectAdvice.onAfterExecute(
73+
contextWith(original), redirect);
74+
75+
assertEquals("Bearer secret", redirect.getFirstHeader("Authorization").getValue());
76+
}
77+
78+
@Test
79+
void resolvesHostRequestOriginFromContext() throws Exception {
80+
HttpGet original = originalRequest("/request");
81+
original.addHeader("Authorization", "Bearer secret");
82+
83+
HttpGet redirect = new HttpGet("http://example.com/redirect");
84+
HttpContext context = contextWith(original);
85+
context.setAttribute("http.target_host", new HttpHost("example.com", 80, "http"));
86+
87+
ApacheHttpClientRedirectInstrumentation.ClientRedirectAdvice.onAfterExecute(context, redirect);
88+
89+
assertEquals("Bearer secret", redirect.getFirstHeader("Authorization").getValue());
90+
}
91+
92+
private static HttpGet originalRequest(final String uri) {
93+
return new HttpGet(uri);
94+
}
95+
96+
private static HttpContext contextWith(final HttpGet request) throws Exception {
97+
HttpContext context = new BasicHttpContext();
98+
context.setAttribute("http.request", HttpRequestWrapper.wrap(request));
99+
return context;
100+
}
101+
}

0 commit comments

Comments
 (0)