1266789 - Hang when loading URL with hundreds of subdomains

I can't reproduce this on either OS X or Windows. From the screencast, it seems you have lots of add-ons installed. Can you try if you can reproduce the issue in safe mode ( https://support.mozilla.org/en-US/kb/troubleshoot-and-diagnose-firefox-problems#w_3-restart-firefox-in-safe-mode ) ?

Group: firefox-core-security

Flags: needinfo?(mohitrawat08)

Keywords: csectype-dos, hang, sec-low

Mohit

Reporter

Comment 2

•

9 years ago

To reproduce the issue in the safe mode kindly increase the length of the URL. To check the issue visit http://factainment.xyz/poc/ and click the "Click here" hyperlink. As you open the link,your browser stops responding.

Ryan VanderMeulen [:RyanVM]

Comment 3

•

9 years ago

Caught this in Visual Studio: xul.dll!nsTHashtable<nsCStringHashKey>::s_HashKey(const void * aKey) Line 373 C++ xul.dll!mozilla::DataStorage::Get(const nsCString & aKey, mozilla::DataStorageType aType) Line 442 C++ xul.dll!nsSiteSecurityService::IsSecureHost(unsigned int aType, const char * aHost, unsigned int aFlags, bool * aResult) Line 1032 C++ xul.dll!nsSiteSecurityService::IsSecureURI(unsigned int aType, nsIURI * aURI, unsigned int aFlags, bool * aResult) Line 895 C++ xul.dll!mozilla::net::nsHttpHandler::SpeculativeConnectInternal(nsIURI * aURI, nsIInterfaceRequestor * aCallbacks, bool anonymous) Line 2236 C++ xul.dll!IOServiceProxyCallback::OnProxyAvailable(nsICancelable * request, nsIChannel * channel, nsIProxyInfo * pi, nsresult status) Line 1890 C++ xul.dll!nsAsyncResolveRequest::DoCallback() Line 253 C++ xul.dll!nsAsyncResolveRequest::OnQueryComplete(nsresult status, const nsCString & pacString, const nsCString & newPACURL) Line 215 C++ xul.dll!ExecuteCallback::Run() Line 87 C++ xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 995 C++ xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 290 C++ xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 98 C++ xul.dll!MessageLoop::Run() Line 204 C++ xul.dll!nsBaseAppShell::Run() Line 158 C++ xul.dll!nsAppShell::Run() Line 262 C++ xul.dll!nsAppStartup::Run() Line 285 C++ xul.dll!XREMain::XRE_mainRun() Line 4347 C++ xul.dll!XREMain::XRE_main(int argc, char * * argv, const nsXREAppData * aAppData) Line 4451 C++ xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData, unsigned int aFlags) Line 4559 C++ firefox.exe!do_main(int argc, char * * argv, char * * envp, nsIFile * xreDirectory) Line 220 C++ firefox.exe!NS_internal_main(int argc, char * * argv, char * * envp) Line 362 C++ firefox.exe!wmain(int argc, wchar_t * * argv) Line 138 C++ Build was off m-c rev https://hg.mozilla.org/mozilla-central/rev/9ce31e9f90cb0e534611b0f617c5bbc232ffe748

Ryan VanderMeulen [:RyanVM]

Updated

•

9 years ago

Component: Untriaged → Networking

Product: Firefox → Core

Version: 45 Branch → unspecified

Liviu Cirdei

Comment 4

•

9 years ago

Reproduced with the steps from comment 2 on Firefox 45.0.2, Firefox 46 and Nightly 49 (Build ID 20160426044609) using Windows 8.1 and Ubuntu 15.04.

Status: UNCONFIRMED → NEW

Ever confirmed: true

Ryan VanderMeulen [:RyanVM]

Comment 5

•

9 years ago

Also worth noting that this hangs the parent process on trunk.

Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}

Comment 6

•

9 years ago

I am seeing a few thousand calls to nsPermissionManager::GetPermissionHashKey(nsIPrincipal*, unsigned int, bool) on the stack. I assume this happens because we check permissions for each individual subdomain.

Flags: needinfo?(michael)

Summary: browser hangs with crafted url → Hang when loading URL with hundreds of subdomains

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

Comment 7

•

9 years ago

