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 <ma...@apache.org> 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<Transformation> createTransformations(List<Extension> 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 0000000000..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 information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.websocket.server; + +import java.nio.charset.StandardCharsets; + +import javax.servlet.http.HttpServletResponse; +import javax.websocket.CloseReason.CloseCodes; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.tomcat.websocket.TesterEchoServer; +import org.apache.tomcat.websocket.WebSocketBaseTest; + +public class TestKeyHeader extends WebSocketBaseTest { + + @Test + public void testEmptyString() throws Exception { + doTest("", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testValid() throws Exception { + // "0123456789012345" encoded with base64 + doTest("MDEyMzQ1Njc4OTAxMjM0NQ==", HttpServletResponse.SC_SWITCHING_PROTOCOLS); + } + + + @Test + public void testInvalidCharacter() throws Exception { + // "0123456789012345" encoded with base64 + doTest("MDEy(zQ1Njc4OTAxMjM0NQ==", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testTooShort() throws Exception { + // "012345678901234" encoded with base64 + doTest("MDEyMzQ1Njc4OTAxMjM0", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testTooLong01() throws Exception { + // "01234567890123456" encoded with base64 + doTest("MDEyMzQ1Njc4OTAxMjM0NTY=", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testTooLong02() throws Exception { + // "012345678901234678" encoded with base64 + doTest("MDEyMzQ1Njc4OTAxMjM0NTY3OA==", HttpServletResponse.SC_BAD_REQUEST); + } + + private void doTest(String keyHeaderValue, int expectedStatusCode) throws Exception { + startServer(TesterEchoServer.Config.class); + + TesterWsClient client = new TesterWsClient("localhost", getPort(), keyHeaderValue); + String req = client.createUpgradeRequest(TesterEchoServer.Config.PATH_BASIC); + client.write(req.getBytes(StandardCharsets.UTF_8)); + int rc = client.readUpgradeResponse(); + + Assert.assertEquals(expectedStatusCode, rc); + + if (expectedStatusCode == HttpServletResponse.SC_SWITCHING_PROTOCOLS) { + client.sendCloseFrame(CloseCodes.NORMAL_CLOSURE); + } + client.closeSocket(); + } +} diff --git a/test/org/apache/tomcat/websocket/server/TesterWsClient.java b/test/org/apache/tomcat/websocket/server/TesterWsClient.java index e060a7168b..7a5c9c4ee0 100644 --- a/test/org/apache/tomcat/websocket/server/TesterWsClient.java +++ b/test/org/apache/tomcat/websocket/server/TesterWsClient.java @@ -30,10 +30,16 @@ import javax.websocket.CloseReason.CloseCode; public class TesterWsClient { private static final byte[] maskingKey = new byte[] { 0x12, 0x34, 0x56, 0x78 }; + private static final String DEFAULT_KEY_HEADER_VALUE = "OEvAoAKn5jsuqv2/YJ1Wfg=="; private final Socket socket; + private final String keyHeaderValue; public TesterWsClient(String host, int port) throws Exception { + this(host, port, DEFAULT_KEY_HEADER_VALUE); + } + + public TesterWsClient(String host, int port, String keyHeaderValue) throws Exception { this.socket = new Socket(host, port); // Set read timeout in case of failure so test doesn't hang socket.setSoTimeout(2000); @@ -41,6 +47,7 @@ public class TesterWsClient { // TODO: Hoping this causes writes to wait for a TCP ACK for TCP RST // test cases but I'm not sure? socket.setTcpNoDelay(true); + this.keyHeaderValue = keyHeaderValue; } public void httpUpgrade(String path) throws IOException { @@ -65,12 +72,15 @@ public class TesterWsClient { write(createFrame(true, 8, codeBytes)); } - private void readUpgradeResponse() throws IOException { + public int readUpgradeResponse() throws IOException { BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); String line = in.readLine(); + // line expected to be "HTTP/1.1 nnn " + int result = Integer.parseInt(line.substring(9, 12)); while (line != null && !line.isEmpty()) { line = in.readLine(); } + return result; } public void closeSocket() throws IOException { @@ -89,14 +99,14 @@ public class TesterWsClient { socket.close(); } - private void write(byte[] bytes) throws IOException { + public void write(byte[] bytes) throws IOException { socket.getOutputStream().write(bytes); socket.getOutputStream().flush(); } - private static String createUpgradeRequest(String path) { + public String createUpgradeRequest(String path) { String[] upgradeRequestLines = { "GET " + path + " HTTP/1.1", "Connection: Upgrade", "Host: localhost:8080", - "Origin: localhost:8080", "Sec-WebSocket-Key: OEvAoAKn5jsuqv2/YJ1Wfg==", "Sec-WebSocket-Version: 13", + "Origin: localhost:8080", "Sec-WebSocket-Key: " + keyHeaderValue, "Sec-WebSocket-Version: 13", "Upgrade: websocket" }; StringBuffer sb = new StringBuffer(); for (String line : upgradeRequestLines) { diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index fc0075a63a..28cc871d8b 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -139,6 +139,17 @@ </fix> </changelog> </subsection> + <subsection name="WebSocket"> + <changelog> + <fix> + <bug>66548</bug>: Expand the validation of the value of the + <code>Sec-Websocket-Key</code> header in the HTTP upgrade request that + initiates a WebSocket connection. The value is not decoded but it is + checked for the correct length and that only valid characters from the + base64 alphabet are used. (markt) + </fix> + </changelog> + </subsection> <subsection name="Other"> <changelog> <update> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org