Skip to content

Commit b66757b

Browse files
stop config realm leaking to a cross-origin redirect target (#2224)
Cross-origin and https-to-http redirects strip the request realm, but exitAfterIntercept re-derived it from config.getRealm(), reattaching a client-wide realm to the redirect target: - the target answers 401 and then receives the configured credentials, leaking them to a different origin - Redirect30xInterceptor already clears future.getRealm() on the strip, so read the realm from the future instead of falling back to config Added a cross-origin 401 case to RedirectCredentialSecurityTest that fails without the change.
1 parent 463e578 commit b66757b

2 files changed

Lines changed: 48 additions & 1 deletion

File tree

client/src/main/java/org/asynchttpclient/netty/handler/intercept/Interceptors.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,12 @@ public boolean exitAfterIntercept(Channel channel, NettyResponseFuture<?> future
8787
ProxyServer proxyServer = future.getProxyServer();
8888
int statusCode = response.status().code();
8989
Request request = future.getCurrentRequest();
90-
Realm realm = request.getRealm() != null ? request.getRealm() : config.getRealm();
90+
// Use the realm the future is carrying (seeded from the request or client config when the
91+
// exchange started, and reset to null by Redirect30xInterceptor on a cross-origin or
92+
// scheme-downgrade redirect). Re-deriving from config.getRealm() here re-attaches the
93+
// client-wide credentials to a redirect target whose auth was just stripped, leaking them
94+
// to a different origin that answers 401.
95+
Realm realm = future.getRealm();
9196

9297
// This MUST BE called before Redirect30xInterceptor because latter assumes cookie store is already updated
9398
CookieStore cookieStore = config.getCookieStore();

client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ public class RedirectCredentialSecurityTest {
7070
private static final AtomicReference<String> cookieOnBounceBack = new AtomicReference<>();
7171
private static final AtomicReference<String> authAfterHttpsDowngrade = new AtomicReference<>();
7272
private static final AtomicReference<String> cookieAfterHttpsDowngrade = new AtomicReference<>();
73+
private static final AtomicReference<String> authOn401Target = new AtomicReference<>();
7374

7475
@BeforeAll
7576
public static void startServers() throws Exception {
@@ -194,6 +195,24 @@ public static void startServers() throws Exception {
194195
exchange.close();
195196
});
196197

198+
// Cross-domain redirect to a target that answers 401: the target must never receive
199+
// credentials, even those configured client-wide via config.setRealm(...).
200+
serverA.createContext("/redirect-to-b-401", exchange -> {
201+
exchange.getResponseHeaders().add("Location", "http://127.0.0.1:" + portB + "/target-401");
202+
exchange.sendResponseHeaders(302, -1);
203+
exchange.close();
204+
});
205+
206+
serverB.createContext("/target-401", exchange -> {
207+
String auth = exchange.getRequestHeaders().getFirst("Authorization");
208+
if (auth != null) {
209+
authOn401Target.set(auth);
210+
}
211+
exchange.getResponseHeaders().add("WWW-Authenticate", "Basic realm=\"target\"");
212+
exchange.sendResponseHeaders(401, -1);
213+
exchange.close();
214+
});
215+
197216
// HTTPS server on 127.0.0.1: issues a redirect downgrading to plain HTTP on server B
198217
httpsServer = HttpsServer.create(new InetSocketAddress("127.0.0.1", 0), 0);
199218
httpsServer.setHttpsConfigurator(new HttpsConfigurator(buildSslContext()));
@@ -700,6 +719,29 @@ void portChangeOnSameHostIsTreatedAsCrossOrigin() throws Exception {
700719
}
701720
}
702721

722+
/**
723+
* Client-wide credentials set via {@code config.setRealm(...)} must not be sent to a
724+
* cross-domain redirect target, even when that target answers 401 to solicit them. The
725+
* redirect clears the request/future realm, but the config realm must not be re-applied.
726+
*/
727+
@Test
728+
void crossDomainRedirectTo401TargetDoesNotLeakConfigRealm() throws Exception {
729+
DefaultAsyncHttpClientConfig config = new DefaultAsyncHttpClientConfig.Builder()
730+
.setFollowRedirect(true)
731+
.setRealm(basicAuthRealm("user", "password").build())
732+
.build();
733+
try (DefaultAsyncHttpClient client = new DefaultAsyncHttpClient(config)) {
734+
authOn401Target.set(null);
735+
736+
client.prepareGet("http://127.0.0.1:" + portA + "/redirect-to-b-401")
737+
.execute()
738+
.get(5, TimeUnit.SECONDS);
739+
740+
assertNull(authOn401Target.get(),
741+
"client-wide config Realm must not be sent to a cross-domain 401 target after redirect");
742+
}
743+
}
744+
703745
/**
704746
* HTTPS-to-HTTP same-host downgrade strips both Cookie and Authorization. Exercises the
705747
* {@code schemeDowngrade} branch in Redirect30xInterceptor.

0 commit comments

Comments
 (0)