1264117 - Long IFrame "src" attributes cause crash

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Dylan Katz]

Attachment: PoC.html

When creating an iframe with a long "src" attribute, a browser-wide crash can be invoked. A PoC is attached. It's pretty straightforward. This causes the browser to freeze, then crash in my testing. This was discovered whilst resolving an issue with the Electronic Frontier Foundation. If I'm not mistaken, Mozilla does not regard DoS bugs as security issues, so I've set this as public.

Comment 1

[:Gijs (out for now; he/him)]

Can you provide a crashreport ID from about:crashes ?

Comment 2

[Dylan Katz]

(In reply to :Gijs Kruitbosch from comment #1)
> Can you provide a crashreport ID from about:crashes ?

Apologies, it would appear that this does not actually crash the browser, but freezes it, rendering it unusable.

Comment 3

[YF (Yang)]

WFM in Fx45.0.1 & 48.0a1 (2016-04-09) on Win10.

Comment 4

[Loic]

I'm able to reproduce the hard hang. (FF45 WIn 7)

Comment 5

[Anne (:annevk)]

This still happens. The entire browser freezes, even with Fission. Do we have limits on IPC?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Anne (:annevk) from comment #5)

This still happens. The entire browser freezes, even with Fission. Do we have limits on IPC?

We do have size limits on IPC, but this URL isn't long enough to exceed them. I'm guessing the slowness here is caused by the large number of . characters which leads to a large number of subdomains being handled by our network layer. I ran the test under gdb, and randomly paused execution after a few seconds, and it appears we're spending a ton of time in TRRService::IsExcludedFromTRR_unlocked here: https://searchfox.org/mozilla-central/rev/0379f315c75a2875d716b4f5e1a18bf27188f1e6/netwerk/dns/TRRService.cpp#793-828. It appears as though this code does 3 hashtable lookups for each . character in the string, which understandably can become quite slow, especially as we're hashing subsets of the same very long string over and over again.

Given that the hang appears to be happening in necko code, I'll redirect this to the networking component.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: subset of the hang backtrace

Comment 8

[Anne (:annevk)]

Nika, amazing, thank you! Hopefully Valentin can take it from there.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

I think it's time to start enforcing the 253 character limit on hostnames. Not totally sure why we don't do that already.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1264117 - Limit length of hostnames to 253 characters r=nhnt11

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #9)

I think it's time to start enforcing the 253 character limit on hostnames. Not totally sure why we don't do that already.

With this, the method in question will do a maximum of 254*3 hashtable lookups by my quick calculation. I think that effectively prevents a DoS.

I believe a name with consecutive dots (i.e. 0-length labels) is illegal. It looks like it would be pretty trivial to also add a check after we get the next dot's position to check if the char after that is also a dot, and if so, return false early. WDYT?

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #11)

I believe a name with consecutive dots (i.e. 0-length labels) is illegal. It looks like it would be pretty trivial to also add a check after we get the next dot's position to check if the char after that is also a dot, and if so, return false early. WDYT?

Hmm, but I suppose it's weird to have the TRR service worry about valid/invalid domains, and the way the net_IsValidHostName is implemented right now this doesn't look trivial to add it there.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd00d1b7fd67
Limit length of hostnames to 253 characters r=nhnt11,necko-reviewers,dragana

Comment 14

[Atila Butkovits]

Backed out for causing failures on test_dns_service_wrap.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/3eb4bf72e783a7a2c28703c4b421a0271b9179a5

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Iank418ZTpCP9EGx97I5uw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=fd00d1b7fd67f811331acac40cef1c5d301e259d&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Copt%2Cxpcshell%2Ctests%2Ctest-linux1804-64-qr%2Fopt-xpcshell-e10s%2Cx1

Failure log: https://treeherder.mozilla.org/logviewer?job_id=330489138&repo=autoland&lineNumber=3049

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b48f6f8e965
Limit length of hostnames to 253 characters r=nhnt11,necko-reviewers,dragana

Comment 16

[Atila Butkovits]

Backed out for causing failures on test_dns_service.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/92b9fb6c7c07ceef33cfe217c157764bbaf412c7

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=X1WSVG8oRuGTHwlEVTolxQ.0&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cxpcshell%2Ctests%2Cwith%2Cnetworking%2Con%2Csocket%2Cprocess%2Ctest-linux1804-64%2Fdebug-xpcshell-spi-nw-e10s%2Cx4&revision=0b48f6f8e965cb60c6e9d511e9cef5db3f563593

Failiure log: https://treeherder.mozilla.org/logviewer?job_id=330522050&repo=autoland&lineNumber=3266

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fc4b1a0a5d0
Limit length of hostnames to 253 characters r=nhnt11,necko-reviewers,dragana

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7fc4b1a0a5d0

Comment 19

[Andrei Purice]

Can you confirm if this is fixed for you Anne?

Comment 20

[Anne (:annevk)]

I can. The testcase from comment 0 no longer hangs. \o/