Skip to content

Commit 04f46bf

Browse files
arturobernalgarturobernalg
committed
Hardened WebSocket transport and permessage-deflate for RFC 6455/7692 conformance
Fixed the client permessage-deflate encoder, which stripped the flush trailer on every fragment and could truncate a SYNC_FLUSH, so a compressed message split across frames now decodes; the frame writer derives the masking key from SecureRandom. The session engine fails the connection on RSV1 set on a control frame, no longer delivers a truncated message after a 1009 close, and the HTTP/2 transport treats a selection key cancelled during shutdown as a gone channel instead of raising CancelledKeyException. On the server side the HTTP/2 handler advertises a bounded inbound flow-control window and back-pressures a bounded outbound queue, the frame reader rejects a 64-bit length with the most significant bit set, the frame writer enforces the 125-byte control frame limit and truncates the CLOSE reason, the processor fails a data frame received in the middle of a fragmented message, and extension negotiation accepts a given extension name only once. Both permessage-deflate implementations release their Deflater and Inflater when the session ends, and the HTTP/2 client accepts any 2xx handshake response.
1 parent 62716cf commit 04f46bf

29 files changed

Lines changed: 937 additions & 66 deletions

‎httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/client/impl/protocol/Http1UpgradeProtocol.java‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,9 @@ public void completed(final WebSocketEndpointConnector.ProtoEndpoint endpoint) {
158158
ext.append("; client_no_context_takeover");
159159
}
160160
if (cfg.getOfferClientMaxWindowBits() != null) {
161-
ext.append("; client_max_window_bits=").append(cfg.getOfferClientMaxWindowBits());
161+
// The JDK Deflater always compresses with a 15-bit window, so a
162+
// smaller configured value must not be promised to the server.
163+
ext.append("; client_max_window_bits=15");
162164
}
163165
if (cfg.getOfferServerMaxWindowBits() != null) {
164166
ext.append("; server_max_window_bits=").append(cfg.getOfferServerMaxWindowBits());
@@ -359,9 +361,28 @@ public void cancelled() {
359361
}
360362
}
361363

362-
private static String headerValue(final HttpResponse r, final String name) {
363-
final Header h = r.getFirstHeader(name);
364-
return h != null ? h.getValue() : null;
364+
static String headerValue(final HttpResponse r, final String name) {
365+
// RFC 6455 section 9.1 permits Sec-WebSocket-Extensions and Sec-WebSocket-Protocol
366+
// to be split across multiple header fields; combine them as a comma-separated list.
367+
final Header[] headers = r.getHeaders(name);
368+
if (headers.length == 0) {
369+
return null;
370+
}
371+
if (headers.length == 1) {
372+
return headers[0].getValue();
373+
}
374+
final StringBuilder buf = new StringBuilder();
375+
for (final Header h : headers) {
376+
final String value = h.getValue();
377+
if (value == null || value.isEmpty()) {
378+
continue;
379+
}
380+
if (buf.length() > 0) {
381+
buf.append(", ");
382+
}
383+
buf.append(value);
384+
}
385+
return buf.length() > 0 ? buf.toString() : null;
365386
}
366387

367388
private static boolean containsToken(final HttpResponse r, final String header, final String token) {
@@ -394,9 +415,10 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
394415
}
395416
boolean pmceSeen = false, serverNoCtx = false, clientNoCtx = false;
396417
Integer clientBits = null, serverBits = null;
397-
final boolean offerServerNoCtx = cfg.isOfferServerNoContextTakeover();
398-
final boolean offerClientNoCtx = cfg.isOfferClientNoContextTakeover();
399-
final Integer offerClientBits = cfg.getOfferClientMaxWindowBits();
418+
// The offer always advertises client_max_window_bits=15 because the JDK Deflater
419+
// cannot compress with a smaller window, so the response is validated against 15
420+
// regardless of the configured value.
421+
final Integer offerClientBits = cfg.getOfferClientMaxWindowBits() != null ? Integer.valueOf(15) : null;
400422
final Integer offerServerBits = cfg.getOfferServerMaxWindowBits();
401423

402424
final String[] tokens = ext.split(",");
@@ -418,15 +440,12 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
418440
final String p = parts[i].trim();
419441
final int eq = p.indexOf('=');
420442
if (eq < 0) {
443+
// RFC 7692 sections 7.1.1.1 and 7.1.1.2 permit the server to include either
444+
// no-context-takeover parameter in the response even when the offer did not;
445+
// both are always safe to honour.
421446
if ("server_no_context_takeover".equalsIgnoreCase(p)) {
422-
if (!offerServerNoCtx) {
423-
throw new IllegalStateException("Server selected server_no_context_takeover not offered");
424-
}
425447
serverNoCtx = true;
426448
} else if ("client_no_context_takeover".equalsIgnoreCase(p)) {
427-
if (!offerClientNoCtx) {
428-
throw new IllegalStateException("Server selected client_no_context_takeover not offered");
429-
}
430449
clientNoCtx = true;
431450
} else {
432451
throw new IllegalStateException("Unsupported permessage-deflate parameter: " + p);
@@ -453,9 +472,8 @@ static ExtensionChain buildExtensionChain(final WebSocketClientConfig cfg, final
453472
throw new IllegalStateException("Invalid client_max_window_bits: " + v, nfe);
454473
}
455474
} else if ("server_max_window_bits".equalsIgnoreCase(k)) {
456-
if (offerServerBits == null) {
457-
throw new IllegalStateException("Server selected server_max_window_bits not offered");
458-
}
475+
// RFC 7692 section 7.1.2.1 permits the server to include this parameter
476+
// uninvited; a server constraining its own window is always acceptable.
459477
try {
460478
if (v.isEmpty()) {
461479
throw new IllegalStateException("server_max_window_bits must have a value");

‎httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/client/impl/protocol/Http2ExtendedConnectProtocol.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,8 +211,9 @@ public void produceRequest(final RequestChannel channel, final HttpContext conte
211211
public void consumeResponse(final HttpResponse response, final EntityDetails entityDetails,
212212
final HttpContext context) throws HttpException, IOException {
213213

214-
if (response.getCode() != HttpStatus.SC_OK) {
215-
failFuture(new IllegalStateException("Unexpected status: " + response.getCode()));
214+
final int code = response.getCode();
215+
if (code < HttpStatus.SC_OK || code >= HttpStatus.SC_REDIRECTION) {
216+
failFuture(new IllegalStateException("Unexpected status: " + code));
216217
return;
217218
}
218219

@@ -321,8 +322,7 @@ public void cancel() {
321322
}
322323

323324
private static String headerValue(final HttpResponse r, final String name) {
324-
final Header h = r.getFirstHeader(name);
325-
return h != null ? h.getValue() : null;
325+
return Http1UpgradeProtocol.headerValue(r, name);
326326
}
327327

328328
private void failFuture(final Exception ex) {

‎httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/transport/DataStreamChannelTransport.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
import java.io.IOException;
3030
import java.nio.ByteBuffer;
31+
import java.nio.channels.CancelledKeyException;
3132

3233
import org.apache.hc.core5.annotation.Internal;
3334
import org.apache.hc.core5.http.nio.DataStreamChannel;
@@ -51,14 +52,24 @@ public int write(final ByteBuffer src) throws IOException {
5152
if (ch == null) {
5253
return 0;
5354
}
54-
return ch.write(src);
55+
try {
56+
return ch.write(src);
57+
} catch (final CancelledKeyException ignore) {
58+
// The selection key was cancelled by a concurrent shutdown; the channel is gone.
59+
return 0;
60+
}
5561
}
5662

5763
@Override
5864
public void requestOutput() {
5965
final DataStreamChannel ch = channel;
6066
if (ch != null) {
61-
ch.requestOutput();
67+
try {
68+
ch.requestOutput();
69+
} catch (final CancelledKeyException ignore) {
70+
// requestOutput is a best-effort nudge; if the channel's selection key was
71+
// cancelled by a concurrent shutdown there is nothing left to flush.
72+
}
6273
}
6374
}
6475

‎httpclient5-websocket/src/main/java/org/apache/hc/client5/http/websocket/transport/WebSocketSessionEngine.java‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ public final class WebSocketSessionEngine {
109109
final AtomicBoolean open = new AtomicBoolean(true);
110110
final AtomicBoolean closeSent = new AtomicBoolean(false);
111111
private final AtomicBoolean closeReceived = new AtomicBoolean(false);
112+
private final AtomicBoolean released = new AtomicBoolean(false);
112113
volatile boolean closeAfterFlush;
113114
private volatile ScheduledFuture<?> closeTimeoutFuture;
114115

@@ -332,6 +333,12 @@ private void handleFrame() {
332333
return;
333334
}
334335
if (FrameOpcode.isControl(op)) {
336+
if (r1) {
337+
// Control frames are never compressed, so an extension never defines RSV1 for them.
338+
initiateClose(1002, "RSV1 set on control frame");
339+
inbuf.clear();
340+
return;
341+
}
335342
if (!fin) {
336343
initiateClose(1002, "fragmented control frame");
337344
inbuf.clear();
@@ -379,8 +386,7 @@ private void handleFrame() {
379386
inbuf.clear();
380387
return;
381388
}
382-
appendToMessage(payload);
383-
if (fin) {
389+
if (appendToMessage(payload) && fin) {
384390
deliverAssembledMessage();
385391
}
386392
break;
@@ -484,15 +490,16 @@ private void startMessage(final int opcode, final ByteBuffer payload, final bool
484490
appendToMessage(payload);
485491
}
486492

487-
private void appendToMessage(final ByteBuffer payload) {
493+
private boolean appendToMessage(final ByteBuffer payload) {
488494
final int n = payload.remaining();
489495
assemblingSize += n;
490496
if (cfg.getMaxMessageSize() > 0 && assemblingSize > cfg.getMaxMessageSize()) {
491497
initiateClose(1009, "Message too big");
492-
return;
498+
return false;
493499
}
494500
assemblingBuf = WebSocketBufferOps.ensureCapacity(assemblingBuf, n);
495501
assemblingBuf.put(payload.asReadOnlyBuffer());
502+
return true;
496503
}
497504

498505
private void deliverAssembledMessage() {
@@ -673,6 +680,9 @@ boolean enqueueData(final OutFrame frame) {
673680
}
674681

675682
private void drainAndRelease() {
683+
if (!released.compareAndSet(false, true)) {
684+
return;
685+
}
676686
if (activeWrite != null) {
677687
if (activeWrite.dataFrame) {
678688
dataQueuedBytes.addAndGet(-activeWrite.size);
@@ -690,6 +700,12 @@ private void drainAndRelease() {
690700
dataQueuedBytes.addAndGet(-f.size);
691701
}
692702
}
703+
if (encChain != null) {
704+
encChain.close();
705+
}
706+
if (decChain != null) {
707+
decChain.close();
708+
}
693709
cancelCloseTimeout();
694710
}
695711

‎httpclient5-websocket/src/main/java/org/apache/hc/core5/websocket/PerMessageDeflateExtension.java‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,17 +144,21 @@ public ByteBuffer encode(final WebSocketFrameType type, final boolean fin, final
144144
deflater.setInput(input);
145145
final ByteArrayOutputStream out = new ByteArrayOutputStream(Math.max(128, input.length / 2));
146146
final byte[] buffer = new byte[Math.min(16384, Math.max(1024, input.length))];
147-
while (!deflater.needsInput()) {
148-
final int count = deflater.deflate(buffer, 0, buffer.length, Deflater.SYNC_FLUSH);
147+
// Drain until a SYNC_FLUSH leaves the output buffer partly filled; testing needsInput()
148+
// would stop once the input is consumed and could drop the trailing 00 00 FF FF flush bytes.
149+
int count;
150+
do {
151+
count = deflater.deflate(buffer, 0, buffer.length, Deflater.SYNC_FLUSH);
149152
if (count > 0) {
150153
out.write(buffer, 0, count);
151-
} else {
152-
break;
153154
}
154-
}
155+
} while (count == buffer.length);
155156
final byte[] data = out.toByteArray();
156157
final ByteBuffer encoded;
157-
if (data.length >= 4) {
158+
// Strip the 00 00 FF FF flush trailer only on the final fragment; non-final fragments
159+
// must keep it so each intermediate empty stored block stays valid and the reassembled
160+
// DEFLATE stream decodes (RFC 7692 section 7.2.1).
161+
if (fin && data.length >= 4) {
158162
encoded = ByteBuffer.wrap(data, 0, data.length - 4);
159163
} else {
160164
encoded = ByteBuffer.wrap(data);
@@ -183,6 +187,12 @@ public WebSocketExtensionData getResponseData() {
183187
return new WebSocketExtensionData(getName(), params);
184188
}
185189

190+
@Override
191+
public void close() {
192+
deflater.end();
193+
inflater.end();
194+
}
195+
186196
private static boolean isDataFrame(final WebSocketFrameType type) {
187197
return type == WebSocketFrameType.TEXT || type == WebSocketFrameType.BINARY;
188198
}

‎httpclient5-websocket/src/main/java/org/apache/hc/core5/websocket/WebSocketExtension.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,13 @@ default ByteBuffer encode(
7777
default WebSocketExtensionData getResponseData() {
7878
return new WebSocketExtensionData(getName(), null);
7979
}
80+
81+
/**
82+
* Releases any native resources held by this extension (e.g. a {@code Deflater} or
83+
* {@code Inflater}). Called once when the owning session terminates.
84+
*
85+
* @since 5.7
86+
*/
87+
default void close() {
88+
}
8089
}

‎httpclient5-websocket/src/main/java/org/apache/hc/core5/websocket/WebSocketExtensionRegistry.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,11 @@
2727
package org.apache.hc.core5.websocket;
2828

2929
import java.util.ArrayList;
30+
import java.util.HashSet;
3031
import java.util.LinkedHashMap;
3132
import java.util.List;
3233
import java.util.Map;
34+
import java.util.Set;
3335

3436
import org.apache.hc.core5.util.Args;
3537

@@ -52,14 +54,22 @@ public WebSocketExtensionNegotiation negotiate(
5254
final boolean server) throws WebSocketException {
5355
final List<WebSocketExtension> extensions = new ArrayList<>();
5456
final List<WebSocketExtensionData> responseData = new ArrayList<>();
57+
final Set<String> accepted = new HashSet<>();
5558
if (requested != null) {
5659
for (final WebSocketExtensionData request : requested) {
60+
// At most one offer per extension name is accepted; a second accepted offer of the
61+
// same extension would claim the same RSV bit and break both the response header and
62+
// the RSV bookkeeping in the frame reader.
63+
if (accepted.contains(request.getName())) {
64+
continue;
65+
}
5766
final WebSocketExtensionFactory factory = factories.get(request.getName());
5867
if (factory != null) {
5968
final WebSocketExtension extension = factory.create(request, server);
6069
if (extension != null) {
6170
extensions.add(extension);
6271
responseData.add(extension.getResponseData());
72+
accepted.add(request.getName());
6373
}
6474
}
6575
}

‎httpclient5-websocket/src/main/java/org/apache/hc/core5/websocket/WebSocketFrameReader.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,9 @@ WebSocketFrame readFrame() throws IOException {
9898
len = (len << 8) | (readByte() & 0xFF);
9999
}
100100
}
101+
if (len < 0) {
102+
throw new WebSocketException("64-bit frame length must have the most significant bit set to 0");
103+
}
101104
if (len > Integer.MAX_VALUE) {
102105
throw new WebSocketException("Frame payload too large: " + len);
103106
}

‎httpclient5-websocket/src/main/java/org/apache/hc/core5/websocket/WebSocketFrameWriter.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.List;
3535

3636
import org.apache.hc.core5.util.Args;
37+
import org.apache.hc.core5.websocket.message.CloseCodec;
3738

3839
class WebSocketFrameWriter {
3940

@@ -63,13 +64,9 @@ void writePong(final ByteBuffer payload) throws IOException {
6364
}
6465

6566
void writeClose(final int statusCode, final String reason) throws IOException {
66-
final byte[] reasonBytes = reason != null ? reason.getBytes(StandardCharsets.UTF_8) : new byte[0];
67-
final int len = 2 + reasonBytes.length;
68-
final ByteBuffer buffer = ByteBuffer.allocate(len);
69-
buffer.put((byte) ((statusCode >> 8) & 0xFF));
70-
buffer.put((byte) (statusCode & 0xFF));
71-
buffer.put(reasonBytes);
72-
buffer.flip();
67+
// CloseCodec truncates the reason to 123 UTF-8 bytes so the payload never
68+
// exceeds the 125-byte control frame limit (RFC 6455 section 5.5).
69+
final ByteBuffer buffer = ByteBuffer.wrap(CloseCodec.encode(statusCode, reason));
7370
writeFrame(WebSocketFrameType.CLOSE, buffer, false, false, false);
7471
}
7572

@@ -102,6 +99,9 @@ private void writeFrame(
10299
Args.notNull(type, "Frame type");
103100
final ByteBuffer buffer = payload != null ? payload.asReadOnlyBuffer() : ByteBuffer.allocate(0);
104101
final int payloadLen = buffer.remaining();
102+
if (type.isControl() && payloadLen > 125) {
103+
throw new IllegalArgumentException("Control frame payload > 125 bytes: " + payloadLen);
104+
}
105105
int firstByte = 0x80 | (type.getOpcode() & 0x0F);
106106
if (rsv1) {
107107
firstByte |= 0x40;

‎httpclient5-websocket/src/main/java/org/apache/hc/core5/websocket/WebSocketSession.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ public final class WebSocketSession {
5252
private final SocketAddress localAddress;
5353
private final WebSocketFrameReader reader;
5454
private final WebSocketFrameWriter writer;
55+
private final List<WebSocketExtension> extensions;
5556
private final ReentrantLock writeLock = new ReentrantLock();
5657
private volatile boolean closeSent;
5758

@@ -68,11 +69,24 @@ public WebSocketSession(
6869
this.remoteAddress = remoteAddress;
6970
this.localAddress = localAddress;
7071
final List<WebSocketExtension> negotiated = extensions != null ? extensions : Collections.emptyList();
72+
this.extensions = negotiated;
7173
this.reader = new WebSocketFrameReader(this.config, this.inputStream, negotiated);
7274
this.writer = new WebSocketFrameWriter(this.outputStream, negotiated);
7375
this.closeSent = false;
7476
}
7577

78+
/**
79+
* Releases native resources held by the negotiated extensions. Invoked once when the
80+
* session processing loop terminates.
81+
*
82+
* @since 5.7
83+
*/
84+
public void releaseExtensions() {
85+
for (final WebSocketExtension extension : extensions) {
86+
extension.close();
87+
}
88+
}
89+
7690
public SocketAddress getRemoteAddress() {
7791
return remoteAddress;
7892
}

0 commit comments

Comments
 (0)