fix: returning addresses instead of hosts on the ClientConnection (#217)

also consolidates checks of whether a host or address is local closes: #CVE-2024-9666 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
2026-01-09 23:12:06 -03:30 · 2024-11-18 03:25:36 -05:00 · 2024-11-18 03:25:36 -05:00 · d0eaed4d82
commit d0eaed4d82
parent c4160df1e8
8 changed files with 265 additions and 33 deletions
--- a/common/src/main/java/org/keycloak/common/ClientConnection.java
+++ b/common/src/main/java/org/keycloak/common/ClientConnection.java
@ -25,6 +25,9 @@ package org.keycloak.common;
 */
 public interface ClientConnection {

+    /**
+     * @return the address as a string if it is available, otherwise null 
+     */
    String getRemoteAddr();
    String getRemoteHost();
    int getRemotePort();
--- a/common/src/main/java/org/keycloak/common/enums/SslRequired.java
+++ b/common/src/main/java/org/keycloak/common/enums/SslRequired.java
@ -35,23 +35,29 @@ public enum SslRequired {
        return isRequired(connection.getRemoteAddr());
    }

-    public boolean isRequired(String address) {
+    public boolean isRequired(String host) {
        switch (this) {
            case ALL:
                return true;
            case NONE:
                return false;
            case EXTERNAL:
-                return !isLocal(address);
+                // NOTE: this is sometimes using hostnames here, which require DNS resolution
+                // It assumes that the resolution will be the same on the client side
+                // - this will go away once EXTERNAL is no longer supported
+                return !isLocal(host);
            default:
                return true;
        }
    }

-    private boolean isLocal(String remoteAddress) {
+    private boolean isLocal(String host) {
+        if (host == null || host.isEmpty()) {
+            return false; // InetAddress.getByName returns localhost for these
+        }
        try {
-            InetAddress inetAddress = InetAddress.getByName(remoteAddress);
-            return inetAddress.isAnyLocalAddress() || inetAddress.isLoopbackAddress() || inetAddress.isSiteLocalAddress();
+            InetAddress inetAddress = InetAddress.getByName(host);
+            return inetAddress.isLoopbackAddress() || inetAddress.isSiteLocalAddress() || inetAddress.isLinkLocalAddress();
        } catch (UnknownHostException e) {
            return false;
        }
--- a/common/src/test/java/org/keycloak/common/enums/SslRequiredTest.java
+++ b/common/src/test/java/org/keycloak/common/enums/SslRequiredTest.java
@ -0,0 +1,20 @@
+package org.keycloak.common.enums;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+
+import org.junit.Test;
+
+public class SslRequiredTest {
+
+    @Test
+    public void sslRequiredExternalTest() throws IOException {
+        assertFalse(SslRequired.EXTERNAL.isRequired("127.0.0.1"));
+        assertTrue(SslRequired.EXTERNAL.isRequired((String)null));
+        assertTrue(SslRequired.EXTERNAL.isRequired(""));
+        assertTrue(SslRequired.EXTERNAL.isRequired("0.0.0.0"));
+    }
+
+}
--- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusClientConnection.java
+++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusClientConnection.java
@ -31,7 +31,7 @@ public final class QuarkusClientConnection implements ClientConnection {

    @Override
    public String getRemoteAddr() {
-        return request.remoteAddress().host();
+        return request.remoteAddress().hostAddress();
    }

    @Override
@ -46,7 +46,7 @@ public final class QuarkusClientConnection implements ClientConnection {

    @Override
    public String getLocalAddr() {
-        return request.localAddress().host();
+        return request.localAddress().hostAddress();
    }

    @Override
--- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ProxyDistTest.java
+++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ProxyDistTest.java
@ -18,6 +18,7 @@
 package org.keycloak.it.cli.dist;

 import io.quarkus.test.junit.main.Launch;
+import io.quarkus.test.junit.main.LaunchResult;
 import io.restassured.RestAssured;
 import io.restassured.config.RedirectConfig;
 import io.restassured.config.RestAssuredConfig;
@ -25,6 +26,7 @@ import org.apache.http.HttpHeaders;
 import org.junit.Assert;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import org.keycloak.it.junit5.extension.CLIResult;
 import org.keycloak.it.junit5.extension.DistributionTest;
 import org.keycloak.it.junit5.extension.RawDistOnly;
 import org.keycloak.it.junit5.extension.WithEnvVars;
@ -38,6 +40,9 @@ import static org.hamcrest.Matchers.containsString;
@WithEnvVars({"KEYCLOAK_ADMIN", "admin123", "KEYCLOAK_ADMIN_PASSWORD", "admin123"})
@RawDistOnly(reason = "Containers are immutable")
 public class ProxyDistTest {
+    
+    private static final String ADDRESS = "12.23.45.67";
+    private static final String NOT_ADDRESS = "notaddress";

    @BeforeAll
    public static void onBeforeAll() {
@ -82,10 +87,13 @@ public class ProxyDistTest {
    }

    @Test
-    @Launch({ "start-dev", "--hostname=mykeycloak.org", "--proxy-headers=forwarded" })
-    public void testForwardedProxyHeaders() {
+    @Launch({ "start-dev", "--hostname=mykeycloak.org", "--proxy=edge", "--spi-event-listener-provider=jboss-logging" })
+    public void testForwardedProxyHeaders(LaunchResult result) {
        assertForwardedHeader();
-        assertXForwardedHeadersAreIgnored();
+
+        CLIResult cliResult = (CLIResult)result;
+        cliResult.assertNoMessage(NOT_ADDRESS);
+        cliResult.assertMessage(ADDRESS);
    }

    @Test
@ -117,10 +125,16 @@ public class ProxyDistTest {
    }

    private void assertForwardedHeader() {
+        // trigger a login error
+        assertForwardedHeader("http://mykeycloak.org:8080/realms/master/protocol/openid-connect/auth?client_id=security-admin-console", "https://test:1234/admin", ADDRESS);
+        assertForwardedHeader("http://mykeycloak.org:8080/realms/master/protocol/openid-connect/auth?client_id=security-admin-console", "https://test:1234/admin", NOT_ADDRESS);
+    }
+
+    private void assertForwardedHeader(String url, String expectedUrl, String forAddress) {
        given()
-                .header("Forwarded", "for=12.34.56.78;host=test:1234;proto=https, for=23.45.67.89")
-                .when().get("http://mykeycloak.org:8080")
-                .then().header(HttpHeaders.LOCATION, containsString("https://test:1234/admin"));
+                .header("Forwarded", "for="+forAddress+";host=test:1234;proto=https, for=23.45.67.89")
+                .when().get(url)
+                .then().header(HttpHeaders.LOCATION, containsString(expectedUrl));
    }

    private void assertForwardedHeaderIsIgnored() {
--- a/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java
+++ b/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java
@ -48,13 +48,12 @@ import org.keycloak.theme.Theme;
 import org.keycloak.theme.freemarker.FreeMarkerProvider;
 import org.keycloak.urls.UrlType;
 import org.keycloak.utils.MediaType;
+import org.keycloak.utils.SecureContextResolver;

 import java.io.IOException;
 import java.io.InputStream;
-import java.net.InetAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.net.UnknownHostException;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
@ -261,25 +260,17 @@ public class WelcomeResource {
    }

    private boolean isLocal() {
-        try {
-            ClientConnection clientConnection = session.getContext().getConnection();
-            InetAddress remoteInetAddress = InetAddress.getByName(clientConnection.getRemoteAddr());
-            InetAddress localInetAddress = InetAddress.getByName(clientConnection.getLocalAddr());
-            HttpRequest request = session.getContext().getHttpRequest();
-            HttpHeaders headers = request.getHttpHeaders();
-            String xForwardedFor = headers.getHeaderString("X-Forwarded-For");
-            logger.debugf("Checking WelcomePage. Remote address: %s, Local address: %s, X-Forwarded-For header: %s", remoteInetAddress.toString(), localInetAddress.toString(), xForwardedFor);
+        ClientConnection clientConnection = session.getContext().getConnection();
+        String remoteAddress = clientConnection.getRemoteAddr();
+        String localAddress = clientConnection.getLocalAddr();
+        HttpRequest request = session.getContext().getHttpRequest();
+        HttpHeaders headers = request.getHttpHeaders();
+        String xForwardedFor = headers.getHeaderString("X-Forwarded-For");
+        String forwarded = headers.getHeaderString("Forwarded");
+        logger.debugf("Checking WelcomePage. Remote address: %s, Local address: %s, X-Forwarded-For header: %s, Forwarded header: %s", remoteAddress.toString(), localAddress.toString(), xForwardedFor, forwarded);

-            // Access through AJP protocol (loadbalancer) may cause that remoteAddress is "127.0.0.1".
-            // So consider that welcome page accessed locally just if it was accessed really through "localhost" URL and without loadbalancer (x-forwarded-for header is empty).
-            return isLocalAddress(remoteInetAddress) && isLocalAddress(localInetAddress) && xForwardedFor == null;
-        } catch (UnknownHostException e) {
-            throw new WebApplicationException(e, Response.Status.INTERNAL_SERVER_ERROR);
-        }
-    }
-
-    private boolean isLocalAddress(InetAddress inetAddress) {
-        return inetAddress.isAnyLocalAddress() || inetAddress.isLoopbackAddress();
+        // Consider that welcome page accessed locally just if it was accessed really through "localhost" URL and without loadbalancer (x-forwarded-for and forwarded header is empty).
+        return xForwardedFor == null && forwarded == null && SecureContextResolver.isLocalAddress(remoteAddress) && SecureContextResolver.isLocalAddress(localAddress);
    }

    private String setCsrfCookie() {
--- a/services/src/main/java/org/keycloak/utils/SecureContextResolver.java
+++ b/services/src/main/java/org/keycloak/utils/SecureContextResolver.java
@ -0,0 +1,92 @@
+package org.keycloak.utils;
+
+import org.keycloak.device.DeviceRepresentationProvider;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.representations.account.DeviceRepresentation;
+
+import java.net.URI;
+import java.util.function.Supplier;
+import java.util.regex.Pattern;
+
+public class SecureContextResolver {
+
+    private static final Pattern LOCALHOST_IPV4 = Pattern.compile("127.\\d{1,3}.\\d{1,3}.\\d{1,3}");
+
+    /**
+     * Determines if a session is within a 'secure context', meaning its origin is considered potentially trustworthy by user-agents.
+     *
+     * @see <a href="https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts">MDN Web Docs — Secure Contexts</a>
+     * @see <a href="https://w3c.github.io/webappsec-secure-contexts/#algorithms">W3C Secure Contexts specification — Is origin potentially trustworthy?</a>
+     * @param session The session to check for trustworthiness.
+     * @return Whether the session can be considered potentially trustworthy by user-agents.
+     */
+    public static boolean isSecureContext(KeycloakSession session) {
+        URI uri = session.getContext().getUri().getRequestUri();
+
+        // Use a Supplier so the user-agent is evaluated lazily, avoiding unnecessary parsing in production deployments.
+        Supplier<DeviceRepresentation> deviceRepresentationSupplier = () -> {
+            DeviceRepresentationProvider deviceRepresentationProvider = session.getProvider(DeviceRepresentationProvider.class);
+            return deviceRepresentationProvider.deviceRepresentation();
+        };
+
+        return isSecureContext(uri, deviceRepresentationSupplier);
+    }
+
+    static boolean isSecureContext(URI uri, Supplier<DeviceRepresentation> deviceRepresentationSupplier) {
+        if (uri.getScheme().equals("https")) {
+            return true;
+        }
+
+        DeviceRepresentation deviceRepresentation = deviceRepresentationSupplier.get();
+        String browser = deviceRepresentation != null ? deviceRepresentation.getBrowser() : null;
+
+        // Safari has a bug where even a secure context is not able to set cookies with the 'Secure' directive.
+        // Hence, we need to assume the worst case scenario and downgrade to an insecure context.
+        // See:
+        // - https://github.com/keycloak/keycloak/issues/33557
+        // - https://webcompat.com/issues/142566
+        // - https://bugs.webkit.org/show_bug.cgi?id=232088
+        // - https://bugs.webkit.org/show_bug.cgi?id=276313
+        if (browser != null && browser.toLowerCase().contains("safari")) {
+            return false;
+        }
+
+        String host = uri.getHost();
+
+        if (host == null) {
+            return false;
+        }
+
+        if (isLocalAddress(host)) {
+            return true;
+        }
+
+        if (host.equals("localhost") || host.equals("localhost.")) {
+            return true;
+        }
+
+        return host.endsWith(".localhost") || host.endsWith(".localhost.");
+    }
+
+    /**
+     * Test whether the given address is the localhost
+     * @param address
+     * @return false if the address is not localhost or not an address value
+     */
+    public static boolean isLocalAddress(String address) {
+        if (address == null) {
+            return false;
+        }
+        // The host matches a CIDR notation of ::1/128
+        if (address.equals("[::1]") || address.equals("[0000:0000:0000:0000:0000:0000:0000:0001]")) {
+            return true;
+        }
+
+        // The host matches a CIDR notation of 127.0.0.0/8
+        if (LOCALHOST_IPV4.matcher(address).matches()) {
+            return true;
+        }
+        
+        return false;
+    }
+}
--- a/services/src/test/java/org/keycloak/utils/SecureContextResolverTest.java
+++ b/services/src/test/java/org/keycloak/utils/SecureContextResolverTest.java
@ -0,0 +1,106 @@
+package org.keycloak.utils;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.keycloak.representations.account.DeviceRepresentation;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.function.Supplier;
+
+public class SecureContextResolverTest {
+
+    static DeviceRepresentation DEVICE_UNKOWN;
+    static DeviceRepresentation DEVICE_SAFARI;
+
+    static {
+        DEVICE_UNKOWN = new DeviceRepresentation();
+        DEVICE_UNKOWN.setBrowser(DeviceRepresentation.UNKNOWN);
+
+        DEVICE_SAFARI = new DeviceRepresentation();
+        DEVICE_SAFARI.setBrowser("Safari/18.0.1");
+    }
+
+    @Test
+    public void testHttps() {
+        assertSecureContext("https://127.0.0.1", true);
+        assertSecureContext("https://something", true);
+    }
+
+    @Test
+    public void testIp4() {
+        assertSecureContext("http://127.0.0.1", true);
+        assertSecureContext("http://127.0.0.128", true);
+        assertSecureContext("http://127.0.0.255", true);
+        assertSecureContext("http://127.0.0.256", false);
+        assertSecureContext("http://127.0.1.1", true);
+        assertSecureContext("http://127.254.232.123", true);
+        assertSecureContext("http://127.256.232.123", false);
+        assertSecureContext("http://10.0.0.10", false);
+        assertSecureContext("http://127.0.0.1.nip.io", false);
+    }
+
+    @Test
+    public void testIp6() {
+        assertSecureContext("http://[::1]", true);
+        assertSecureContext("http://[0000:0000:0000:0000:0000:0000:0000:0001]", true);
+        assertSecureContext("http://[::2]", false);
+        assertSecureContext("http://[2001:0000:130F:0000:0000:09C0:876A:130B]", false);
+        assertSecureContext("http://::1", false);
+        assertSecureContext("http://[FE80:0000:130F:0000:0000:09C0:876A:130B]", false);
+    }
+
+    @Test
+    public void testLocalhost() {
+        assertSecureContext("http://localhost", true);
+        assertSecureContext("http://localhost.", true);
+        assertSecureContext("http://localhostn", false);
+        assertSecureContext("http://test.localhost", true);
+        assertSecureContext("http://test.localhost.", true);
+        assertSecureContext("http://test.localhostn", false);
+        assertSecureContext("http://test.localhost.not", false);
+    }
+    
+    @Test
+    public void testIsLocalhost() {
+        assertTrue(SecureContextResolver.isLocalAddress("127.0.0.1"));
+        assertFalse(SecureContextResolver.isLocalAddress("not.an.ip"));
+        assertFalse(SecureContextResolver.isLocalAddress(null));
+        assertFalse(SecureContextResolver.isLocalAddress(""));
+    }
+
+    @Test
+    public void testQuirksSafari() {
+        assertSecureContext("https://127.0.0.1", DEVICE_SAFARI, true);
+        assertSecureContext("https://something", DEVICE_SAFARI, true);
+        assertSecureContext("http://[::1]", DEVICE_SAFARI,false);
+        assertSecureContext("http://[0000:0000:0000:0000:0000:0000:0000:0001]", DEVICE_SAFARI, false);
+        assertSecureContext("http://localhost", DEVICE_SAFARI, false);
+        assertSecureContext("http://localhost.", DEVICE_SAFARI, false);
+        assertSecureContext("http://test.localhost", DEVICE_SAFARI, false);
+        assertSecureContext("http://test.localhost.", DEVICE_SAFARI, false);
+    }
+
+    @Test
+    public void testNoDeviceRepresentation() {
+        assertSecureContext("http://localhost", null, true);
+    }
+
+    void assertSecureContext(String url, boolean expectedSecureContext) {
+        assertSecureContext(url, DEVICE_UNKOWN, expectedSecureContext);
+    }
+
+    void assertSecureContext(String url, DeviceRepresentation deviceRepresentation, boolean expectedSecureContext) {
+        Supplier<DeviceRepresentation> deviceRepresentationSupplier = () -> deviceRepresentation;
+
+        try {
+            Assert.assertEquals(expectedSecureContext, SecureContextResolver.isSecureContext(new URI(url), deviceRepresentationSupplier));
+        } catch (URISyntaxException e) {
+            Assert.fail(e.getMessage());
+        }
+    }
+
+}