Skip to content

Commit

Permalink
8344228: Revisit SecurityManager usage in java.net.http after JEP 486…
Browse files Browse the repository at this point in the history 
… integration

Reviewed-by: jpai
  • Loading branch information
@dfuch
dfuch committed Nov 15, 2024
1 parent 84ffb64 commit 40a055e
Show file tree
Hide file tree
Showing 25 changed files with 86 additions and 759 deletions.
3 changes: 0 additions & 3 deletions src/java.net.http/share/classes/java/net/http/HttpClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@
import java.net.InetSocketAddress;
import java.net.Proxy;
import java.net.ProxySelector;
import java.net.URLPermission;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.time.Duration;
import java.util.Objects;
import java.util.Optional;
Expand Down
155 changes: 3 additions & 152 deletions src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,15 @@
package jdk.internal.net.http;

import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.ProtocolException;
import java.net.ProxySelector;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLPermission;
import java.security.AccessControlContext;
import java.time.Duration;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import java.net.http.HttpClient;
import java.net.http.HttpHeaders;
import java.net.http.HttpResponse;
import java.net.http.HttpTimeoutException;

Expand All @@ -53,20 +43,11 @@
import jdk.internal.net.http.common.Utils;
import jdk.internal.net.http.common.Log;

import static jdk.internal.net.http.common.Utils.permissionForProxy;

/**
* One request/response exchange (handles 100/101 intermediate response also).
* depth field used to track number of times a new request is being sent
* for a given API request. If limit exceeded exception is thrown.
*
* Security check is performed here:
* - uses AccessControlContext captured at API level
* - checks for appropriate URLPermission for request
* - if permission allowed, grants equivalent SocketPermission to call
* - in case of direct HTTP proxy, checks additionally for access to proxy
* (CONNECT proxying uses its own Exchange, so check done there)
*
*/
final class Exchange<T> {

Expand All @@ -83,8 +64,6 @@ final class Exchange<T> {
// used to record possible cancellation raised before the exchImpl
// has been established.
private volatile IOException failed;
@SuppressWarnings("removal")
final AccessControlContext acc;
final MultiExchange<T> multi;
final Executor parentExecutor;
volatile boolean upgrading; // to HTTP/2
Expand All @@ -103,22 +82,6 @@ final class Exchange<T> {
this.upgrading = false;
this.client = multi.client();
this.multi = multi;
this.acc = multi.acc;
this.parentExecutor = multi.executor;
this.pushGroup = multi.pushGroup;
this.dbgTag = "Exchange";
}

/* If different AccessControlContext to be used */
Exchange(HttpRequestImpl request,
MultiExchange<T> multi,
@SuppressWarnings("removal") AccessControlContext acc)
{
this.request = request;
this.acc = acc;
this.upgrading = false;
this.client = multi.client();
this.multi = multi;
this.parentExecutor = multi.executor;
this.pushGroup = multi.pushGroup;
this.dbgTag = "Exchange";
Expand Down Expand Up @@ -338,7 +301,7 @@ private void checkCancelled() {
}
}

<T> CompletableFuture<T> checkCancelled(CompletableFuture<T> cf, HttpConnection connection) {
<U> CompletableFuture<U> checkCancelled(CompletableFuture<U> cf, HttpConnection connection) {
return cf.handle((r,t) -> {
if (t == null) {
if (multi.requestCancelled()) {
Expand All @@ -354,7 +317,7 @@ <T> CompletableFuture<T> checkCancelled(CompletableFuture<T> cf, HttpConnection
} catch (Throwable x) {
if (debug.on()) debug.log("Failed to close connection", x);
}
return MinimalFuture.<T>failedFuture(t);
return MinimalFuture.<U>failedFuture(t);
}
}
}
Expand Down Expand Up @@ -422,15 +385,6 @@ public CompletableFuture<Response> responseAsync() {
return responseAsyncImpl(null);
}

CompletableFuture<Response> responseAsyncImpl(HttpConnection connection) {
SecurityException e = checkPermissions();
if (e != null) {
return MinimalFuture.failedFuture(e);
} else {
return responseAsyncImpl0(connection);
}
}

// check whether the headersSentCF was completed exceptionally with
// ProxyAuthorizationRequired. If so the Response embedded in the
// exception is returned. Otherwise we proceed.
Expand Down Expand Up @@ -584,7 +538,7 @@ private CompletableFuture<Response> ignore1xxResponse(final Response rsp) {
}
}

CompletableFuture<Response> responseAsyncImpl0(HttpConnection connection) {
CompletableFuture<Response> responseAsyncImpl(HttpConnection connection) {
Function<ExchangeImpl<T>, CompletableFuture<Response>> after407Check;
bodyIgnored = null;
if (request.expectContinue()) {
Expand Down Expand Up @@ -735,109 +689,6 @@ HttpResponse.BodySubscriber<T> ignoreBody(HttpResponse.ResponseInfo hdrs) {
return MinimalFuture.completedFuture(resp);
}

private URI getURIForSecurityCheck() {
URI u;
String method = request.method();
InetSocketAddress authority = request.authority();
URI uri = request.uri();

// CONNECT should be restricted at API level
if (method.equalsIgnoreCase("CONNECT")) {
try {
u = new URI("socket",
null,
authority.getHostString(),
authority.getPort(),
null,
null,
null);
} catch (URISyntaxException e) {
throw new InternalError(e); // shouldn't happen
}
} else {
u = uri;
}
return u;
}

/**
* Returns the security permission required for the given details.
* If method is CONNECT, then uri must be of form "scheme://host:port"
*/
private static URLPermission permissionForServer(URI uri,
String method,
Map<String, List<String>> headers) {
if (method.equals("CONNECT")) {
return new URLPermission(uri.toString(), "CONNECT");
} else {
return Utils.permissionForServer(uri, method, headers.keySet().stream());
}
}

/**
* Performs the necessary security permission checks required to retrieve
* the response. Returns a security exception representing the denied
* permission, or null if all checks pass or there is no security manager.
*/
private SecurityException checkPermissions() {
String method = request.method();
@SuppressWarnings("removal")
SecurityManager sm = System.getSecurityManager();
if (sm == null || method.equals("CONNECT")) {
// tunneling will have a null acc, which is fine. The proxy
// permission check will have already been preformed.
return null;
}

HttpHeaders userHeaders = request.getUserHeaders();
URI u = getURIForSecurityCheck();
URLPermission p = permissionForServer(u, method, userHeaders.map());

try {
assert acc != null;
sm.checkPermission(p, acc);
} catch (SecurityException e) {
return e;
}
String hostHeader = userHeaders.firstValue("Host").orElse(null);
if (hostHeader != null && !hostHeader.equalsIgnoreCase(u.getHost())) {
// user has set a Host header different to request URI
// must check that for URLPermission also
URI u1 = replaceHostInURI(u, hostHeader);
URLPermission p1 = permissionForServer(u1, method, userHeaders.map());
try {
assert acc != null;
sm.checkPermission(p1, acc);
} catch (SecurityException e) {
return e;
}
}
ProxySelector ps = client.proxySelector();
if (ps != null) {
if (!method.equals("CONNECT")) {
// a non-tunneling HTTP proxy. Need to check access
URLPermission proxyPerm = permissionForProxy(request.proxy());
if (proxyPerm != null) {
try {
sm.checkPermission(proxyPerm, acc);
} catch (SecurityException e) {
return e;
}
}
}
}
return null;
}

private static URI replaceHostInURI(URI u, String hostPort) {
StringBuilder sb = new StringBuilder();
sb.append(u.getScheme())
.append("://")
.append(hostPort)
.append(u.getRawPath());
return URI.create(sb.toString());
}

HttpClient.Version version() {
return multi.version();
}
Expand Down
3 changes: 1 addition & 2 deletions src/java.net.http/share/classes/jdk/internal/net/http/HttpClientBuilderImpl.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -46,7 +46,6 @@ public class HttpClientBuilderImpl implements HttpClient.Builder {
Authenticator authenticator;
HttpClient.Version version;
Executor executor;
// Security parameters
SSLContext sslContext;
SSLParameters sslParams;
int priority = -1;
Expand Down
51 changes: 6 additions & 45 deletions src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -48,10 +48,7 @@
import java.nio.channels.SelectableChannel;
import java.nio.channels.SelectionKey;
import java.nio.channels.Selector;
import java.security.AccessControlContext;
import java.security.AccessController;
import java.security.NoSuchAlgorithmException;
import java.security.PrivilegedAction;
import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
Expand Down Expand Up @@ -97,7 +94,6 @@
import jdk.internal.net.http.common.OperationTrackers.Trackable;
import jdk.internal.net.http.common.OperationTrackers.Tracker;
import jdk.internal.net.http.websocket.BuilderImpl;
import jdk.internal.misc.InnocuousThread;

/**
* Client implementation. Contains all configuration information and also
Expand Down Expand Up @@ -131,16 +127,10 @@ private static final class DefaultThreadFactory implements ThreadFactory {
namePrefix = "HttpClient-" + clientID + "-Worker-";
}

@SuppressWarnings("removal")
@Override
public Thread newThread(Runnable r) {
String name = namePrefix + nextId.getAndIncrement();
Thread t;
if (System.getSecurityManager() == null) {
t = new Thread(null, r, name, 0, false);
} else {
t = InnocuousThread.newThread(name, r);
}
Thread t = new Thread(null, r, name, 0, false);
t.setDaemon(true);
return t;
}
Expand Down Expand Up @@ -188,15 +178,9 @@ public void ensureExecutedAsync(Runnable command) {
}
}

@SuppressWarnings("removal")
private void shutdown() {
if (delegate instanceof ExecutorService service) {
PrivilegedAction<?> action = () -> {
service.shutdown();
return null;
};
AccessController.doPrivileged(action, null,
new RuntimePermission("modifyThread"));
service.shutdown();
}
}
}
Expand Down Expand Up @@ -336,7 +320,6 @@ static void abortPendingRequests(HttpClientImpl client, Throwable reason) {
private final ConnectionPool connections;
private final DelegatingExecutor delegatingExecutor;
private final boolean isDefaultExecutor;
// Security parameters
private final SSLContext sslContext;
private final SSLParameters sslParams;
private final SelectorManager selmgr;
Expand Down Expand Up @@ -445,16 +428,6 @@ private HttpClientImpl(HttpClientBuilderImpl builder,
SingleFacadeFactory facadeFactory) {
id = CLIENT_IDS.incrementAndGet();
dbgTag = "HttpClientImpl(" + id +")";
@SuppressWarnings("removal")
var sm = System.getSecurityManager();
if (sm != null && builder.localAddr != null) {
// when a specific local address is configured, it will eventually
// lead to the SocketChannel.bind(...) call with an InetSocketAddress
// whose InetAddress is the local address and the port is 0. That ultimately
// leads to a SecurityManager.checkListen permission check for that port.
// so we do that security manager check here with port 0.
sm.checkListen(0);
}
localAddr = builder.localAddr;
if (builder.sslContext == null) {
try {
Expand Down Expand Up @@ -484,7 +457,7 @@ private HttpClientImpl(HttpClientBuilderImpl builder,
Redirect.NEVER : builder.followRedirects;
this.userProxySelector = builder.proxy;
this.proxySelector = Optional.ofNullable(userProxySelector)
.orElseGet(HttpClientImpl::getDefaultProxySelector);
.orElseGet(ProxySelector::getDefault);
if (debug.on())
debug.log("proxySelector is %s (user-supplied=%s)",
this.proxySelector, userProxySelector != null);
Expand Down Expand Up @@ -642,12 +615,6 @@ private static SSLParameters getDefaultParams(SSLContext ctx) {
return params;
}

@SuppressWarnings("removal")
private static ProxySelector getDefaultProxySelector() {
PrivilegedAction<ProxySelector> action = ProxySelector::getDefault;
return AccessController.doPrivileged(action);
}

// Returns the facade that was returned to the application code.
// May be null if that facade is no longer referenced.
final HttpClientFacade facade() {
Expand Down Expand Up @@ -992,7 +959,6 @@ private void debugCompleted(String tag, long startNanos, HttpRequest req) {
return sendAsync(userRequest, responseHandler, pushPromiseHandler, delegatingExecutor.delegate);
}

@SuppressWarnings("removal")
private <T> CompletableFuture<HttpResponse<T>>
sendAsync(HttpRequest userRequest,
BodyHandler<T> responseHandler,
Expand All @@ -1012,11 +978,7 @@ private void debugCompleted(String tag, long startNanos, HttpRequest req) {
return MinimalFuture.failedFuture(selmgr.selectorClosedException());
}

AccessControlContext acc = null;
if (System.getSecurityManager() != null)
acc = AccessController.getContext();

// Clone the, possibly untrusted, HttpRequest
// Clone the possibly untrusted HttpRequest
HttpRequestImpl requestImpl = new HttpRequestImpl(userRequest, proxySelector);
if (requestImpl.method().equals("CONNECT"))
throw new IllegalArgumentException("Unsupported method CONNECT");
Expand Down Expand Up @@ -1049,8 +1011,7 @@ private void debugCompleted(String tag, long startNanos, HttpRequest req) {
requestImpl,
this,
responseHandler,
pushPromiseHandler,
acc);
pushPromiseHandler);
CompletableFuture<HttpResponse<T>> mexCf = mex.responseAsync(executor);
CompletableFuture<HttpResponse<T>> res = mexCf.whenComplete((b,t) -> requestUnreference());
if (DEBUGELAPSED) {
Expand Down
Loading

1 comment on commit 40a055e

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.