Last Comment Bug 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
: Allocation of string is not checked in WebSockets text frame parsing, which c...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jason Duell [:jduell] (needinfo me)
:
Mentors:
Depends on: 737164
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-04 20:21 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-06-23 09:57 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
v1. check allocation (1.34 KB, patch)
2012-05-11 02:05 PDT, Jason Duell [:jduell] (needinfo me)
brian: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Same patch w/o fallible_t for beta (1.39 KB, patch)
2012-06-20 13:42 PDT, Jason Duell [:jduell] (needinfo me)
brian: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-04 20:21:43 PDT
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?) 2012-04-04 20:24:35 PDT
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 [:jduell] (needinfo me) 2012-05-11 02:05:51 PDT
Created attachment 623072 [details] [diff] [review]
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?) 2012-06-12 14:41:50 PDT
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 [:jduell] (needinfo me) 2012-06-12 15:03:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/496ea1df8876
Comment 5 Jason Duell [:jduell] (needinfo me) 2012-06-12 15:05:19 PDT
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] 2012-06-13 05:58:38 PDT
https://hg.mozilla.org/mozilla-central/rev/496ea1df8876
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-15 15:55:52 PDT
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 [:jduell] (needinfo me) 2012-06-20 12:50:42 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/54b3267d0292

beta patch coming up...
Comment 9 Jason Duell [:jduell] (needinfo me) 2012-06-20 13:42:46 PDT
Created attachment 635046 [details] [diff] [review]
Same patch w/o fallible_t for beta

Just a version w/o using fallible_t, for the beta branch
Comment 10 Jason Duell [:jduell] (needinfo me) 2012-06-20 17:04:02 PDT
Comment on attachment 635046 [details] [diff] [review]
Same patch w/o fallible_t for beta

Beta version of patch.
Comment 11 Jason Duell [:jduell] (needinfo me) 2012-06-22 16:15:29 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/c53e6d97f7eb

Note You need to log in before you can comment on or make changes to this bug.