1436809 - (CVE-2018-5153) Sending websocket message with text and binary data corrupts binary data

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect, P3)

Description

[DennisF]

Attachment: Test HTML page that reproduces the problem. Edit the server URL to point to a websockets endpoint. Then pick a small text file to send. Debugging messages are displayed to the console.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180205211730

Steps to reproduce:

Sent a websocket message that was composed of text and binary data concatenated into a single blob. To see the problem, text data must be before the binary data.




Actual results:

On Firefox 58+, the binary data gets corrupted. If you open the attached HTML file, and pick a file.  The websocket message that gets sent looks like this:

<header>ееееееееееееееееее

"<header>" is the text part of the message. "eeeeeeee...." is the binary data that should be the content of the file. Each binary byte has been replaced by an "e".


Expected results:

On earlier versions of Firefox (<58) and other browsers (IE & Chrome) the websocket message looks like this:

<header>text file content.

Comment 1

[DennisF]

Other items of note:

I have included a workaround in the attached HTML file. By sending a single byte of the file data before the text data the problem does not appear. Just search for "workaround" and swap in the commented out line.

The blob that I am passing to websocket.send() looks fine, but the data that is sent is corrupted. The attached HTML file prints each byte of the blob (in numeric and character form) to the web browser console before the websocket message is sent.

For a webserver I am using JBoss (Wildfly 10). Using annotations in a Java class I have setup a websockets endpoint that takes the websocket messages and writes them to files. Let me know if this code would be helpful to have.

Comment 2

[Michal Novotny [:michal]]

Attachment: patch

This is a regression caused by bug 1415081. NS_ReadInputStreamToString doesn't read all bytes because the input stream is nsMultiplexInputStream and the second part is stored in SlicedInputStream which doesn't implement ReadSegments. The result is that we go beyond the buffer end and we send a random data from memory to the server.

This patch doesn't fix NS_ReadInputStreamToString, it just adds a check to OutboundMessage::ConvertStreamToString() so we abort the connection when all data wasn't converted.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8964066 [details] [diff] [review]
patch

Review of attachment 8964066 [details] [diff] [review]:
-----------------------------------------------------------------

This seems more likely to be a bug in nsMultiplexInputStream and in SlicedInputStream.

1. for nsMultiplexInputStream the bug is here: https://searchfox.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.cpp#428-430,446 because we don't propagate the ReadSegments() error.

2. For SlicedInputStream, if we need to implement ReadSegments, let's do it.

The proposed patch seems just a way to hide the bug.

Comment 4

[Michal Novotny [:michal]]

(In reply to Andrea Marchesini [:baku] from comment #3)
> 1. for nsMultiplexInputStream the bug is here:
> https://searchfox.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.
> cpp#428-430,446 because we don't propagate the ReadSegments() error.

Although it's not clearly stated in nsIInputStream.idl, the general rule IMHO is that Read/ReadSegments returns success if some data was read. So the code in nsMultiplexInputStream seems to be correct.

> 2. For SlicedInputStream, if we need to implement ReadSegments, let's do it.

Agree, implementing SlicedInputStream::ReadSegments() will fix this bug so that the message will be correctly sent.

> The proposed patch seems just a way to hide the bug.

I don't agree. If we fix SlicedInputStream then nsMultiplexInputStream can fail on other input stream type. If we fix nsMultiplexInputStream then NS_ReadInputStreamToString can read the data from other buggy input stream type. So I think we have to check how many bytes were read by NS_ReadInputStreamToString.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8964066 [details] [diff] [review]
patch

Review of attachment 8964066 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Michal; we really need this check.
We should also try to implement SlicedInputStream::ReadSegments() in a follow-up bug.
I'd also like to see a test for this bug added, so we don't regress in the future.

Comment 6

[Michal Novotny [:michal]]

Attachment: patch v2 - removed checkin comment that tells how to exploit the bug

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The exploit is easy to make.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A test would point to the problem, so we should land some test later.

Which older supported branches are affected by this flaw?
58 and newer versions

If not all supported branches, which bug introduced the flaw?
bug 1415081

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial to backport if needed, but the patch should probably apply cleanly as is.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely, no special testing should be needed.

Comment 7

[Michal Novotny [:michal]]

(In reply to Valentin Gosu [:valentin] from comment #5)
> I'd also like to see a test for this bug added, so we don't regress in the
> future.

The test should probably land together with the fix of SlicedInputStream. In any case, the test should land after this fix is in the release.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We should nominate for beta as well.

I agree that the test should land after a release with the fix ships so as not to 0day us.

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 8964107 [details] [diff] [review]
patch v2 - removed checkin comment that tells how to exploit the bug

Approval Request Comment
[Feature/Bug causing the regression]: bug 1415081
[User impact if declined]: out of bounds read, the data is sent to the server
[Is this code covered by automated tests?]: there is couple of websocket tests
[Has the fix been verified in Nightly?]: no, I tested it manually against a test case
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change
[String changes made/needed]: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b229809d17ff18263becfad6f9777c9ab0165279

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8cf59abf9839

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b229809d17ff

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Michal Novotny (:michal) from comment #9)
> [Is this code covered by automated tests?]: there is couple of websocket
> tests
> [Has the fix been verified in Nightly?]: no, I tested it manually against a
> test case
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Michal's assessment on manual testing needs and the fact that the affected area has automated tests.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #3)
> This seems more likely to be a bug in nsMultiplexInputStream and in
> SlicedInputStream.
> 
> 1. for nsMultiplexInputStream the bug is here:
> https://searchfox.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.
> cpp#428-430,446 because we don't propagate the ReadSegments() error.
> 
> 2. For SlicedInputStream, if we need to implement ReadSegments, let's do it.

We should file a follow-up bug to implement this.
The patch should also MOZ_DIAGNOSTIC_ASSERT that temp->Length() == mLength so we don't just silently ignore this problem in the future.

Comment 15

[Daniel Veditz [:dveditz]]

Was a follow-up bug to fix SlicedInputStream filed? Please add a link to it here (maybe in See Also?)

Comment 16

[Michal Novotny [:michal]]

There is no bug for it yet. I have a patch, but it doesn't help because the underlying stream doesn't implement it either https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/netwerk/base/nsFileStreams.cpp#221

Looks like the best solution here is to use Read instead of ReagSegments at https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/netwerk/base/nsNetUtil.cpp#1631.

Baku, what do you think?

Comment 17

[Andrea Marchesini [:baku]]

In theory, BufferWriter::Write() checks if the stream is buffered: NS_InputStreamIsBuffered(). If that is not the case, NS_NewBufferedInputStream is used to wrap the underlying stream. BufferedStream implements ReadSegments().

But NS_InputStreamIsBuffered() checks if ReadSegments() works  reading 1 byte only. This doesn't work for nsMultiplexInputStream because maybe just 1 of the sub streams doesn't support ReadSegments() and, if that one is not the first stream, NS_InputStreamIsBuffered() will not detect it.

Here my proposal:

1. NS_InputStreamIsBuffered() should QI the inputStream to see if it implements nsIMultiplexInputStream.
2. if it does, it calls NS_InputStreamIsBuffered() on each substream. When one of them fails, it returns false.

Comment 18

[Michal Novotny [:michal]]

(In reply to Andrea Marchesini [:baku] from comment #17)
> 1. NS_InputStreamIsBuffered() should QI the inputStream to see if it
> implements nsIMultiplexInputStream.
> 2. if it does, it calls NS_InputStreamIsBuffered() on each substream. When
> one of them fails, it returns false.

This won't work when nsMultiplexInputStream is hidden behind some other stream. Let's say I'll land the ReadSegments implementation of SlicedInputStream, then we can have something like this:

sliced stream -> muxed stream -> ( string stream + file input stream )

This passes the test but fails to read.

Comment 19

[Andrea Marchesini [:baku]]

Right. Probably the best way to do this is to introduce a new interface and use it to know if the stream supports ReadSegments(). The cleanest way would be to move ReadSegments() in a separate interface. Each 'meta' stream should expose such interface only if the underlying stream(s) supports that too.

A similar approach has been done for nsIAsyncInputStream and other interfaces in SlicedInputStream, Multiplex, MIME and so on.

Comment 20

[Jason Duell]

There are a lot of stream interfaces--we're wondering if we could just hack around this for now by detecting the failure in bufferwriter when we try to readSeg, then fall back to seeking the streams back to 0 and using the slow Read() path (i.e. read w/o using ReadSegements).

Baku, does that hack sound OK?

Comment 21

[Andrea Marchesini [:baku]]

Let's file a separate bug: 1460561