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

RESOLVED FIXED in Firefox 14

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: jduell)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed)

Details

Attachments

(2 attachments)

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.
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.
Assignee: mcmanus → nobody
(Assignee)

Updated

5 years ago
Assignee: nobody → jduell.mcbugs
(Assignee)

Comment 2

5 years ago
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.
Attachment #623072 - Flags: review?(bsmith)
(Assignee)

Updated

5 years ago
Attachment #623072 - Flags: review?(bsmith) → review?(mcmanus)
Comment on attachment 623072 [details] [diff] [review]
v1. check allocation

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

This will stop a crash.
Attachment #623072 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/496ea1df8876
Target Milestone: --- → mozilla16
(Assignee)

Comment 5

5 years ago
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
Attachment #623072 - Flags: approval-mozilla-aurora?

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/496ea1df8876
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
Attachment #623072 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 8

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

beta patch coming up...
(Assignee)

Comment 9

5 years ago
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
Attachment #635046 - Flags: review?(bsmith)
Attachment #635046 - Flags: review?(bsmith) → review+
(Assignee)

Comment 10

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

Beta version of patch.
Attachment #635046 - Flags: approval-mozilla-beta?
Attachment #635046 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 11

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

Updated

5 years ago
status-firefox14: --- → fixed
status-firefox15: --- → fixed
You need to log in before you can comment on or make changes to this bug.