Open Bug 1520489 Opened 2 years ago Updated 2 years ago

Setting cookies in a loop renders browser unresponsive

Categories

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

defect

Tracking

()

Tracking Status
firefox64 --- affected
firefox65 --- ?
firefox66 --- ?

People

(Reporter: CuPcakeN1njA, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-dos, Whiteboard: [reporter-external] [client-bounty-form] [verif?][necko-triaged])

Attachments

(1 file)

Attached file POC.html

I discovered this issue whilst experimenting with infinite loops in Javascript. It has come to my attention that you are able to crash(exploitable crash) the latest version of firefox with a very simple infinite loop. By using an infinite loop and attempting to reset a cookie value firefox Quantum becomes un responsive and results in a crash report.

The javascript that has this effect can be seen bellow.
<script>
while(true){
document.cookie="POC=POC;";
}
</script>

My operating system is Windows 10 Enterprise 64 Bit.
Firefox version 64.0.2 (32-bit)

Flags: sec-bounty?
Blocks: eviltraps
Summary: Exploitable crash in Firefox Quantum → Setting cookies in a loop renders browser unresponsive

Hmm I'm pretty sure I knew about this already, I just can't find the dupe right now. Hence leaving this open.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csectype-dos
Priority: -- → P3

(In reply to CuPcakeN1njA from comment #0)

you are able to crash(exploitable crash)

Do you have any evidence of exploitability here? stack traces, a crash-stats report?

Flags: needinfo?(CuPcakeN1njA)

My apologies, when I submitted the report I hadn't read over what other reports meant by "exploitable" and I had misinterpreted it's meaning. Therefore I do not believe this is exploitable.

I have linked a crash report anyway: https://crash-stats.mozilla.com/report/index/11a95b2b-8a64-4eec-92ef-25d7e0190117

Flags: needinfo?(CuPcakeN1njA)
Group: firefox-core-security
Component: Security → Networking: Cookies
Product: Firefox → Core

This crash when creating a ipc message. In this case, the scrip is producing too many SendSetCookie messages and cookie service is not fast enough to consume them. The same problem can be produce with other means as well.

Can we look at a more common solution. Should we flow control the ipc buffer? We do a flow control of OnDataAvailable messages for example.

Flags: needinfo?(nfroyd)

Without having a lot of in-depth knowledge of IPC, I'm reasonably sure that flow control for IPC as a general solution would help resolve a bunch of open evil traps bugs (like maybe bug 1438214?).

Sorry, many ignorant questions incoming.

(In reply to Dragana Damjanovic [:dragana] from comment #4)

This crash when creating a ipc message. In this case, the scrip is producing too many SendSetCookie messages and cookie service is not fast enough to consume them. The same problem can be produce with other means as well.

Are we running into buffering problems at the actual OS-specific IPC layer (blocking the I/O thread on sendmsg() or the like) or are we just repeatedly calling SendSetCookieString, and we're serializing everything into IPC messages, and then we're queueing all those IPC messages, and that causes us to run out of RAM? I'm assuming we're doing the latter?

Is it possible for the cookie service ipc parts to coalesce cookie sets somehow? Like here:

https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieServiceChild.cpp#570-575

since we're already sending that message asynchronously, couldn't we queue up a runnable to do the actual SendSetCookieString, and then repeatedly update that runnable with the most recent cookie set, until at some point the runnable sends off the latest cookie set, and then we start with a new runnable? I'm not exactly sure where that runnable should run, though...

Can we look at a more common solution. Should we flow control the ipc buffer? We do a flow control of OnDataAvailable messages for example.

I didn't know we flow controlled OnDataAvailable! I'd like to learn about this; where do we do that?

Assuming the above scenario is correct--we're creating so many IPC messages that we run out of memory before we can send them all out--I think this is proposing something like blocking during IPC message creation until the IPC queue is an appropriate size? Or I guess we could always permit IPC messages to be created, but then when they got inserted into the queue, we'd block if the queue had some N MB of messages already pending, or something like that?

Assuming I'm understanding correctly, that sounds kind of reasonable! It feels like the better thing is to investigate whether we can improve the cookie service in some way, but I don't know whether that's just my reluctance to touch the IPC layer, a drastic misunderstanding of how the cookie service works, or something else entirely.

Flags: needinfo?(nfroyd)

Backpressure by blocking seems like not the best idea, because it would block the entire actor thread, and in the worst case there could be deadlock from two or more processes all waiting for someone else to start handling messages.

We do have something that might help here: the compress and compressall keywords in IPDL, where if there are several pending instances of the same message for the same actor (back-to-back for compress, or anywhere in the queue for compressall), only the last will be handled. This was meant for state updates like mouse position (bug 1130051).

One problem is that, looking at the implementation, compression is implemented on the receiving side only. (Also, compressall is accidentally quadratic.) But that could be changed.

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #7)

(Also, compressall is accidentally quadratic.)

Filed bug 1522863 for this. I'm not sure compressall would help us here unless we made the duplication condition more flexible. But the cookie service itself could implement something roughly equivalent?

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][necko-triaged]

denial of service bugs are excluded from the bug bounty program

Flags: sec-bounty? → sec-bounty-

I understand this doesn't qualify for a bounty however do you know if this issue will be fixed?

See Also: → 1588509
You need to log in before you can comment on or make changes to this bug.