I've profiled the error, and cleopatra shows the hang occuring within nsSiteSecurityService::IsSecureHost. This method performs a hash table lookup for every segment in the input URL. The time appears to be dominated by the sheer number of hashtable lookups. I'm not sure what change could be made to that method to avoid the performance problem caused by these hashtable lookups. nsPermissionManager::GetPermissionHashKey also performs a large number of hashtable lookups, as it has to check for permissions on every subdomain. I've thrown together a patch which removes some of the non-hashtable work from GetPermissionHashKey's loop, and I'll attach it, but it's not a solution to the whole problem (and I can't even tell if it improves the situation, as even mousing over the link causes the hang due to the predictor calling IsSecureHost). cleopatra link: https://cleopatra.io/#report=c156acba9a0ff147596b2fca32ca657910b584f9

Flags: needinfo?(michael)

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

Comment 8

•

9 years ago

Attached patch Simplify nsPermissionManager::GetPermissionHashKey loop — Details — Splinter Review

This is the patch (which doesn't fix the problem, but I figured I should attach it in case it becomes useful later) which hopefully will improve the performance of the GetPermissionHashKey loop.

Patrick McManus [:mcmanus]

Updated

•

9 years ago

Whiteboard: [necko-backlog]

Mohit

Reporter

Comment 9

•

9 years ago

Hi, Is there any update regarding the issue?

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

Comment 10

•

9 years ago

:keeler, it looks like you have touched code near the problematic hashtable lookups in nsSiteSecurityService::IsSecureHost before. Any idea as to what we can do to fix this?

Flags: needinfo?(dkeeler)

Dana Keeler (she/her) [:keeler]

Comment 11

•

9 years ago

Well, we're never going to actually store something in that table with a key longer than 256 characters, so we could just check for that and early-return.

Flags: needinfo?(dkeeler)

Mohit

Reporter

Comment 12

•

9 years ago

Hi, Is there any update regarding the issue?

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

Comment 13

•

9 years ago

Attached patch Don't try to look up subdomains which have keys longer than MAX_KEY_LENGTH in nsSiteSecurityService (obsolete) — Details — Splinter Review

I have had this patch sitting around forever, and for some reason I just stumbed across this bug again, and realized I hadn't submitted this patch yet. I'm going to do that now...

Attachment #8779829 - Flags: review?(dkeeler)

Dana Keeler (she/her) [:keeler]

Comment 14

•

9 years ago

Comment on attachment 8779829 [details] [diff] [review] Don't try to look up subdomains which have keys longer than MAX_KEY_LENGTH in nsSiteSecurityService Review of attachment 8779829 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. This is the right idea, but I think the details need to be fixed (unless I'm misunderstanding this code). Also, it would be good to have some tests for this (particularly for the scenario I mention below) - you can work off of existing tests in https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/ ::: security/manager/ssl/DataStorage.h @@ +91,5 @@ > > public: > + // The maxumum length of the key passed to a function storing or retrieving > + // data from this structure. Validated by DataStorage::ValidateKeyAndValue. > + enum { MAX_KEY_LENGTH = 256 }; Maybe let's just make this a static const uint32_t? Also, let's update any documentation in this file or DataStorage.cpp that references "256" to say "MAX_KEY_LENGTH (currently 256)" or something. ::: security/manager/ssl/nsSiteSecurityService.cpp @@ +1029,5 @@ > const char *subdomain; > > uint32_t offset = 0; > + // If the host is too long, we can save some processing time by not looking at > + // the subdomains which are longer than MAX_KEY_LENGTH characters, as they won't be found nit: keep lines <80 characters, please @@ +1032,5 @@ > + // If the host is too long, we can save some processing time by not looking at > + // the subdomains which are longer than MAX_KEY_LENGTH characters, as they won't be found > + // within our DataStorage. > + if (host.Length() > DataStorage::MAX_KEY_LENGTH) { > + offset = DataStorage::MAX_KEY_LENGTH; I'm not sure this is correct. Imagine host is 300 characters long. This will make the code start looking for ancestor domains 256 characters into host (so domains less than 300 - 256 = 44 characters long), and we could potentially miss an ancestor domain that is less than 256 characters but also longer than 44 characters. I think what we want here is `offset = host.Length() - DataStorage::MAX_KEY_LENGTH`, right? Also, let's avoid the "subdomain" terminology in comments, since we're really looking for ancestor domains (the name of the variable pointing to the host data is perhaps not the best).

Attachment #8779829 - Flags: review?(dkeeler) → review-

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

Comment 15

•

9 years ago

Attached patch Don't try to look up subdomains which have keys longer than MAX_KEY_LENGTH in nsSiteSecurityService — Details — Splinter Review

Attachment #8780255 - Flags: review?(dkeeler)

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

Updated

•

9 years ago

Attachment #8779829 - Attachment is obsolete: true

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

Updated

•

9 years ago

Assignee: nobody → michael

Dana Keeler (she/her) [:keeler]

Comment 16

•

9 years ago

Comment on attachment 8780255 [details] [diff] [review] Don't try to look up subdomains which have keys longer than MAX_KEY_LENGTH in nsSiteSecurityService Review of attachment 8780255 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but it still needs some xpcshell tests. Let me know if some pointers on that would be helpful. ::: security/manager/ssl/nsSiteSecurityService.cpp @@ +1012,5 @@ > + // If the host is too long, we can save some processing time by not looking at > + // the ancestor domains which are longer than MAX_KEY_LENGTH characters, as > + // they won't be found within our DataStorage. > + if (host.Length() > DataStorage::MAX_KEY_LENGTH) { > + offset = host.Length() - DataStorage::MAX_KEY_LENGTH; We should document that this is more of a heuristic than an exact optimization because when we actually turn the ancestor domain into a lookup key, we append ":HSTS" or ":HPKP" onto it. I don't think it's worth the added complexity to take this into account in code.

Attachment #8780255 - Flags: review?(dkeeler)

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

Comment 17

•

9 years ago

Unfortunately, I don't know enough about how this component works to come up with a coherent strategy for testing the new changes, other than generating a link with 1000+ subdomains and mousing over / clicking on it, timing whether or not the system becomes unresponsive. i'd love some pointers as to how I can verify that this doesn't change existing behavior.

Flags: needinfo?(dkeeler)

Dana Keeler (she/her) [:keeler]

Comment 18

•

9 years ago

Well, I don't know of a way to deterministically test that the hang has been made less severe (note that we may still have ~128 lookups if the domain is something like a.a.a.a.a.a.....), but it would be good to ensure that if there's a domain in storage that's just at the length limit, a subdomain is properly identified as HSTS when we do a lookup on it. https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_sts_fqdn.js might be a helpful starting point. Another thing to do would be to manually verify that this change makes a difference in practice.

Flags: needinfo?(dkeeler)

Firefox Bug Husbandry Bot

Comment 19

•

8 years ago

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Priority: -- → P1

Firefox Bug Husbandry Bot

Comment 20

•

8 years ago

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Priority: P1 → P3

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

Comment 21

•

6 years ago

Unassigning as I don't have time to work on this.

Assignee: nika → nobody

Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}

Updated

•

6 years ago

Whiteboard: [necko-backlog] → [necko-backlog][has-patch][needs-tests]

BugBot [:suhaib / :marco/ :calixte]

Comment 22

•

3 years ago

Redirect a needinfo that is pending on an inactive user to the triage owner.
:dragana, since the bug has high severity, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mohitrawat08) → needinfo?(dd.mozilla)

Dragana Damjanovic [:dragana]

Updated

•

3 years ago

Flags: needinfo?(dd.mozilla)

BugBot [:suhaib / :marco/ :calixte]

Comment 23

•

3 years ago

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: critical → --

Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}

Comment 24

•

3 years ago

I don't think this happens anymore. I couldn't reproduce it with comment 0, and comment 2 doesn't work anymore.
I'm going to leave this open for now.

Severity: -- → S3

Firefox.avi 9 years ago Mohit 883.50 KB, video/avi		Details
Simplify nsPermissionManager::GetPermissionHashKey loop 9 years ago Nika Layzell [:nika] (ni? for response) 7.57 KB, patch		Details \| Diff \| Splinter Review
Don't try to look up subdomains which have keys longer than MAX_KEY_LENGTH in nsSiteSecurityService 9 years ago Nika Layzell [:nika] (ni? for response) 2.90 KB, patch	keeler : review-	Details \| Diff \| Splinter Review
Don't try to look up subdomains which have keys longer than MAX_KEY_LENGTH in nsSiteSecurityService 9 years ago Nika Layzell [:nika] (ni? for response) 4.31 KB, patch		Details \| Diff \| Splinter Review