1557677 - nsAboutProtocolHandler.cpp should not duplicate safe_for_untrusted_content + make_linkable information

Status: NEW

(Core :: Networking, defect, P3)

Description

[:Gijs (he/him)]

I spent most of my morning chasing this down... I'm trying to fix bug 1556559, and just removing MAKE_LINKABLE from the page definition in nsAboutRedirector.cpp didn't work (that is, other pages could still not be linked from about:license). I was confused, because this is e.g. how about:cache links to about:cache-entry. So I spent a long time debugging and eventually realized that the URL for about:license was still nested, whereas it wasn't for all the other non-linkable content-safe URLs, which breaks the lock-step loop of source/target URIs in checkLoadURI (because the target is not nested and the source is, which is unexpected). So that eventually led me to the additional hardcoded list at https://searchfox.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#55-60 , added recently, in bug 1536744.

I don't know why this duplicates the list - if I had to guess, it's something to do with thread-safety?

Besides maintainability issues, I expect this won't work right if we dynamically add new about: URIs at runtime that have this flag.

I haven't worked out why these changes didn't break https://searchfox.org/mozilla-central/source/caps/tests/mochitest/browser_checkloaduri.js, but that's somewhat unfortunate, too...

Valentin, why aren't we using about: modules to make the determination here? And/or should we just take this as incentive to fix bug 1228118 ? :-(

Comment 1

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

(In reply to :Gijs (he/him) from comment #0)

Valentin, why aren't we using about: modules to make the determination here?

It was done specifically for thread safety. Using the dynamically added about modules is not thread safe when creating URLs OMT, especially considering those can be JS implemented.

We should indeed have a way to make sure IsSafeToLinkForUntrustedContent isn't out of date.
Maybe an assert when registering an about module, we could create a URL for that module, and call the method for that URL, making sure it matches the flags on the module?

And/or should we just take this as incentive to fix bug 1228118 ? :-(

Yes, I think that's the way to go. Having about URLs be regular nsSimpleURIs would be of huge help in the future, if we want to reduce the number of URL parsers in the tree.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Fixing bug 1228118 would not address the basic problem of the "safe-to-link" predicate not being evaluated correctly because it's also hardcoded elsewhere, right?

How important is it to have JS-implemented nsIAboutModule impls? Because if we could avoid doing that, we could just make nsIAboutModule threadsafe and not need the hardcoding.

The other thing we could consider is changing the contract so that we have a set of about: paths that are marked as linkable, registering a new module (on main thread) asks the module for what set of things it wants to make linkable and adds to the set, and we check that set in nsAboutProtocolHandler::CreateNewURI. That would involve locking around access to the set, but that lock should not be contested much, esp. if we use an RWLock, because writes should be extremely rare, right?

Comment 3

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

Fixing bug 1228118 would not address the basic problem of the "safe-to-link" predicate not being evaluated correctly because it's also hardcoded elsewhere, right?

Well, the only hardcoding right now is in https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/netwerk/protocol/about/nsAboutProtocolHandler.cpp#55-60 which is in static bool IsSafeToLinkForUntrustedContent(nsIURI* aURI) which is only called from nsAboutProtocolHandler::CreateNewURI, where it is used specifically to decide whether or not we need a nested URI.

If we stop using nested URIs, presumably that code can all go away. So I think it would address this, because linkable-ness would no longer be represented in the URI itself, you'd need to call getURIFlags() on the about module. But CheckLoadURI does that today, anyway, it just gets it wrong if the nested-ness of the URI is different from what it expects (note: that code might need a few tweaks once we get rid of the nested about: stuff, but hopefully those will simplify it...)

How important is it to have JS-implemented nsIAboutModule impls?

I'm not sure. It's currently in use at least by automated tests, fennec, web-compat system add-on, and normandy on desktop, potentially other things, I didn't do a full audit. So it'd be non-trivial to remove entirely.

Because if we could avoid doing that, we could just make nsIAboutModule threadsafe and not need the hardcoding.

This is where I'm hoping you can tell me that scriptsecuritymanager doesn't pretend to be threadsafe? Because it totally relies on nsiaboutmodule things today, so uh, that'd be awkward...

The other thing we could consider is changing the contract so that we have a set of about: paths that are marked as linkable, registering a new module (on main thread) asks the module for what set of things it wants to make linkable and adds to the set, and we check that set in nsAboutProtocolHandler::CreateNewURI. That would involve locking around access to the set, but that lock should not be contested much, esp. if we use an RWLock, because writes should be extremely rare, right?

I think so, though would "not be contested much" still be true at startup when all these things would likely be registered?

Dumb question, but why do we need a lock if the set can only be modified from the mainthread? That is, could we effectively accept that other threads trying to access the list may race in terms of what entries they get? I'm fairly sure that runtime-added about modules (like normandy, studies, webcompat) don't really care about access to their pages from other threads racing with their code starting up (which is always mainthread).

Comment 4

[Boris Zbarsky [:bzbarsky]]

where it is used specifically to decide whether or not we need a nested URI

Ah, I see.

This is where I'm hoping you can tell me that scriptsecuritymanager doesn't pretend to be threadsafe?

Correct.

I think so, though would "not be contested much" still be true at startup when all these things would likely be registered?

That would all happen on the main thread, so the lock would not be contested much because all the access would be serialized anyway. ;)

Dumb question, but why do we need a lock if the set can only be modified from the mainthread?

Because otherwise a reader on a background thread might read it mid-modification and get garbage data, depending on the exact implementation. It's not just a matter of "does or doesn't get the entry"; it might get an address to read, then the buffer gets resized and realloced before it reads and now it's reading deleted memory or so.