826766 - Offline cache (AppCache) enforces a same-scheme restriction that is too restrictive, which causes HSTS to fail

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

<bsmith>: OK. By the way, I think "same scheme" isn't correct either. We should never prevent HTTP manifests from referencing HTTPS content, only we need to prevent TLS-protected (e.g. HTTPS) manifests from referencing non-protected (e.g. HTTP) content, AFAICT

<mayhemer>: bsmith: I think opposite
<mayhemer> bsmith: I've heard that some content doesn't need or cannot be secured when served by cdns
<mayhemer> but the main code and content of the web app can be

<bsmith> mayhemer: Generally, we must allow HTTPS wherever we allow HTTP because of HSTS
<bsmith> For example, I noticed in the offline cache update code, we reject redirects except internal redirects, but then we enforce the same-scheme check
<bsmith> this means that we will cause the HSTS internal redirects to fail

<mayhemer> bsmith: is hsts redirect internal or not?

<bsmith> it is internal

I believe what we need to do is this:

* After we detect that the origin for the AppCache manifest is using HSTS, after we've previously retrieved the AppCache manifest over HTTP, then we need to re-retrieve the AppCache manifest over HTTPS and 

* We need to make sure that no internal redirects from http://X to https://X cause AppCache update to fail.

* We need to make sure that HTTP (external) redirects from http://X to https://X do not fail.

* We should update the spec. to take these special considerations for HSTS into account.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here is (some of) the problematic code:

NS_IMETHODIMP
nsOfflineCacheUpdateItem::AsyncOnChannelRedirect(nsIChannel *aOldChannel,
                                                 nsIChannel *aNewChannel,
                                                 uint32_t aFlags,
                                                 nsIAsyncVerifyRedirectCallback *cb)
{
    if (!(aFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) {
        // Don't allow redirect in case of non-internal redirect and cancel
        // the channel to clean the cache entry.
        LogToConsole("Offline cache manifest failed because an item redirects", this);

        aOldChannel->Cancel(NS_ERROR_ABORT);
        return NS_ERROR_ABORT;
    }

[...]

    nsAutoCString oldScheme;
    mURI->GetScheme(oldScheme);

    bool match;
    if (NS_FAILED(newURI->SchemeIs(oldScheme.get(), &match)) || !match) {
        LOG(("rejected: redirected to a different scheme\n"));
        return NS_ERROR_ABORT;
    }

[...]

We should allow the case where oldScheme is "http" and newScheme is "https".

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Additionally, we implement the part of the spec that says: 'If the resulting absolute URL has a different <scheme> component than the manifest's URL (compared in an ASCII case-insensitive manner), then jump back to the step labeled "start of line".' i.e. we silently skip explicit resources that have a different scheme.

Again, clearly we should prevent HTTPS manifests from referencing non-HTTPS resources, but I am not sure that we should disallow the case where the AppCache manifest scheme is "http" but the referenced resources are "https". Except for things that are implementing same-origin policy, we should (AFAICT) always allow HTTPS to be used anywhere HTTP would be allowed.

(Along these same lines, we were supposed to change the CSP spec to require implementations to allow https://X to be used anywhere that the CSP specifies http://X. I am not sure we actually did that or implemented the change though.)

Comment 3

[Honza Bambas (:mayhemer)]

We should loose the same-scheme rule for explicit entries completely IMO (needs a sec review and spec change).  

For internal redirects allow only http->https change.

I've heard a lot of web developers not be able to serve their active content securely but icons/images/whatever-non-fatal insecurely via a CDN or just with less resource exhaustion from their server.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> We should loose the same-scheme rule for explicit entries completely IMO
> (needs a sec review and spec change).  

I disagree. Our current stance on mixed content is that we are tolerating mixed content for images/audio/video only for backward compatibility. Because we have implemented the same-scheme mechanism for AppCache for a long time (since the beginning?), we don't have a backward compatibility issue here, so we should continue skipping non-HTTPS content referenced by HTTPS-served manifests. (Note that our stance is different than Chrome's, which is more liberal in allowing mixed content, and that's something we should resolve in the websec working group or WHATWG.)

Comment 5

[Ian Melven :imelven]

(In reply to Brian Smith (:bsmith) from comment #2)
>
> (Along these same lines, we were supposed to change the CSP spec to require
> implementations to allow https://X to be used anywhere that the CSP
> specifies http://X. I am not sure we actually did that or implemented the
> change though.)

(off topic, sorry - I don't see this in the latest CSP 1.1 spec : If the source expression has a scheme that is not a case insensitive match for uri-scheme, then return does not match.)

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Slightly off-topic, but let's not forget that we also need to make sure that 1/ developers actually get to see whichever error message are caused by our restriction mechanism; 2/ we document these error messages somewhere.

Comment 7

[Anne (:annevk)]

Is this still relevant? I doubt we want to fix anything for AppCache at this point.

(Note bug 881830 which recently got fixed and seems somewhat related.)