1491637 - Origin Parser crashes while parsing "about\+home\+[0-9]+\+*"

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Tom Tung [:tt, :ttung]]

After a modification, right now about protocol strip '?' and '#' for its origins. However, the quota manager might support some of them before.

For instance, before that change, I visited both "about:home?1", "about:home#2" and quota manager created "about+home+1" and "about+home+2" folder in my profile. The reason was that QM reckoned the number after '?' and '#' as a port. However, it's interesting that, for example, 'about:home:3' didn't generate "about+home+3" folder in my profile (I guess it's because the URL is not allowed by Necko so that the document haven't been downloaded yet).

We might want to support it to allow some users to keep using it. 

However, before starting to implement, there are a few things I need to investigate.

- It's still not really clear to me, whether we want to support '?number' and '#number' but not ':number' if QM reckons that number as a port number, then most of the port number is followed by ':'.
- I suspect that if that is used by users because about protocol seems to be handled by Gecko internally. Also, about:xxx might not about to be redirected by general origin. If it isn't used by users outside, then I need to know whether there is the code used this way to create "about+xxx+portnumber" in our current code base.

Comment 1

[Tom Tung [:tt, :ttung]]

I will investigate that on Monday.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Tom Tung [:tt] from comment #0)
> We might want to support it to allow some users to keep using it. 

I don't think we do.  I doubt there was any intent in any about protocol implementations to use this as a mechanism for partitioning storage.

Ideally we should have a plan for how these unintentional origins get eliminated from disk.  The simplest approach is as part of an upgrade, but we could also do it during some type of eviction sweep.  (Even under permanent/ there's the real possibility of abandoned data ending up there, so we will need to consider that at some point.)  If you've seen this happen for origins where we store meaningful private data, we should prioritize this, otherwise it's probably only necessary to have a plan.

> - It's still not really clear to me, whether we want to support '?number'
> and '#number' but not ':number' if QM reckons that number as a port number,
> then most of the port number is followed by ':'.

https://tools.ietf.org/html/rfc6454#section-4 basically specifies how to derive an origin.  It's not really canonical for our purposes for internal protocols, but it's a good starting point.  Step 6 there specifies to use the default port if there isn't a port.  (Aside: We've obviously deviated from this in QM, only encoding the port if it's non-default.)

Our about protocol implementation does the following at https://searchfox.org/mozilla-central/rev/7a91966c106a3dde924e040b4e5b72d33eb77bf7/netwerk/protocol/about/nsAboutProtocolHandler.cpp#61
```
NS_IMETHODIMP
nsAboutProtocolHandler::GetDefaultPort(int32_t *result)
{
    *result = -1;        // no port for about: URLs
    return NS_OK;
}
```

Which I think means it's quite reasonable for us to omit the port for about protocols.  And it's also reasonable for us to hard-code this decision based on the scheme.

> - I suspect that if that is used by users because about protocol seems to be
> handled by Gecko internally. Also, about:xxx might not about to be
> redirected by general origin. If it isn't used by users outside, then I need
> to know whether there is the code used this way to create
> "about+xxx+portnumber" in our current code base.

I'm not sure I 100% parsed what you said here, but if I understand the general idea, the answer is that no code is intentionally creating distinct origins for "?foo" or "#foo" (and ":number" should't work, as you note).  When they are used, it's because that's the only way to store state in an about URL.

That's because the about protocol is effectively a redirection service.  When a channel is requested to be created for the about protocol, a lookup is done using XPCOM contract id (after truncating off any "#" or "?" found in the URL) and handing off the channel creation to that code.  You can see the heart of that logic at https://searchfox.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolUtils.h and follow its uses back to nsAboutProtocolHandler.

So if you wanted to have a stateful about URL that can be bookmarked, like about:preferences, you need to use "#" like how we have the "about:preferences#home" URL.  Some code may have also used query strings because our platform and existing JS libraries make it easier to deal with URLSearchParams than parse up the anchor payload.

Comment 3

[:Gijs (he/him)]

