Implement websockets connection limits

RESOLVED FIXED in mozilla7

Status

()

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

People

(Reporter: dchan, Assigned: mcmanus)

Tracking

({dev-doc-complete})

Trunk
mozilla7
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
The current networking code restricts the maximum number of http connections
network.http.max-connections*
network.http.max-persistent-connections*

Should websockets be included in these counts or should a separate preference be created?

Websocket are kept alive longer than a normal http request which could lead to hitting the max earlier. A malicious website could open thousands of websockets on a client and consume system resources. The impact would be similar to a webpage running an infinite loop but harder to diagnosis since the machine wouldn't appear to run slower, network connections would fail.

Comment 1

6 years ago
fwiw, my opinion is that a separate pref makes the most sense here since websockets are different from regular http connections as outlined in dchan's comment, but are also not necessarily persistent
Keywords: dev-doc-needed

Comment 2

6 years ago
I second Ian's opinion. A separate preference allows you to control the max number of HTTP connections and max number of WebSocket connections independently.

> A malicious website could open thousands of websockets on a client
> and consume system resources.

Bear in mind that such a malicious website would need to host a WebSocket listener capable of handling all these thousands of concurrent connections and be able to transmit enough data to impact the client. And then do that for each concurrent client as well. This is not impossible, or even hugely difficult as there is at least one WebSocket solution out there that can scale like that.

But if all you're trying to achieve is to mess with people's PCs, why go to all that hassle? It's a lot more challenging to do that than to simply write some malicious code and let it do its thing. Therefore since this kind of attack has a higher barrier to entry and little payoff, the probability of it being an issue is low. A separate preference to specify the max number of WebSocket connections will easily deal with this.
(Reporter)

Comment 3

6 years ago
(In reply to comment #2)
> I second Ian's opinion. A separate preference allows you to control the max
> number of HTTP connections and max number of WebSocket connections
> independently.
> 
> > A malicious website could open thousands of websockets on a client
> > and consume system resources.
> 
> Bear in mind that such a malicious website would need to host a WebSocket
> listener capable of handling all these thousands of concurrent connections
> and be able to transmit enough data to impact the client. And then do that
> for each concurrent client as well. This is not impossible, or even hugely
> difficult as there is at least one WebSocket solution out there that can
> scale like that.
> 
> But if all you're trying to achieve is to mess with people's PCs, why go to
> all that hassle? It's a lot more challenging to do that than to simply write
> some malicious code and let it do its thing. Therefore since this kind of
> attack has a higher barrier to entry and little payoff, the probability of
> it being an issue is low. A separate preference to specify the max number of
> WebSocket connections will easily deal with this.

True there are better ways to launch a denial of service on the client machine. I agree that the impact is minimal. A more likely attack is to attack another server which is websockets capable. You visit a malicious page which then opens multiple long lived connections to this server. Ideally the target server would have implemented their own connection controls. I think you were saying that there is already an implementation which could handle this.
(Assignee)

Comment 4

6 years ago
Created attachment 541488 [details] [diff] [review]
websocket maxconnections v1

this patch prevents websockets from hogging the whole connection pool and thereby stalling http. (by default there are about 300 non-http sockets available, this limits us to 200). Given the history of servers themselves subverting low per-server limits (meant to protect those same servers), I don't really see a point in a local limit rather than the global one.

I don't see a reason to promote to aurora but if someone feels strongly then feel free to nom and provide rationale for the drivers.

there is a ridealong fix to nsPrelaodedStream in here.. I've tucked it in here because it can't really be triggered unless you are calling asyncwait(null,0,0,null) (via abortsession()) after the stream has been transferred to websockets but before any read has taken place.. that doesn't seem like it can happen without the rest of the patch.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #541488 - Flags: review?(cbiesinger)
(Assignee)

Comment 5

6 years ago
Created attachment 541756 [details] [diff] [review]
websocket maxconnections v2

the interdiff is just breaking out the nspreloadedstream fix into bug 666997
Attachment #541488 - Attachment is obsolete: true
Attachment #541488 - Flags: review?(cbiesinger)
Attachment #541756 - Flags: review?(cbiesinger)
Comment on attachment 541756 [details] [diff] [review]
websocket maxconnections v2

+    // from then main thread before StartWebsocketData() has completed

then -> the
Attachment #541756 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/43f88c89f4cb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Version: unspecified → Trunk
Updated documentation:

https://developer.mozilla.org/en/WebSockets#Gecko_notes

And mentioned on Firefox 7 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.