Bug 66548 - Tomcat does not validate value of Sec-Websocket-Key header
Summary: Tomcat does not validate value of Sec-Websocket-Key header
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: 9.0.73
Hardware: All All
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-30 19:44 UTC by dan.rabe
Modified: 2023-05-24 10:32 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description dan.rabe 2023-03-30 19:44:48 UTC
In the websocket RFC (https://www.rfc-editor.org/rfc/rfc6455#section-4.1) we read:

The request MUST include a header field with the name
        |Sec-WebSocket-Key|.  The value of this header field MUST be a
        nonce consisting of a randomly selected 16-byte value that has
        been base64-encoded (see Section 4 of [RFC4648]).  The nonce
        MUST be selected randomly for each connection.

Tomcat appears to accept any value for Sec-WebSocket-Key - even if it's not a base64 string, and even if it's not the correct length.

I don't think this causes any functional or security issues, but since the WebSocket spec is worded pretty strongly ("MUST"), I think it would make sense for Tomcat to throw an exception if the Sec-WebSocket-Key header does not meet this requirement.
Comment 1 Christopher Schultz 2023-03-30 20:08:35 UTC
Seems reasonable.

Care you provide a patch/PR?
Comment 2 Mark Thomas 2023-03-31 00:18:11 UTC
Throwing an exception isn't appropriate here. Just returning SC_BAD_REQUEST is sufficient.

I'll note that RFC 6455 also states:

"It is not necessary for the server to base64-decode the |Sec-WebSocket-Key| value."

Which begs the question exactly how far should the server go to validate this value? Possible tests:
a) length of 24 characters
b) ends with "=="
c) characters 0 to 21 are valid for use in base64

Or just decode and check the length despite RFC 6455 saying it is unnecessary.

I think you either do a) + b) or do the full decode. c)
Comment 3 Mark Thomas 2023-03-31 00:21:52 UTC
Sorry, comment was posted while incomplete. Continuing...

The changes required for c) are such that it would be simpler just to do the decode.

I'd lean towards the a) + b) approach but have no objection to the decode.
Comment 4 Remy Maucherat 2023-03-31 09:49:50 UTC
This could break things if someone we find out that clients have been using random values there instead of faithfully implementing the specification.
Comment 5 Mark Thomas 2023-03-31 16:54:48 UTC
Yes, but.

Historically, we have been generally unsympathetic to clients that don't follow the relevant specs.

Given there doesn't seem to be any advantage for the server here (the feature benefits clients) maybe just add the validation in 11.0.x?

I am happy for it to be added to all versions but if folks are concerned about breakage then I'm fine with not back-porting.
Comment 6 Christopher Schultz 2023-03-31 16:59:15 UTC
(In reply to Remy Maucherat from comment #4)
> This could break things if someone we find out that clients have been using
> random values there instead of faithfully implementing the specification.

This is an RFC-MUST situation, so I think it's okay to enforce it.

Maybe we implement this in Tomcat 11 and 10.1 but wait to back-port to 9.0/8.5 until after a few releases to see if we break anything.

(In reply to Mark Thomas from comment #3)
> The changes required for c) are such that it would be simpler just to do the
> decode.

One point in favor of "validating" the string without decoding it is avoiding the buffer-allocation that would come along with decoding string->bytes.
Comment 7 Mark Thomas 2023-04-11 07:20:33 UTC
Fixed in:
- 11.0.x for 11.0.0-M5 onwards
- 10.1.x for 10.1.8 onwards

As per schultz's suggestion, I am going to wait at least one release cycle before back-porting this to 9.0.x and 8.5.x in case there are clients out there that do not provide a valid value for this header.
Comment 8 Mark Thomas 2023-04-11 07:22:10 UTC
Waiting for (lack of) feedback from 11.0.x and 10.1.x users.
Comment 9 Mark Thomas 2023-05-24 10:21:23 UTC
It has been two release cycles with non negative feedback so I am going to proceed with the back-port.
Comment 10 Mark Thomas 2023-05-24 10:32:39 UTC
Fixed in:
- 9.0.76 onwards
- 8.5.90 onwards