591070 - Support specifying an XPI hash through the initial HTTPS request such that redirects to HTTP can be followed securely

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Comment 1

[Dave Townsend [:mossop]]

Here is what I intend to implement:

On https requests that respond with a redirect the following header will be looked for:

X-Target-Digest: sha1:3847273423....

The first digest encountered is used, so if InstallTrigger passes a hash then that is used, if not then the first https response or the second or so on.

If a redirect from https to http occurs and no digest has yet been provided then the download is aborted.

Downloads that start from http are unaffected.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This implements support for the hash header and adds tests. The testing relies on bug 435743.

I removed the separate notification callbacks object we were using, there seems no reason for it and we need to share the hash around anyway and we only create the crypto stream in onStartRequest which always comes after any redirects have occurred.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 471883 [details] [diff] [review]
patch rev 1

I started the review of the two bugs out of order... I'll do the other one now. This is very nicely done!

>+  // nsIChannelEventSink
The CertUtils and the original CertUtils didn't format this comment correctly. Please use the same format as used elsewhere in this file
/**
 * blah
 *
 * @see nsIChannelEventSink
 */

Comment 4

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/552065e24bc8

Comment 5

[Jeff Balogh (:jbalogh)]

(In reply to comment #1)
> On https requests that respond with a redirect the following header will be
> looked for:
> 
> X-Target-Digest: sha1:3847273423....

Is this still what we should be sending?  Is anything besides sha:<string> accepted?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (In reply to comment #1)
> > On https requests that respond with a redirect the following header will be
> > looked for:
> > 
> > X-Target-Digest: sha1:3847273423....
> 
> Is this still what we should be sending?  Is anything besides sha:<string>
> accepted?

Yes that is what is supported, the algorithm can be any of md5, sha1, sha256, sha384, sha512.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, which steps have to be done to verify this fix?

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Dave, which steps have to be done to verify this fix?

Unless you want to set up custom headers for an XPI file on a webserver you'll need to wait till AMO update the install service to support this (bug 591028)

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #8)
> Unless you want to set up custom headers for an XPI file on a webserver you'll
> need to wait till AMO update the install service to support this (bug 591028)

So what I would have to add to the .htaccess file and when I want to use an existing extension, do I also have to modify its install.rdf?

Comment 10

[Dave Townsend [:mossop]]

Attachment: php script to add the hash header and redirect

I couldn't work out hte magic htaccess to do this but here is a php script that looks for a file in the same directory, adds it's hash to the header and then redirects to it over http. You'll also need a webpage that uses InstallTrigger passing it the url of the redirect script with the name of the XPI as the query string.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Dave! I have now setup a testcase on my domain for that case. It works fine when the 'X-Target-Digest' header gets set. But when I comment it out, we still download the xpi file - at least that's what the messages in the error console are telling me.

Here the example:
https://www.hskupin.info/mozilla/addons/hash/

That we do not remove the downloading entry in the doorhanger is probably the already known issue.

Comment 12

[Dave Townsend [:mossop]]

For me it looks like it does stop the download. Did you clear your cache after you commented it out it might be using the cached data.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, clearing the cache helped. Now I get:

"Warning: WARN addons.xpi: Download failed: 302 Moved Temporarily". 

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre