302284 - add xpi hash support to InstallTrigger.install()

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Daniel Veditz [:dveditz]]

InstallTrigger installs need to support support crypto hashes in order to cope
with distribution via mirrors of uncertain reliability. The most compatible way
to do this is to add an optional property to the package objects passed to
InstallTrigger.install().  If it's there the downloaded package must hash to
that value or we return a failure code; if the hash property is missing we skip
the check.

Comment 1

[Doug Turner (:dougt)]

love to see a design doc before going to actually code this.  we ensure that we
are building a secure system.  

Comment 2

[Doug Turner (:dougt)]

lack of food and english don't mix.  sorry. 

Comment 3

[Daniel Veditz [:dveditz]]

Bug 304357 blocks my patch for this one -- as of bug 296566 the Extension
manager is killing the old nsXPInstallManager (with my hash data) and creating a
new one of its own.

Comment 4

[Daniel Veditz [:dveditz]]

Attachment: Support optional crypto hashes

Comment 5

[Daniel Veditz [:dveditz]]

This patch can be checked in independently of bug 304357, but no hash checking
will actually occur until that one is checked in. I've tested them together.

Comment 6

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

Comment on attachment 192478 [details] [diff] [review]
Support optional crypto hashes

Looks fine and works as advertised

r=me

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 192478 [details] [diff] [review]
Support optional crypto hashes

> /**
>  * Interface to XPInstallManager - manages download and install operations.
>  *
>  * @status UNDER_REVIEW
>  */
>-[scriptable, uuid(50941e6a-ecfd-481c-9725-d0695c7c538e)]
>+[scriptable, uuid(566689cb-9926-4bec-a66e-a034e364ad2c)]

Is nsIXPInstallManager still UNDER_REVIEW?  dougt and I seem to think not,
so we should probably change it back to UNFROZEN or BIOHAZARD or whatever.

>+    /**
>+     * Initiates a set of downloads and checks the supplied hashes after
>+     * download. Just like initManagerFromChrome() in all other respects
>+     * @param aURLs     array of XPI urls to download and install
>+     * @param aHashes   array of hash strings to validate. The entire array
>+     *                  or individual hashes can be null to indicate no
>+     *                  checking. If supplied looks like "type:hash", like
>+     *                  "md5:3232bc5624041c507db0965324188024".
>+     *                  Supports the types in nsICryptoHash
>+     * @param aURLCount number of XPI urls in aURLs and aHashes
>+     * @param aListener a listener to receive status notifications
>+     */
>+    void initManagerWithHashes([array, size_is(aURLCount)] in wstring aURLs,
>+                               [array, size_is(aURLCount)] in string aHashes,
>+                               in unsigned long aURLCount,
>+                               in nsIXPIProgressDialog aListener);
> };

What error is given if the hash type is unknown?  We should document that here,
as
well as the error code reported for hash mismatch.

(Actually, as I read the code below, we will treat an unknown hash type as
though
none was provided, I think.  Which means that we will report no error at all,
giving the caller the impression that we verified the hash, when we did no such
thing!)

>@@ -301,28 +303,33 @@ InstallTriggerGlobalInstall(JSContext *c
>     JSIdArray *ida = JS_Enumerate( cx, JSVAL_TO_OBJECT(argv[0]) );
>     if ( ida )
>     {
>       jsval v;
>       const PRUnichar *name, *URL;
>       const PRUnichar *iconURL = nsnull;
>+      const char *hash;
> 
>       for (int i = 0; i < ida->length && !abortLoad; i++ )
>       {
>         JS_IdToValue( cx, ida->vector[i], &v );
>         name = NS_REINTERPRET_CAST(const PRUnichar*, JS_GetStringChars( JS_ValueToString( cx, v ) ));
> 
>         URL = iconURL = nsnull;
>+        hash = nsnull;
>         JS_GetUCProperty( cx, JSVAL_TO_OBJECT(argv[0]), NS_REINTERPRET_CAST(const jschar*, name), nsCRT::strlen(name), &v );
>-        if ( JSVAL_IS_OBJECT(v) ) 
>+        if ( JSVAL_IS_OBJECT(v) && !JSVAL_IS_NULL(v) )
>         {
>           jsval v2;
>-          if (JS_GetProperty( cx, JSVAL_TO_OBJECT(v), "URL", &v2 ))
>+          if (JS_GetProperty( cx, JSVAL_TO_OBJECT(v), "URL", &v2 ) && !JSVAL_IS_VOID(v2))
>             URL = NS_REINTERPRET_CAST(const PRUnichar*, JS_GetStringChars( JS_ValueToString( cx, v2 ) ));
> 
>-          if (JS_GetProperty( cx, JSVAL_TO_OBJECT(v), "IconURL", &v2 ))
>+          if (JS_GetProperty( cx, JSVAL_TO_OBJECT(v), "IconURL", &v2 ) && !JSVAL_IS_VOID(v2))
>             iconURL = NS_REINTERPRET_CAST(const PRUnichar*, JS_GetStringChars( JS_ValueToString( cx, v2 ) ));
>+
>+          if (JS_GetProperty( cx, JSVAL_TO_OBJECT(v), "Hash", &v2) && !JSVAL_IS_VOID(v2))
>+            hash = NS_REINTERPRET_CAST(const char*, JS_GetStringBytes( JS_ValueToString( cx, v2 ) ));
>         }

This string munging isn't GC-safe -- we don't root the string returned from
JS_ValueToString, so it can be destroyed by a GC forced by the second
JS_ValueToString, and then the jschar* returned by JS_GetStringChars would
dangle, crashingly.

Also, we don't seem to handle JS_ValueToString failure.  It looks like 
passing an object like
  { IconURL: { toString: function() { throw 5; } } }
would crash us.

There are UI/UE issues here that need addressing, too, but we should file
another bug on that.

Comment 8

[Doug Turner (:dougt)]

I also really don't like that xpinstall knows about the hashing algorithms
directly.  We should remove the kHashtypes array and rely on the cryptohash
interface to tell us this info.  Better yet, we could have parameterized
contract id's for the cryptohash (ie.  mozilla.org/cryptohash/type=sha256).

I will see if I can put something together to address these problems and post a
new patch shortly.

Comment 9

[Doug Turner (:dougt)]

rob, can you attach a test case or point me to what you used to test this?  this
would save me a bit of time.

Comment 10

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

Attachment: testcase

I'm unable to verify this testcase is correct due to losing the tree I tested
this with along with a hard drive this weekend... this should work as is
though.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.2

This patch:

1) removes the algorithm string parsing from xpinstall and moves it into the
crypto hash implemenation.

2) adds the ablity to determine if the hash type in the trigger is unknown or
invalid.

3) drops the UNDER_REVIEW status from the nsIXPInstallManager.idl.


I tested this with Rob's testcase and seams to work.

I have not address any of the js string issues.  I think that this might be a
wider problem the what this patch introduces (new bug: 304988)

Doug

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 192963 [details] [diff] [review]
patch v.2

> [scriptable, uuid(8751865e-b01d-4f33-bc13-b361dde165ed)]
> interface nsICryptoHash : nsISupports

Please rev the UUID.

>     /**
>+     * Initialize the hashing object. This method may be
>+     * called multiple times with different algorithm types.
>+     *
>+     * @param aAlgorithm the algorithm type to be used.
>+     *
>+     * @throws NS_ERROR_INVALID_ARG if an unsupported algorithm
>+     *         type is passed.
>+     *
>+     * NOTE: This method must be called before any other method on 
>+     *       this interface is called.
>+     */

That comment (and the one for init(alg)) should say that either
init or initWithString must be called before any other method.

>+    void initWithString(in string aAlgorithm);

Should use ACString here.