(In reply to Andrew Sutherland [:asuth] from comment #2)
> (In reply to Tom Tung [:tt] from comment #0)
> > We might want to support it to allow some users to keep using it. 
> 
> I don't think we do.  I doubt there was any intent in any about protocol
> implementations to use this as a mechanism for partitioning storage.

Yeah, also from a front-end perspective, I don't think anyone wants/needs ports on the about: protocol.

Comment 4

[Jan Varga [:janv]]

Just to be clear, in bug 1422456, I didn't mean to add support for "?", "#" or ":" on the about protocol as something that can be used normally. I just meant to extend the origin parser to support it, so we don't end up with a storage initialization failure when we find such origins on disk. The return value should be ObsoleteOrigin in the case so the origin directory gets removed by the storage directory helper.

Comment 5

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #2)
Thanks for giving feedback and explaining the about protocol from both perspectives of spec and our implementation!

My original thoughts for this issue was to investigate that is there any use case which created "about+xxx+number" in profiles intentionally? If there is, I wanted to provide a temporary support to prevent storage initialization from failure and would remove it in the future minor upgrade as you said.

At that time, I could only imagine that only "about:xxx?number" and "about:xxx#number" would create that kinds of folders in profiles based on our implementation. Also, the reason behind was that the OriginParser reckoned the number as a port because the sanitized string of origin which the OriginParser received was "about+xxx+number" and then it interpreted that number as a port.

Therefore, my plan for this issue was maybe:
1. Provide a support/fix for not breaking the origin parser iff 'about:xxx?number' and 'about:xxx#number' do break the origin parser and the use case existed. (before that I need to confirm/investigate whether the use case exists and does it break the storage initialization)
2. Somehow correct the origin parser to not reckon them as a port because it's wrong to me semantically.
3. Mark those origin as obsolete origins and remove them in the future upgrade.

After dipping deeper, I found:
- Currently, "about+xxx+number" doesn't break the storage initialization and it seems that there are no internal implementation create that intentionlly. However, note that, based on bug 1422456 Comment 10, it was possible to having directories like about+home+1 becase about:home was linkable from web (right now, it's not allowed).
- The case that broke the storage initialization was "about+xxx+number+*". However, since it had broken the intiailzation, the directory wasn't created.

> I don't think we do.  I doubt there was any intent in any about protocol
> implementations to use this as a mechanism for partitioning storage.

I see. Then the use case (1) shouldn't be existed.

> Ideally we should have a plan for how these unintentional origins get
> eliminated from disk.  The simplest approach is as part of an upgrade, but
> we could also do it during some type of eviction sweep.  (Even under
> permanent/ there's the real possibility of abandoned data ending up there,
> so we will need to consider that at some point.)  If you've seen this happen
> for origins where we store meaningful private data, we should prioritize
> this, otherwise it's probably only necessary to have a plan.

Got it! 

> https://tools.ietf.org/html/rfc6454#section-4 basically specifies how to
> derive an origin.  It's not really canonical for our purposes for internal
> protocols, but it's a good starting point.  Step 6 there specifies to use
> the default port if there isn't a port.  (Aside: We've obviously deviated
> from this in QM, only encoding the port if it's non-default.)
> 
> Our about protocol implementation does the following at
> https://searchfox.org/mozilla-central/rev/
> 7a91966c106a3dde924e040b4e5b72d33eb77bf7/netwerk/protocol/about/
> nsAboutProtocolHandler.cpp#61
> ```
> NS_IMETHODIMP
> nsAboutProtocolHandler::GetDefaultPort(int32_t *result)
> {
>     *result = -1;        // no port for about: URLs
>     return NS_OK;
> }
> ```
> 
> Which I think means it's quite reasonable for us to omit the port for about
> protocols.  And it's also reasonable for us to hard-code this decision based
> on the scheme.
 
That's what I was confused for interpreting numbers as port numbers in the origin parser because here the protocol isn't allowed.

> I'm not sure I 100% parsed what you said here, but if I understand the
> general idea, the answer is that no code is intentionally creating distinct
> origins for "?foo" or "#foo" (and ":number" should't work, as you note). 
> When they are used, it's because that's the only way to store state in an
> about URL.
> 
> That's because the about protocol is effectively a redirection service. 
> When a channel is requested to be created for the about protocol, a lookup
> is done using XPCOM contract id (after truncating off any "#" or "?" found
> in the URL) and handing off the channel creation to that code.  You can see
> the heart of that logic at
> https://searchfox.org/mozilla-central/source/netwerk/protocol/about/
> nsAboutProtocolUtils.h and follow its uses back to nsAboutProtocolHandler.
> 
> So if you wanted to have a stateful about URL that can be bookmarked, like
> about:preferences, you need to use "#" like how we have the
> "about:preferences#home" URL.  Some code may have also used query strings
> because our platform and existing JS libraries make it easier to deal with
> URLSearchParams than parse up the anchor payload.

Thanks for explaining that. I'll take more looks at nsAboutProtocolHandler!

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to :Gijs (he/him) from comment #3)
> (In reply to Andrew Sutherland [:asuth] from comment #2)
> > (In reply to Tom Tung [:tt] from comment #0)
> > > We might want to support it to allow some users to keep using it. 
> > 
> > I don't think we do.  I doubt there was any intent in any about protocol
> > implementations to use this as a mechanism for partitioning storage.
> 
> Yeah, also from a front-end perspective, I don't think anyone wants/needs
> ports on the about: protocol.

I see! Thanks for providing a perspective from front-end!

(In reply to Jan Varga [:janv] from comment #4)
> Just to be clear, in bug 1422456, I didn't mean to add support for "?", "#"
> or ":" on the about protocol as something that can be used normally. I just
> meant to extend the origin parser to support it, so we don't end up with a
> storage initialization failure when we find such origins on disk. The return
> value should be ObsoleteOrigin in the case so the origin directory gets
> removed by the storage directory helper.

Thanks for clarifying!
But, because both an existing "about+xxx+numebr" (e.g. about+home+1) or a new sanitized origin in that format (shouldn't have after bug 1422456) don't break the origin parser and use case for internal seems to not exist, I think the only two things I can do here are:
- Maybe not parsing the number in "about+xxx+number" as a port number.
- Mark them as an obsolete origin directory and remove them in the future minor upgrade.

Comment 7

[Jan Varga [:janv]]

(In reply to Tom Tung [:tt] from comment #6)
> (In reply to Jan Varga [:janv] from comment #4)
> > Just to be clear, in bug 1422456, I didn't mean to add support for "?", "#"
> > or ":" on the about protocol as something that can be used normally. I just
> > meant to extend the origin parser to support it, so we don't end up with a
> > storage initialization failure when we find such origins on disk. The return
> > value should be ObsoleteOrigin in the case so the origin directory gets
> > removed by the storage directory helper.
> 
> Thanks for clarifying!
> But, because both an existing "about+xxx+numebr" (e.g. about+home+1) or a
> new sanitized origin in that format (shouldn't have after bug 1422456) don't
> break the origin parser and use case for internal seems to not exist, I
> think the only two things I can do here are:
> - Maybe not parsing the number in "about+xxx+number" as a port number.
> - Mark them as an obsolete origin directory and remove them in the future
> minor upgrade.

I don't think we are safe here. Try this:
1. Open FF 57
2. Type about:home?1:q in the URL bar
3. Verify that FF created storage/default/moz-safe-about+home+1+q origin directory
4. Quit FF
5. Remove storage/default/moz-safe-about+home+1+q/.metadata-v2 file
6. Run FF 57 again
7. Surprise ... crash! (we crash in OriginParser::HandleToken when handling default: in the switch)

If I do step 6 using latest debug nightly I get:
Assertion failure: mSchemeType == eNone, at /Users/varga/Sources/Mozilla0/dom/quota/ActorsParent.cpp:8637

The code may look like we are expecting a port there (for about protocol), but that was not intentional at the time the code was written (since we assumed there can't be anything after about:home string). We just need to teach the origin parser to handle the extra stuff after about:home and treat it as invalid origin.

This can be a serious problem I think, not something we can do in future.

I'm going to hide this bug.

The summary may need an update too.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #7)
> I don't think we are safe here. Try this:
> 1. Open FF 57
> 2. Type about:home?1:q in the URL bar
> 3. Verify that FF created storage/default/moz-safe-about+home+1+q origin
> directory

I overlooked these case because I thought it should hit the MOZ_CRASH() here while the OriginParser is parsing that string [1]. Thanks for catching that!

[1] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#8723

> 4. Quit FF
> 5. Remove storage/default/moz-safe-about+home+1+q/.metadata-v2 file
> 6. Run FF 57 again
> 7. Surprise ... crash! (we crash in OriginParser::HandleToken when handling
> default: in the switch)
> 
> If I do step 6 using latest debug nightly I get:
> Assertion failure: mSchemeType == eNone, at
> /Users/varga/Sources/Mozilla0/dom/quota/ActorsParent.cpp:8637

Comment 9

[Jan Varga [:janv]]

(In reply to Tom Tung [:tt] from comment #8)
> (In reply to Jan Varga [:janv] from comment #7)
> > I don't think we are safe here. Try this:
> > 1. Open FF 57
> > 2. Type about:home?1:q in the URL bar
> > 3. Verify that FF created storage/default/moz-safe-about+home+1+q origin
> > directory
> 
> I overlooked these case because I thought it should hit the MOZ_CRASH() here
> while the OriginParser is parsing that string [1]. Thanks for catching that!
> 

That's what it does, it hits the MOZ_CRASH in release builds.
Origin parser used to be involved only when a metadata file was missing or if we did an upgrade.
The check for origin verification was enabled for release builds only recently (bug 1482812).
But even with the check enabled, we end up with MOZ_CRASH, because origin parser is confused by the extra stuff after about:home string.

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #9)
Thanks! I really learn a lesson today!

> That's what it does, it hits the MOZ_CRASH in release builds.
> Origin parser used to be involved only when a metadata file was missing or
> if we did an upgrade.
> The check for origin verification was enabled for release builds only
> recently (bug 1482812).
> But even with the check enabled, we end up with MOZ_CRASH, because origin
> parser is confused by the extra stuff after about:home string.

Oh, I see. I should have had that context since I implemented that bug.

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: bug1491637.p1.patch

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: bug1491637.p2.patch

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: bug1491637.p3.patch

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1491637 - P1 - Make unparsable about protocol origins obsolete and abort parsing the rest of tokens after the host token;

Jan, this is a fix for making an unexpected "about: protocol" origin as an obsolete origin so that it will be deleted while restoring the directory metadata.

This patch does two things:
1. abort keep parsing the rest of token after the host token for about protocol
2. recognize the error while parsing so that Origin Parser can put the certain origins as obsolete origins.

Note that for (2), instead of having a extra member variable to track different cases of fliping mError flag, this patch simplily marks all "about:" protocol's origins that have errors while parsing tokens with eObsolete. That might make the other failure cases which are not in this issue be obsolete. However, given the "about:" protocol is an internal protocol and there is no any other issue related to "about:" on QM, I think it's fine to do that here.

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1491637 - P2 - Avoid getting metadata files for the same origin after they were removed;

I found this issue while writing a test, but I'm not really sure whether should I put it on this issue or not. However, if I put it in another bug, the test in P3 might be a little strange.

The issue is that GetDirectoryMetadata2WithRestore() didn't expect the given origin might be obsolete. However, the obsolete origin will be removed in RestoreDirectoryMetadata(). That means if the given origin is obsolete, it might fail after restoring, but I don't think we want this happen in all the cases.

This patch makes all the callers for GetDirectoryMetadata2WithRestore() understand if the origin is obsolete (which indicates it was removed or not), and then do the right behavior after that.

There are six places using GetDirectoryMetadata2WithRestore()

- InitializeRespository [1] 
    if it's obsolete origin and the restore happens => bypass that origin

- EnsureOriginIsInitialized for persistent origin [2]
    if it's obsolete origin and the restore happens => return NS_ERROR_FAILURE because the origin is unrecoverable and its directory was removed

- Traversing in GetUsageOp [3]
    if it's obsolete origin and the restore happens => bypass that origin since its directory was removed.

- Deleting files in ClearRequestBaseOp [4]
    if it's obsolete origin and the restore happens => keep original logic 

- Getting persisted status for PersistedOp [5]
    if it's obsolete origin and the restore happens => return with persisted status false and NS_OK

- Getting persisted status for PersistOp [6]
    if it's obsolete origin and the restore happens => return NS_ERROR_FAILURE for not being able to persist the origin

[1] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#4219
[2] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5345
[3] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#7134
[4] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#7644
[5] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#7905
[6] https://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#7991

Comment 16

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1491637 - P3 - Have a test to make sure restoring and initializing origins don't fail for an obsolete origin;

Comment 17

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #16)
> Created attachment 9010304 [details] [diff] [review]
> Bug 1491637 - P3 - Have a test to make sure restoring and initializing
> origins don't fail for an obsolete origin;

I verified that without P1 and P2 the test hit the assertion (Assertion failure: mSchemeType == eNone, at /Users/varga/Sources/Mozilla0/dom/quota/ActorsParent.cpp:8637), but with applying these two it succeeded.

Comment 18

[Andrew McCreight [:mccr8]]

Jan, is this just a safe crash? I'm trying to figure out why you marked this as a security bug so that I can rate it. If it is just a crash, it could be sec-low or maybe unhidden.

Comment 19

[Jan Varga [:janv]]

(In reply to Andrew McCreight [:mccr8] from comment #18)
> Jan, is this just a safe crash? I'm trying to figure out why you marked this
> as a security bug so that I can rate it. If it is just a crash, it could be
> sec-low or maybe unhidden.

It's a safe crash, similar to bug 1422456, so I guess sec-low would be ok.

Comment 20

[Marion Daly [:mdaly]]

Status?

Comment 21

[Tom Tung [:tt, :ttung]]

Waiting for review and feedback, but I guess I can rewrite P3 (test) to make it use async/await pattern.

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: P3 - test

Update test for changing:
- using await/async in the test
- rather than a generic test on OriginParser, making it for "about:xxx" origins only.
- removing an unnecessary zip file

Comment 23

[Tom Tung [:tt, :ttung]]

Andrew, please feel free to give feedback/review these patches if you have time. Thanks! 

The concern for P1 is the same: I'm not so sure about whether it's okay with aborting parsing early if the parser understands the origin is obsolete.

Comment 24

[Tom Tung [:tt, :ttung]]

This should be fixed by the patches for "about:reader", so I'm going to close this.