Potential bug in UTF-8 validation when the UTF-8 sequence crossing frame boundary

[jangko]

@jlokier pointed out to UTF-8 encoding in websocket spec.

https://datatracker.ietf.org/doc/html/rfc6455#section-5.6

Text
The "Payload data" is text data encoded as UTF-8. Note that a particular text frame might include a partial UTF-8 sequence; however, the whole message MUST contain valid UTF-8. Invalid UTF-8 in reassembled messages is handled as described in Section 8.1.

Autobahn have this kind of test cases, but surprisingly, we pass all of them.
Because we don't handle this case specifically, there is still potential for this bug to manifest itself.

The reason we pass autobahn test is we read using big enough buffer to assemble all arrived UTF-8 frames into complete message and thus satisfy the spec condition. but if we are using smaller buffer, behavior will be different.

[jangko]

The following snippet will trigger this bug.

proc test() {.async.} =
    let testData = "12345\xF4\x8F\xBF\xBF12345" # this is a valid UTF-8 string
    proc handle(request: HttpRequest) {.async.} =
      check request.uri.path == "/ws"
      let server = WSServer.new()
      let ws = await server.handleRequest(request)
      let res = await ws.recv(7)

    server = HttpServer.create(
      initTAddress("127.0.0.1:8888"),
      handle,
      flags = {ReuseAddr})
    server.start()

    let client = await WebSocket.connect(
      "127.0.0.1:8888",
      path = "/ws",
      frameSize = 7
    )

    await client.send(testData)
    await waitForClose(client)

waitFor test()

[dryajov]

So, as I understand it, there is no straightforward way of fixing it as of yet?

[jangko]

I will collaborate with @mjfh in an attempt to fix this. but we need to understand the precise interaction between frames, message, and data stream of websocket without leaking it to users.

we already have a state based UTF-8 validator, and I think we can try to integrate it into nim-websock by carefully fits it into frame and message concept of websocket.

the websocket protocol itself can be viewed as a state based algorithm with first, continuation, and final condition. and we can design a UTF-8 validator with reset, validate, and finish state. or something similar to that.

[dryajov]

I'm a bit amused by this issue in general. By forcing the protocol to validate correctness of the incoming stream, it makes streaming that data to the application impossible, quite a shortsighted design. This essentially invalidates streaming for textual data.

This is something that we should fix by extending the api with the corresponding recvText/sendText and move the utf8 validation there. Calling the raw recv should default to reading the data stream as it comes and should not attempt to do any utf8 validation at all - we can consider recv/send as api extension points used only by extensions and other low level protocols, such as the libp2p transport. We can add the corresponding recvBinary/sendBinary just for consistency's sake, but this would be thin wrappers around send/recv.

This is the most flexible and straight forward approach overall and the one I would prefer.

[dryajov]

Btw, streaming is broken in WebSockets all over the place, for example compression. So we're not breaking anything, the protocol is simply not designed with streaming in mind.

[jangko]

the decision to include text encoding such as UTF-8 into the specification itself also a weird decision. it only complicates things unnecessarily. maybe the design committee consist of politicians and not engineers.

[jlokier]

I'm a bit amused by this issue in general. By forcing the protocol to validate correctness of the incoming stream, it makes streaming that data to the application impossible, quite a shortsighted design. This essentially invalidates streaming for textual data.

No, it's fine for streaming UTF-8.

UTF-8 code points are very clearly delimited as 0b11xxxxxx followed by some number of 0b10xxxxxx, and there is a maximum length which is quite short.

This means only a few bytes of state (or preseeding the next buffer) need to be retained between incoming frames when streaming. This part is almost trivial. Then you just validate each adjusted frame for UTF-8 as usual.

[jlokier]

the decision to include text encoding such as UTF-8 into the specification itself also a weird decision. it only complicates things unnecessarily. maybe the design committee consist of politicians and not engineers.

No, the alternative to UTF-8 text is "unspecified text encoding", typically ISO-8859-1 or 8859-15 or KOI8-R CP-1252 or ... Allowing unspecified text encoding in Web protocols had already become a high-complexity problem that resulted in a lot of compatibility problems and security holes with other protocols.

So they standardised on UTF-8 for text fields in web protocols going forwards, because it's the simplest thing that's compatible with ASCII, usable for everything, and unambiguous.

WebSocket was designed to carry short text messages to JavaScript in browsers. That was the use case.

Binary was added later, and in fact binary was objected to for a long time because it wasn't usable in JavaScript, the primary reason for WebSocket to exist.

I would say UTF-8 is almost trivial (when you know it), and every modern language that deals with text input has a function to validate UTF-8 input. Engineers knew at the time of WS design that text encoding (whether in UTF-8 or "unspecified") was a large vector for security vulnerabilities. The simple answer was to specifiy one easy encoding and always parse it when inputting from untrusted sources, so that there's no invalid UTF-8 text inside a running program.

Invalid UTF-8 text inside a running program is a source of many security issues, and even sometimes crashes a program runtime. So validating all external input has become the security best practice. This is also why the definition of valid UTF-8 is a bit strict about some code points; it used to be more flexible.

The UTF-8 choice really did come from engineers. A non-engineer committee would have passed a WS-Charset header or something for "flexibility" and required implementations to understand all the different charsets they might be sent. Be grateful that didn't happen :-)

[jlokier]

we can design a UTF-8 validator with reset, validate, and finish state. or something similar to that.

You can do a UTF-8 state machine, but it's unnecessary. All that is required is to find UTF-8 codepoint start without validating UTF-8, which is simple and fast (check it's not in the range 0x80-0xbf). Then when you have a partial codepoint at the end of a buffer, reduce the buffer length, and save those bytes (maximum 3) for the start of the next buffer instead. You can think of those 3 bytes as the "state", or you can think of it as delimited stream (similar to if you were reading raw text, finding newlines to stream newline-terminated lines to a sink).

Then run the normal UTF-8 validator (without state machine) on the adjusted buffers that are going to be returned.

Alternatively, you can avoid truncating/adjusting the buffers, and return a partial UTF-8 codepoint, but still keep those <= 3 bytes for testing the first codepoint of the next buffer. That will satisfy the requirement of the spec, and would be the correct thing to do for a WebSocket forwarder. But for an API that streams parts of text messages, I would not recommend this approach; it is safer from a security and undefined behaviour perspective to return whole codepoints through an API that is expecting text.

[dryajov]

All that is required is to find UTF-8 codepoint start without validating UTF-8, which is simple and fast (check it's not in the range 0x80-0xbf). Then when you have a partial codepoint at the end of a buffer, reduce the buffer length, and save those bytes (maximum 3) for the start of the next buffer instead. You can think of those 3 bytes as the "state", or you can think of it as delimited stream (similar to if you were reading raw text, finding newlines to stream newline-terminated lines to a sink).

It's even simpler than that. Since the spec explicitly states that frames can carry partial UTF-8 data messages in the frame's data, it doesn't make sense to perform UTF-8 validation unless we've received the entire message. It's possible to validate up to the last valid codepoint, but there is no point and we already allow reading partial frames with the bare recv(). I'm inclined to say that All that is required is to find UTF-8 codepoint start without validating UTF-8, which is simple and fast (check it's not in the range 0x80-0xbf). Then when you have a partial codepoint at the end of a buffer, reduce the buffer length, is performed by the application if so desired.