Re: [tomcat] branch 9.0.x updated: Fix BZ 66548 - Add validation of Sec-Websocket-Key header
On 08/06/2023 09:57, Rémy Maucherat wrote: On Thu, Jun 8, 2023 at 6:07 AM Rainer Jung wrote: Am 08.06.23 um 05:50 schrieb Rainer Jung: The new TestKeyHeader fails for me very frequently for TC 9 and TC 8.5, but not for 10.1 or 11. For TC 9 it fails roughly 30-50% of the times I run it. It fails for jsse and for tcnative and for a wide range of JDKs and RHEL/SLES Linux versions. The failure happens only for NIO2, not for NIO. When it fails, the test case takes about 60 seconds longer than when it is OK. The failing test case is often testValid - the first test case that runs - and sometime the second one testTooLong01. It seems the pattern for TC 8.5 is the same but the tests are still running. You probably already fixed it for 10.1 and 11, but for 9.0 and 8.5 the backport of 44e7282b54 seems to be missing. I'm not running into the issue but I added the commit to the two branches. Thanks. Arguably, reading and ignoring the response was fixing the symptom not the cause. The root cause being that: - Tomcat was stopped while processing an HTTP upgrade to WebSocket - the upgrade did not complete - the error was swallowed so the connection wasn't cleaned-up https://github.com/apache/tomcat/commit/a2c34d6569f2b959413b06852d7a957ff9e9c39d (back-ported to all versions) should have addressed the root cause. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Fix BZ 66548 - Add validation of Sec-Websocket-Key header
On Thu, Jun 8, 2023 at 6:07 AM Rainer Jung wrote: > > Am 08.06.23 um 05:50 schrieb Rainer Jung: > > The new TestKeyHeader fails for me very frequently for TC 9 and TC 8.5, > > but not for 10.1 or 11. > > > > For TC 9 it fails roughly 30-50% of the times I run it. It fails for > > jsse and for tcnative and for a wide range of JDKs and RHEL/SLES Linux > > versions. > > > > The failure happens only for NIO2, not for NIO. When it fails, the test > > case takes about 60 seconds longer than when it is OK. The failing test > > case is often testValid - the first test case that runs - and sometime > > the second one testTooLong01. > > > > It seems the pattern for TC 8.5 is the same but the tests are still > > running. > > You probably already fixed it for 10.1 and 11, but for 9.0 and 8.5 the > backport of 44e7282b54 seems to be missing. I'm not running into the issue but I added the commit to the two branches. Rémy - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Fix BZ 66548 - Add validation of Sec-Websocket-Key header
Am 08.06.23 um 05:50 schrieb Rainer Jung:
The new TestKeyHeader fails for me very frequently for TC 9 and TC 8.5,
but not for 10.1 or 11.
For TC 9 it fails roughly 30-50% of the times I run it. It fails for
jsse and for tcnative and for a wide range of JDKs and RHEL/SLES Linux
versions.
The failure happens only for NIO2, not for NIO. When it fails, the test
case takes about 60 seconds longer than when it is OK. The failing test
case is often testValid - the first test case that runs - and sometime
the second one testTooLong01.
It seems the pattern for TC 8.5 is the same but the tests are still
running.
You probably already fixed it for 10.1 and 11, but for 9.0 and 8.5 the
backport of 44e7282b54 seems to be missing.
Best regards,
Rainer
Am 24.05.23 um 12:29 schrieb ma...@apache.org:
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 37f979762b Fix BZ 66548 - Add validation of
Sec-Websocket-Key header
37f979762b is described below
commit 37f979762b6ce28031e8e14a3d258cb1349e05a1
Author: Mark Thomas
AuthorDate: Tue Apr 11 08:18:43 2023 +0100
Fix BZ 66548 - Add validation of Sec-Websocket-Key header
Note that the validation isn't perfect. It aims to be good enough.
https://bz.apache.org/bugzilla/show_bug.cgi?id=66548
---
.../apache/tomcat/util/codec/binary/Base64.java | 9 +++
.../tomcat/websocket/server/UpgradeUtil.java | 39 +-
.../tomcat/websocket/server/TestKeyHeader.java | 87
++
.../tomcat/websocket/server/TesterWsClient.java | 18 -
webapps/docs/changelog.xml | 11 +++
5 files changed, 159 insertions(+), 5 deletions(-)
diff --git a/java/org/apache/tomcat/util/codec/binary/Base64.java
b/java/org/apache/tomcat/util/codec/binary/Base64.java
index c3b4fa9c16..884c3190d0 100644
--- a/java/org/apache/tomcat/util/codec/binary/Base64.java
+++ b/java/org/apache/tomcat/util/codec/binary/Base64.java
@@ -434,6 +434,15 @@ public class Base64 extends BaseNCodec {
}
+ public static boolean isInAlphabet(char c) {
+ // Fast for valid data. May be slow for invalid data.
+ try {
+ return STANDARD_DECODE_TABLE[c] != -1;
+ } catch (ArrayIndexOutOfBoundsException ex) {
+ return false;
+ }
+ }
+
/**
* Encode table to use: either STANDARD or URL_SAFE. Note: the
DECODE_TABLE above remains static because it is able
* to decode both STANDARD and URL_SAFE streams, but the
encodeTable must be a member variable so we can switch
diff --git a/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
b/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
index eec6698f75..96d7aaf943 100644
--- a/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
+++ b/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
@@ -95,7 +95,7 @@ public class UpgradeUtil {
return;
}
key = req.getHeader(Constants.WS_KEY_HEADER_NAME);
- if (key == null) {
+ if (!validateKey(key)) {
resp.sendError(HttpServletResponse.SC_BAD_REQUEST);
return;
}
@@ -224,6 +224,43 @@ public class UpgradeUtil {
}
+ /*
+ * Validate the key. It should be the base64 encoding of a random
16-byte value. 16-bytes are encoded in 24 base64
+ * characters, the last two of which must be ==.
+ *
+ * The validation isn't perfect:
+ *
+ * - it doesn't check the final non-'=' character is valid in the
context of the number of bits it is meant to be
+ * encoding.
+ *
+ * - it doesn't check that the value is random and changes for
each connection.
+ *
+ * Given that this header is for the benefit of the client, not
the server, this should be good enough.
+ */
+ private static boolean validateKey(String key) {
+ if (key == null) {
+ return false;
+ }
+
+ if (key.length() != 24) {
+ return false;
+ }
+
+ char[] keyChars = key.toCharArray();
+ if (keyChars[22] != '=' || keyChars[23] != '=') {
+ return false;
+ }
+
+ for (int i = 0; i < 22; i++) {
+ if (!Base64.isInAlphabet(keyChars[i])) {
+ return false;
+ }
+ }
+
+ return true;
+ }
+
+
private static List
createTransformations(List negotiatedExtensions) {
TransformationFactory factory =
TransformationFactory.getInstance();
diff --git
a/test/org/apache/tomcat/websocket/server/TestKeyHeader.java
b/test/org/apache/tomcat/websocket/server/TestKeyHeader.java
new file mode 100644
index 00..fa05e44304
--- /dev/null
+++ b/test/org/apache/tomcat/websocket/server/TestKeyHeader.java
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apa
Re: [tomcat] branch 9.0.x updated: Fix BZ 66548 - Add validation of Sec-Websocket-Key header
The new TestKeyHeader fails for me very frequently for TC 9 and TC 8.5,
but not for 10.1 or 11.
For TC 9 it fails roughly 30-50% of the times I run it. It fails for
jsse and for tcnative and for a wide range of JDKs and RHEL/SLES Linux
versions.
The failure happens only for NIO2, not for NIO. When it fails, the test
case takes about 60 seconds longer than when it is OK. The failing test
case is often testValid - the first test case that runs - and sometime
the second one testTooLong01.
It seems the pattern for TC 8.5 is the same but the tests are still running.
Best regards,
Rainer
Am 24.05.23 um 12:29 schrieb ma...@apache.org:
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 37f979762b Fix BZ 66548 - Add validation of Sec-Websocket-Key header
37f979762b is described below
commit 37f979762b6ce28031e8e14a3d258cb1349e05a1
Author: Mark Thomas
AuthorDate: Tue Apr 11 08:18:43 2023 +0100
Fix BZ 66548 - Add validation of Sec-Websocket-Key header
Note that the validation isn't perfect. It aims to be good enough.
https://bz.apache.org/bugzilla/show_bug.cgi?id=66548
---
.../apache/tomcat/util/codec/binary/Base64.java| 9 +++
.../tomcat/websocket/server/UpgradeUtil.java | 39 +-
.../tomcat/websocket/server/TestKeyHeader.java | 87 ++
.../tomcat/websocket/server/TesterWsClient.java| 18 -
webapps/docs/changelog.xml | 11 +++
5 files changed, 159 insertions(+), 5 deletions(-)
diff --git a/java/org/apache/tomcat/util/codec/binary/Base64.java
b/java/org/apache/tomcat/util/codec/binary/Base64.java
index c3b4fa9c16..884c3190d0 100644
--- a/java/org/apache/tomcat/util/codec/binary/Base64.java
+++ b/java/org/apache/tomcat/util/codec/binary/Base64.java
@@ -434,6 +434,15 @@ public class Base64 extends BaseNCodec {
}
+public static boolean isInAlphabet(char c) {
+// Fast for valid data. May be slow for invalid data.
+try {
+return STANDARD_DECODE_TABLE[c] != -1;
+} catch (ArrayIndexOutOfBoundsException ex) {
+return false;
+}
+}
+
/**
* Encode table to use: either STANDARD or URL_SAFE. Note: the
DECODE_TABLE above remains static because it is able
* to decode both STANDARD and URL_SAFE streams, but the encodeTable must
be a member variable so we can switch
diff --git a/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
b/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
index eec6698f75..96d7aaf943 100644
--- a/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
+++ b/java/org/apache/tomcat/websocket/server/UpgradeUtil.java
@@ -95,7 +95,7 @@ public class UpgradeUtil {
return;
}
key = req.getHeader(Constants.WS_KEY_HEADER_NAME);
-if (key == null) {
+if (!validateKey(key)) {
resp.sendError(HttpServletResponse.SC_BAD_REQUEST);
return;
}
@@ -224,6 +224,43 @@ public class UpgradeUtil {
}
+/*
+ * Validate the key. It should be the base64 encoding of a random 16-byte
value. 16-bytes are encoded in 24 base64
+ * characters, the last two of which must be ==.
+ *
+ * The validation isn't perfect:
+ *
+ * - it doesn't check the final non-'=' character is valid in the context
of the number of bits it is meant to be
+ * encoding.
+ *
+ * - it doesn't check that the value is random and changes for each
connection.
+ *
+ * Given that this header is for the benefit of the client, not the
server, this should be good enough.
+ */
+private static boolean validateKey(String key) {
+if (key == null) {
+return false;
+}
+
+if (key.length() != 24) {
+return false;
+}
+
+char[] keyChars = key.toCharArray();
+if (keyChars[22] != '=' || keyChars[23] != '=') {
+return false;
+}
+
+for (int i = 0; i < 22; i++) {
+if (!Base64.isInAlphabet(keyChars[i])) {
+return false;
+}
+}
+
+return true;
+}
+
+
private static List createTransformations(List
negotiatedExtensions) {
TransformationFactory factory = TransformationFactory.getInstance();
diff --git a/test/org/apache/tomcat/websocket/server/TestKeyHeader.java
b/test/org/apache/tomcat/websocket/server/TestKeyHeader.java
new file mode 100644
index 00..fa05e44304
--- /dev/null
+++ b/test/org/apache/tomcat/websocket/server/TestKeyHeader.java
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional
