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

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla2.0b7
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

7 years ago
blocking2.0: --- → beta6+
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
(Assignee)

Updated

7 years ago
Blocks: 537761
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
Created attachment 471883 [details] [diff] [review]
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.
Attachment #471883 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
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
 */
Attachment #471883 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 4

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/552065e24bc8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
(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?
(Assignee)

Comment 6

7 years ago
(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.
Dave, which steps have to be done to verify this fix?
(Assignee)

Comment 8

7 years ago
(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)
(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?
(Assignee)

Comment 10

7 years ago
Created attachment 486684 [details]
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.
Attachment #486684 - Attachment mime type: text/php → text/plain
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.
(Assignee)

Comment 12

6 years ago
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.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.