742614 - Allocation of string is not checked in WebSockets text frame parsing, which can lead to data being truncated before being returned to the application

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See http://hg.mozilla.org/mozilla-central/annotate/ed9cbe6a817e/netwerk/protocol/websocket/WebSocketChannel.cpp#l1065

We attempt to create an nsCString of the given length, which may be VERY large (up to 2^31, AFAICT). After we create the string, we don't check the length of the string to make sure it succeeded. Consequently, when we call CallOnMessageAvailable, we may have truncated the data to "". 

And, CallOnMessageAvailable makes a copy of that string, and that copying has the same problem. 

BTW, does this mean that, once we have infallable strings (bug 737164), then any server can instantly crash Firefox with an OOM by sending a single very large text frame--i.e. a ping of death? I know that we generally accept that a server can cause us OOM accidentally or on purpose, but this seems a little too easy.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

BTW, I assigned this to Patrick because I originally thought it was a security bug caused by his checkin (based on hg blame), but AFAICT it isn't directly a security bug, so I am unassigning it.

Comment 2

[Jason Duell]

Attachment: v1. check allocation

Thanks for noticing this.   Patch uses new fallible_t String API that landed in bug 737164.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 623072 [details] [diff] [review]
v1. check allocation

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

This will stop a crash.

Comment 4

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/496ea1df8876

Comment 5

[Jason Duell]

Comment on attachment 623072 [details] [diff] [review]
v1. check allocation

One liner allocation check that prevents large websocket messages from crashing browser.

I can port to beta too if we want, but will need different patch since this one relies on bug 737164.  Let me know.

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky): ~0%
String or UUID changes made by this patch: none

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/496ea1df8876

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 623072 [details] [diff] [review]
v1. check allocation

[Triage comment]
Looks good, feel free to nominate a patch for beta and we'll approve it.

Comment 8

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54b3267d0292

beta patch coming up...

Comment 9

[Jason Duell]

Attachment: Same patch w/o fallible_t for beta

Just a version w/o using fallible_t, for the beta branch

Comment 10

[Jason Duell]

Comment on attachment 635046 [details] [diff] [review]
Same patch w/o fallible_t for beta

Beta version of patch.

Comment 11

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-beta/rev/c53e6d97f7eb