794507 - Valid HTTPS content is displayed as broken HTTPS (globe icon instead of lock) because offline cache does not save securityInfo of entries

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P1)

Description

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

Consequences:

1. We do not necessarily always show the correct security indicator (especially for EV and maybe for mixed content).

2. We cannot re-validate the certificate of entries from the offline cache since we do not have the cert.

Comment 1

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

When this bug is fixed, the patch in bug 788365 should be backed out.

Comment 2

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

It seems like the result of this bug is that any HTTPS AppCache-based site is indistinguishable from a site that has broken SSL (e.g. mixed content).

On the B2G phone we show a crossed-out lock icon in the address bar instead of just the globe like we do on desktop. This makes HTTPS sites that use AppCache look like they're doing something wrong--including, in particular, the Mozilla Marketplace. (I haven't actually verified that this is the reason that the broken lock is shown for the Mozilla Marketplace when AppCache is enabled for it, but it seems very likely.)

Comment 3

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

Honza and Michal, do you have time to work on this? Basically we need to serialize/deserialize the securityInfo for each cached page in the offline cache just like we do for the normal cache.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Damn, I'm fast!

Comment 5

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

Comment on attachment 704743 [details] [diff] [review]
v1

Review of attachment 704743 [details] [diff] [review]:
-----------------------------------------------------------------

Honza *is* fast!

Note there is also this code in nsHttpChannel.cpp:

        // XXX: We should not be skilling this check in the offline cache
        // case, but we have to do so now to work around bug 794507.
        MOZ_ASSERT(mCachedSecurityInfo || mLoadedFromApplicationCache);
        if (!mCachedSecurityInfo && !mLoadedFromApplicationCache) {
            LOG(("mCacheEntry->GetSecurityInfo returned success but did not "
                 "return the security info [channel=%p, entry=%p]",
                 this, mCacheEntry.get()));
            return NS_ERROR_UNEXPECTED; // XXX error code
        }

Any suggestions about how to handle previously-offline-cached data (data that was cached before this fix).

The proposed fix looks very simple so we should uplift it to aurora and beta after it's been on central for a week.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #5)
> Any suggestions about how to handle previously-offline-cached data (data
> that was cached before this fix).

May be a bit drastic, but we could delete manifests loaded via https those don't have the security-info metadata from cache to force update.  Since all items must have same scheme or same origin, we will reload only what we have to reload.

On the other hand, this code (in comment 5) is harmless, so we can remove it in some very future version.  Then, we should log to some console that the cache is obsolete or do the previously suggested thing.

Comment 7

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

(In reply to Honza Bambas (:mayhemer) from comment #6)
> May be a bit drastic, but we could delete manifests loaded via https those
> don't have the security-info metadata from cache to force update.  Since all
> items must have same scheme or same origin, we will reload only what we have
> to reload.

How about we change the code for conditional requests so that we never add the conditional request headers if we're doing an offline cache update and the existing entry doesn't have a security-info entry?

Also, when we fetch the cache manifest, if the cache manifest itself doesn't have security-info then we should consider the cache manifest as having been updated, to force a refresh of the app.

> On the other hand, this code (in comment 5) is harmless, so we can remove it
> in some very future version.  Then, we should log to some console that the
> cache is obsolete or do the previously suggested thing.

I agree that it doesn't need to be removed right away and probably can't be, to avoid breaking things. But, the other things I suggest above are very important to do now.

Comment 8

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

(In reply to Lee Farrell from bug 826309 comment #10)
> We depend heavily on the appcache, so we cannot dump it. 
> 
> Our customers are suspicious that the lock does not show. 
> It the bug to be fixed do you know?

Lee, this bug is the bug where we are working on the fix for the issue that you described in bug 826309. It seems like we're close to having a solution.

In comment 6, Honza wrote:
> Since all items must have same scheme or same origin, we will reload only what we have to reload.

according to our investigation in bug 826309, there is no same-origin restriction, right?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > May be a bit drastic, but we could delete manifests loaded via https those
> > don't have the security-info metadata from cache to force update.  Since all
> > items must have same scheme or same origin, we will reload only what we have
> > to reload.
> 
> How about we change the code for conditional requests so that we never add
> the conditional request headers if we're doing an offline cache update and
> the existing entry doesn't have a security-info entry?

That seems effectively identical to my proposal.  I want to let the cache reload on access.  Deleting or artificial invalidating is the same actually.  However, for the manifest we also have to remove the "hash" metadata to prevent seeing the manifest identical even when revalidated.

> 
> Also, when we fetch the cache manifest, if the cache manifest itself doesn't
> have security-info then we should consider the cache manifest as having been
> updated, to force a refresh of the app.
> 
> > On the other hand, this code (in comment 5) is harmless, so we can remove it
> > in some very future version.  Then, we should log to some console that the
> > cache is obsolete or do the previously suggested thing.
> 
> I agree that it doesn't need to be removed right away and probably can't be,
> to avoid breaking things. But, the other things I suggest above are very
> important to do now.

Reported bug 833834 on that.

Comment 10

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

Comment on attachment 704743 [details] [diff] [review]
v1

> Reported bug 833834 on that.

Great. I think it makes sense to split bug 833834, because we can land the patch in this bug for B2G 1.0 (I hope), whereas the changes in bug 833834 shouldn't be needed for B2G because B2G 1.0 won't have any old data without security-info if this lands before 1.0 is frozen.

I hope we can land the patch on central soon so that Lee can test it out to make sure it solves the problem he reported.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): long-existing bug
User impact if declined: Users will see a scary broken lock icon in the B2G browser (not just a harmless-looking globe like in desktop Firefox) for any site that uses AppCache.

Testing completed: None yet. We should bake this on central for a few days.

Risk to taking this patch (and alternatives if risky): From a correctness/stability standpoint, this patch is very low risk, as we already use the same logic in the regular (not offline) cache. However, there may be some negative performance impact, since we're writing and reading an extra metdata element from the cache, and we know that I/O in the offline cache is prone to performance issues.

String or UUID changes made by this patch: none.

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Brian Smith (:bsmith) from comment #10)
> Testing completed: None yet. We should bake this on central for a few days.

To set expectations here, once this has baked for a bit, we'll uplift to FF20 on Aurora and likely land for B2G in the 20+ timeframe. Since we shipped FF18 and FFOS v1.0.0 without this fix, it can wait one more cycle.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 704743 [details] [diff] [review]
v1

Not taking this change this late in the b2g game.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 704743 [details] [diff] [review]
v1

This can land to Aurora 20, and we'll approve in the 20+ b2g timeframe for v1.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 704743 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c95ca16eb66

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 704743 [details] [diff] [review]
v1

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c1ae6ca956a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c95ca16eb66

Comment 17

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

Lee, could you please check the latest Firefox Nightly build and make sure the issue is fixed: http://nightly.mozilla.org/

Note that it may not be fixed for already-AppCached content. But, if you clear your appcache then newly-cached content should work.

Comment 18

[Lee Farrell]

Hi guys, 

Yes, I can verify that the nightly build has fixed that issue, thank you!!!