> NS_IMETHODIMP
>+nsCryptoHash::InitWithString(const char* algorithm)
>+{
>+  if (!PL_strncasecmp(algorithm, "MD2", 3))
>+    return Init(nsICryptoHash::MD2);
>+
>+  if (!PL_strncasecmp(algorithm, "MD5", 3))
>+    return Init(nsICryptoHash::MD5);
>+
>+  if (!PL_strncasecmp(algorithm, "SHA1", 4))
>+    return Init(nsICryptoHash::SHA1);

Once you switch to ACString in the IDL, these should become
  if (algorithm.LowerCaseEqualsLiteral("md2"))

The current code will cause "SHA1024" to match "SHA1" (only
the first 4 characters are compared)!

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.3

address shaver's comments.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 192968 [details] [diff] [review]
patch v.3

sr=shaver (didn't see this earlier because it wasn't requested against me, and
I'm not cc:d on the bug, sorry)

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 192968 [details] [diff] [review]
patch v.3

Caillon, can you review at least the caps changes?

Doug, can you request review from specific module owners and peers for files
this patch touches?

/be

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 192968 [details] [diff] [review]
patch v.3

not much to do there brendan...

caillon is the owner of caps.  The nsICryptoHash interface sits there.	He is
on the hook for the review.

I do not think we have any owners or peers of security/manager.  Is kai still
active?

As for the toolkit change -- it is a xpinstall string we are adding.  Does this
need to go through some l10n process since we are going to land this on the
branch?

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The only CAPS change is to the IDL, to add a method that it probably should have
had in the first place (and I probably missed that fact in my own review of the
patch at the time, mea maxima etc.).  I don't think we need caillon specifically
here, if he's not readily available.

If this is a blocker, though, why did it sit for a week before review was
specifically requested of someone?

Comment 18

[Doug Turner (:dougt)]

shaver, you know the answer to that -- I didn't have the time as dveditz hoped
for and i didn't single the reviewers out on the review=? like instead relying
on the cc' list to jump in.  

caillon|rob, can we get an r= in the next day?

Comment 19

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

Sorry but I won't have the time to review this until Friday evening at the earliest.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 192968 [details] [diff] [review]
patch v.3

r=dveditz

Comment 21

[Doug Turner (:dougt)]

Trunk:

Checking in caps/idl/nsICryptoHash.idl;
/cvsroot/mozilla/caps/idl/nsICryptoHash.idl,v  <--  nsICryptoHash.idl
new revision: 1.3; previous revision: 1.2
done
Checking in security/manager/ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <-- 
nsNSSComponent.cpp
new revision: 1.127; previous revision: 1.126
done
Checking in toolkit/locales/en-US/chrome/global/xpinstall/xpinstall.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/xpinstall/xpinstall.properties,v
 <--  xpinstall.properties
new revision: 1.6; previous revision: 1.5
done
Checking in xpinstall/public/nsIXPInstallManager.idl;
/cvsroot/mozilla/xpinstall/public/nsIXPInstallManager.idl,v  <-- 
nsIXPInstallManager.idl
new revision: 1.3; previous revision: 1.2
done
Checking in xpinstall/src/nsInstall.h;
/cvsroot/mozilla/xpinstall/src/nsInstall.h,v  <--  nsInstall.h
new revision: 1.101; previous revision: 1.100
done
Checking in xpinstall/src/nsJSInstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstall.cpp,v  <--  nsJSInstall.cpp
new revision: 1.119; previous revision: 1.118
done
Checking in xpinstall/src/nsJSInstallTriggerGlobal.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp,v  <-- 
nsJSInstallTriggerGlobal.cpp
new revision: 1.49; previous revision: 1.48
done
Checking in xpinstall/src/nsXPITriggerInfo.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.cpp,v  <--  nsXPITriggerInfo.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in xpinstall/src/nsXPITriggerInfo.h;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.h,v  <--  nsXPITriggerInfo.h
new revision: 1.22; previous revision: 1.21
done
Checking in xpinstall/src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.137; previous revision: 1.136
done
Checking in xpinstall/src/nsXPInstallManager.h;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.h,v  <--  nsXPInstallManager.h
new revision: 1.40; previous revision: 1.39
done


1.8 Branch:
Checking in caps/idl/nsICryptoHash.idl;
/cvsroot/mozilla/caps/idl/nsICryptoHash.idl,v  <--  nsICryptoHash.idl
new revision: 1.2.4.1; previous revision: 1.2
done
Checking in security/manager/ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <-- 
nsNSSComponent.cpp
new revision: 1.126.2.1; previous revision: 1.126
done
Checking in toolkit/locales/en-US/chrome/global/xpinstall/xpinstall.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/xpinstall/xpinstall.properties,v
 <--  xpinstall.properties
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in xpinstall/public/nsIXPInstallManager.idl;
/cvsroot/mozilla/xpinstall/public/nsIXPInstallManager.idl,v  <-- 
nsIXPInstallManager.idl
new revision: 1.2.18.1; previous revision: 1.2
done
Checking in xpinstall/src/nsInstall.h;
/cvsroot/mozilla/xpinstall/src/nsInstall.h,v  <--  nsInstall.h
new revision: 1.100.8.1; previous revision: 1.100
done
Checking in xpinstall/src/nsJSInstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstall.cpp,v  <--  nsJSInstall.cpp
new revision: 1.118.4.1; previous revision: 1.118
done
Checking in xpinstall/src/nsJSInstallTriggerGlobal.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp,v  <-- 
nsJSInstallTriggerGlobal.cpp
new revision: 1.47.2.2; previous revision: 1.47.2.1
done
Checking in xpinstall/src/nsXPITriggerInfo.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.cpp,v  <--  nsXPITriggerInfo.cpp
new revision: 1.30.2.1; previous revision: 1.30
done
Checking in xpinstall/src/nsXPITriggerInfo.h;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.h,v  <--  nsXPITriggerInfo.h
new revision: 1.21.4.1; previous revision: 1.21
done
Checking in xpinstall/src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.135.2.2; previous revision: 1.135.2.1
done
Checking in xpinstall/src/nsXPInstallManager.h;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.h,v  <--  nsXPInstallManager.h
new revision: 1.39.18.1; previous revision: 1.39
done

Marking fixed. 

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 192968 [details] [diff] [review]
patch v.3

r=caillon retroactively for caps.