Skip to content

decline invalid permessage-deflate offers rather than fail handshake #1019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sahvx655-wq wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −66
Open

decline invalid permessage-deflate offers rather than fail handshake #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 75 additions & 66 deletions java/org/apache/tomcat/websocket/PerMessageDeflate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,82 +87,91 @@ static PerMessageDeflate build(List<List<Parameter>> preferences, boolean isServ
boolean clientContextTakeover = true;
int clientMaxWindowBits = -1;

for (Parameter param : preference) {
if (SERVER_NO_CONTEXT_TAKEOVER.equals(param.getName())) {
if (serverContextTakeover) {
serverContextTakeover = false;
} else {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", SERVER_NO_CONTEXT_TAKEOVER));
}
} else if (CLIENT_NO_CONTEXT_TAKEOVER.equals(param.getName())) {
if (clientContextTakeover) {
clientContextTakeover = false;
} else {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", CLIENT_NO_CONTEXT_TAKEOVER));
}
} else if (SERVER_MAX_WINDOW_BITS.equals(param.getName())) {
if (serverMaxWindowBits == -1) {
serverMaxWindowBits = Integer.parseInt(param.getValue());
if (serverMaxWindowBits < 8 || serverMaxWindowBits > 15) {
throw new IllegalArgumentException(sm.getString("perMessageDeflate.invalidWindowSize",
SERVER_MAX_WINDOW_BITS, Integer.valueOf(serverMaxWindowBits)));
}
// Java SE API (as of Java 11) does not expose the API to
// control the Window size. It is effectively hard-coded
// to 15
if (isServer && serverMaxWindowBits != 15) {
ok = false;
break;
// Note server window size is not an issue for the
// client since the client will assume 15 and if the
// server uses a smaller window everything will
// still work
try {
for (Parameter param : preference) {
if (SERVER_NO_CONTEXT_TAKEOVER.equals(param.getName())) {
if (serverContextTakeover) {
serverContextTakeover = false;
} else {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", SERVER_NO_CONTEXT_TAKEOVER));
}
} else {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", SERVER_MAX_WINDOW_BITS));
}
} else if (CLIENT_MAX_WINDOW_BITS.equals(param.getName())) {
if (clientMaxWindowBits == -1) {
if (param.getValue() == null) {
// Hint to server that the client supports this
// option. Java SE API (as of Java 11) does not
// expose the API to control the Window size. It is
// effectively hard-coded to 15
clientMaxWindowBits = 15;
} else if (CLIENT_NO_CONTEXT_TAKEOVER.equals(param.getName())) {
if (clientContextTakeover) {
clientContextTakeover = false;
} else {
clientMaxWindowBits = Integer.parseInt(param.getValue());
if (clientMaxWindowBits < 8 || clientMaxWindowBits > 15) {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", CLIENT_NO_CONTEXT_TAKEOVER));
}
} else if (SERVER_MAX_WINDOW_BITS.equals(param.getName())) {
if (serverMaxWindowBits == -1) {
serverMaxWindowBits = Integer.parseInt(param.getValue());
if (serverMaxWindowBits < 8 || serverMaxWindowBits > 15) {
throw new IllegalArgumentException(sm.getString("perMessageDeflate.invalidWindowSize",
CLIENT_MAX_WINDOW_BITS, Integer.valueOf(clientMaxWindowBits)));
SERVER_MAX_WINDOW_BITS, Integer.valueOf(serverMaxWindowBits)));
}
// Java SE API (as of Java 11) does not expose the API to
// control the Window size. It is effectively hard-coded
// to 15
if (isServer && serverMaxWindowBits != 15) {
ok = false;
break;
// Note server window size is not an issue for the
// client since the client will assume 15 and if the
// server uses a smaller window everything will
// still work
}
} else {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", SERVER_MAX_WINDOW_BITS));
}
// Java SE API (as of Java 11) does not expose the API to
// control the Window size. It is effectively hard-coded
// to 15
if (!isServer && clientMaxWindowBits != 15) {
ok = false;
break;
// Note client window size is not an issue for the
// server since the server will assume 15 and if the
// client uses a smaller window everything will
// still work
} else if (CLIENT_MAX_WINDOW_BITS.equals(param.getName())) {
if (clientMaxWindowBits == -1) {
if (param.getValue() == null) {
// Hint to server that the client supports this
// option. Java SE API (as of Java 11) does not
// expose the API to control the Window size. It is
// effectively hard-coded to 15
clientMaxWindowBits = 15;
} else {
clientMaxWindowBits = Integer.parseInt(param.getValue());
if (clientMaxWindowBits < 8 || clientMaxWindowBits > 15) {
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.invalidWindowSize", CLIENT_MAX_WINDOW_BITS,
Integer.valueOf(clientMaxWindowBits)));
}
}
// Java SE API (as of Java 11) does not expose the API to
// control the Window size. It is effectively hard-coded
// to 15
if (!isServer && clientMaxWindowBits != 15) {
ok = false;
break;
// Note client window size is not an issue for the
// server since the server will assume 15 and if the
// client uses a smaller window everything will
// still work
}
} else {
// Duplicate definition
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", CLIENT_MAX_WINDOW_BITS));
}
} else {
// Duplicate definition
// Unknown parameter
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.duplicateParameter", CLIENT_MAX_WINDOW_BITS));
sm.getString("perMessageDeflate.unknownParameter", param.getName()));
}
} else {
// Unknown parameter
throw new IllegalArgumentException(
sm.getString("perMessageDeflate.unknownParameter", param.getName()));
}
} catch (IllegalArgumentException iae) {
// An invalid extension parameter has been offered. RFC 7692
// section 5.1 requires the offer to be declined and the handshake to
// continue (without this extension) rather than failing. Try the
// next offered configuration.
ok = false;
}
if (ok) {
return new PerMessageDeflate(serverContextTakeover, serverMaxWindowBits, clientContextTakeover,
Expand Down
25 changes: 25 additions & 0 deletions test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,31 @@ public void testFlushBatchMessagePart() throws IOException {
Assert.assertEquals(mp2, compressedParts.get(1));
}


/*
* RFC 7692 section 5.1 requires a permessage-deflate offer that contains an invalid extension parameter to be
* declined so the handshake continues without compression. The offer must not fail the handshake.
*/
@Test
public void testInvalidParameterDeclinesOffer() {
// client_max_window_bits with an out-of-range value
assertDeclined("client_max_window_bits", "16");
// client_max_window_bits with a non-numeric value
assertDeclined("client_max_window_bits", "x");
// server_max_window_bits offered without a value
assertDeclined("server_max_window_bits", null);
}


private static void assertDeclined(String name, String value) {
List<Parameter> parameters = new ArrayList<>();
parameters.add(new WsExtensionParameter(name, value));
List<List<Parameter>> preferences = new ArrayList<>();
preferences.add(parameters);

Assert.assertNull(PerMessageDeflate.build(preferences, true));
}

/*
* Minimal implementation to enable other transformations to be tested. It is NOT robust.
*/
Expand Down