178687 - Support Signed XPI packages

Status: VERIFIED FIXED

(SeaMonkey :: Installer, defect, P2)

Description

[Jaime Rodriguez, Jr.]

We should be able to check authencity of packages that have been signed, and
provide applications to take action upon validity and source of the signature.

Comment 1

[Doug Turner (:dougt)]

*** Bug 179018 has been marked as a duplicate of this bug. ***

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

This patch, along with the patches in the two blocking bugs,
implements signing support in XPInstall.  There are a couple of
assumptions: The first is that the zigbert.rsa file is the first file
in the xpi file.  This can easily be support using the zip and
signtool utilities:

signtool -d ./certs -kdougt test 
cd test 
zip test.xpi META-INF/zigbert.rsa 
zip -r -D test.xpi * -x META-INF/zigbert.rsa 
mv test.xpi ../ 
cd ..

The next assumption is that the cert authority is installed in the
browser.  This kind of goes without saying, but I wanted to mention it
in case try it and didn’t know.  This is the exact behavior we want
as if this signing worked with whatever CA, what would be the point?

The UI needs work.  We don’t have to check any of it in.  Without
any of the UI changes, signing verification will still run.  However,
the user will not be alerted that an installer is signed.  Someone
from the front end should provide a better UI.	What I have done here
is push the signing cert’s Organization Name into XUL.	Someone on
the frontend team needs to take it from here.

I have created a few test cases that I uploaded to
http://www.mozilla.org/projects/xpinstall/signed/testcases/index.html.

I am looking forward to hear some feedback on this patch.

Comment 3

[Doug Turner (:dougt)]

dan, mitch, please review the above patch.

Comment 4

[[e-mail] (away - not reading bugmail)]

Doug, Dan,

would you know of any plans to support XPI versioning as well? I ran a few
queries with no avail. I've seen a couple REFs recently:

bug 162801 and http://bugscape.nscp.aoltw.net/show_bug.cgi?id=18119

Comment 5

[Doug Turner (:dougt)]

jbetak, please start a thread somewhere else.  this topic has *nothing* to do
with signing.  Thank you.

Comment 6

[[e-mail] (away - not reading bugmail)]

Doug, I was merely looking for a clue. I've put some L10n people on cc, as this
bug will directly affect their work.

Comment 7

[Doug Turner (:dougt)]

Attachment: Includes everything

This is a roll up of all the patches to make xpinstall signing work.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 106752 [details] [diff] [review]
Includes everything

This is the real patch to review.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 105895 [details] [diff] [review]
patch v.1

obsolete patch doesn't need to be in the request queue.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 106752 [details] [diff] [review]
Includes everything

>             NS_IMETHOD InstallJar(nsIFile* localFile,
>                                   const PRUnichar* URL,
>                                   const PRUnichar* arguments,
>+                                  nsIPrincipal* aPrincipalDisplayed,
>                                   PRUint32 flags,
>                                   nsIXPIListener* aListener = 0) = 0;

Hm, this is going to make freezing nsISoftwareUpdate harder. Ok for now though.

>+[scriptable, uuid(42cd7162-ea4a-4088-9888-63ea5095869e)]
>+interface nsPICertNotification : nsISupports

Does this need to be scriptable? If it's not it'll save xpt size (disk and
memory).

>Index: xpinstall/res/content/institems.js
>===================================================================
>+  certName.setAttribute("tooltiptext", aUrl);
>+
>+  // thrid column is the host serving the file

We'll re-do the UI later, but the certname doesn't need a tooltip,
and fix "thrid"

>     moduleName = gParam.GetString(i);
>     URL = gParam.GetString(++i);
>-    addTreeItem(row++, moduleName, URL);
>+    certName = gParam.GetString(++i);
>+
>+    addTreeItem(row++, moduleName, URL, certName);
>   }

Please change the comments in nsIXPIDialogService.idl for params
packageList and count (twice each) to reflect your changed usage
of these APIs.

>Index: xpinstall/res/locale/en-US/xpinstall.properties
>===================================================================
> error-239=Chrome registration failed
> error-240=Unfinished install
> error-299=Out of memory
>+error-600=Signing could not be verified.

This error should be in the -2xx range. -203, -205 and -206 are obsolete errors
related to signing (see nsInstall.h), perhaps you could re-use those slots.

>Index: xpinstall/src/CertReader.cpp
>===================================================================
>RCS file: xpinstall/src/CertReader.cpp
>+static unsigned int xtoint (unsigned char *ii)
>+static unsigned long xtolong (unsigned char *ll)

Note to self: make these inlines in some appropriate header

>+static int deflate(unsigned char* compr, PRUint32 comprLen, unsigned char* uncompr, PRUint32 uncomprLen)

Please don't call this "deflate" when you're calling the zlib "inflate" call.
zlib has
a "deflate" method that is exactly the opposite of what you're doing and that's
just asking for confusion. If you don't like "inflate" for some reason then use
"uncompress".

>+  strcpy((char*)uncompr, "garbage");

You don't know that uncompr is big enough to hold "garbage". If it's a real
cert, sure, but someday the first file in the archive is going to end up being
some dinky thing and then we're in trouble.

Why do you need it anyway? Trust the return value.

>+  err = inflate(&d_stream, Z_NO_FLUSH);
>+  if (err != Z_OK && err != Z_STREAM_END) { 
>+    return -1;
>+  }
>+  
>+  err = inflateEnd(&d_stream);

You need to call inflateEnd() even if inflate returns an error or you
can leak things allocated off the d_stream object.

>+CertReader::OnStartRequest(nsIRequest *request, nsISupports* context)
>+  mLeftoverBuffer.Assign("");

How 'bout .Truncate()? slightly more efficient.

>+CertReader::OnDataAvailable(nsIRequest *request, 

>+  while (aLength) 

I don't see you decrement aLength by amt each time through the loop.
If you ever shrank your buffer size or necko started feeding data in
larger chunks...

>+    const char* carot = mLeftoverBuffer.get();

"caret" ... do I have the wrong version of this file? we went over this next
part before.

>+    PRUint32 fileEntryLen = (ZIPLOCAL_SIZE + 
>+                             xtoint(ziplocal->filename_len) +
>+                             xtoint(ziplocal->extrafield_len) +
>+                             xtoint(ziplocal->filename_len));

Don't need the filename_len twice, the last one should be ziplocal->size
(and xtolong rather than xtoint).

Sometimes the downloaded file is not a zip archive but might be the wrong
file, or an HTML server error (404 or something). Before using ziplocal you
should make sure ziplocal->signature is LOCALSIG.

>+    if (mLeftoverBuffer.Length() < fileEntryLen)
>+    {
>+      // we are just going to buffer and continue.
>+      continue;
>+    }

If you don't, we could be interpreting bogus size data and get into an infinite
loop
where Length() could never measure up. well, I guess the Read() would
eventually
fail and we'd be saved that way.

>+    // we don't really need the filename since we only look at the first file.
>+    nsCString filename;
>+    filename.Assign((const char*) (carot + ZIPLOCAL_SIZE), 
>+                    xtoint(ziplocal->filename_len));

But given the filename you could at least sanity check the extension to see
if it even looks like a signature file. That would be especially useful in a
non-signed archive where the first file could be a really big binary file. We
wouldn't want to wait that long before putting up a dialog. In other words
read the name as soon as you can, earlier than this.

>+    int err = deflate((unsigned char*)data, 

You only want to do this if ziplocal->method says it's compressed. Sometimes
it's just stored.

>Index: xpinstall/src/nsInstallTrigger.cpp
>===================================================================

This is the wrong place to put the CertReader stuff, it needs to go
into nsXPInstallManager instead. If you're going to do it here then you
need to catch all ways into the install manager, here you catch
StartSoftwareUpdate but not the more common Install() method.

You changed the constructor for nsXPITriggerInfo, but this patch doesn't
show diffs for nsJSInstallTriggerGlobal.cpp which creates some of these.

>         if (uri) {
>-            nsCAutoString spec;
>-            rv = uri->GetSpec(spec);
>+          nsCAutoString spec;
>+          rv = uri->GetSpec(spec);

You changed the indentation here from 4 spaces to 2. Unless you're planning
on changing the entire file please keep it consistent with the rest (4).

>Index: xpinstall/src/nsSoftwareUpdate.cpp
>+                               nsIPrincipal* aPrinciple,

change "principle" to "principal" everywhere, not just this file.

>+nsresult VerifySigning(nsIZipReader* hZip, nsIPrincipal* aPrincipal)
 ...
>+        if ( PL_strncasecmp("META-INF", name.get(), 8) == 0)
>+            continue;

Compare against "META-INF/" instead, to avoid getting spoofed by files
with "META-INSUCKER" in the path.

You could also check the last character for "/" and skip directory entries.

>+    if (entryCount != manifestEntryCount)
>+        return NS_ERROR_FAILURE;  // some files were delete from archive

Or added, which is more troubling.

>+        NS_ASSERTION(0, "Signing check of archive failed!");

NS_ERROR() or NS_WARNING() are better than NS_ASSERTION(0,...). Works out
pretty much the same though.

>+    nsCOMPtr<nsICertificatePrincipal> cp(do_QueryInterface(mPrincipal));
>+    if (cp) {
>+      nsXPIDLCString cName;
>+      cp->GetCommonName(getter_Copies(cName));
>+      mCertName = NS_ConvertUTF8toUCS2(cName);

Are all valid certs guaranteed to have a common name?

>Index: xpinstall/src/nsXPInstallManager.cpp
>===================================================================
>+
>+    NR_StartupRegistry();
> }
>+
>+    NR_ShutdownRegistry();

These are already in the required nsSoftwareUpdate service, you shouldn't need
them

Comment 11

[Doug Turner (:dougt)]

Great comments.  I fixed all of them except the following:

First, the filename check.  I don't think that there is any real value is
testing for the file name.  In fact, there are two many possible file names that
a cert can have.  Ensuring it is the first file, buys us the ablity not to have
to worry about the actually filename.

I thought that stored files should inflate fine.  Are you saying that I need to
really test the method prior to calling inflate?

The cert reader stuff is just fine in the trigger - that is how every jar 
file is feed into the the install manager.  The install manager shouldn't have
to worry about the getting the principle.  if we have to, we can move it later,
but I see not value in doing this.  

Yes, all valid certs guaranteed to have a common name - which actually the 
Organization name.  You could create a cert without one, but all ca do use 
this field.

New patch to just xpinstall coming up...

Comment 12

[Doug Turner (:dougt)]

Attachment: dan's suggestions to xpinstall

Comment 13

[Daniel Veditz [:dveditz]]

You missed my point about the filename check (really an extension check).
Currently all xpis are unsigned, and if the first file in the archive just
happened to be a 1Mb binary the user is sitting there wondering why the heck
nothing is happening while we're downloading this huge file, only to find out
it's not a signature.

Alternatively is there some size where you can say "No way it's a cert if it's
bigger than X"?

As mentioned on IM one reason for moving the CertReader stuff into
nsXPInstallManager (beyond it just being the intended place for this kind of
logic) is that it would centralize the code into one spot rather than having to
duplicate it for each different type of trigger and perhaps missing one
inadvertantly (as you in fact did with InstallTrigger.install() -- the
predominant form of javascript trigger).

Comment 14

[Daniel Veditz [:dveditz]]

One approach would be to split InitManager in two, with the bulk of the current
functionality being in a new ManagerCore() function. The remaining InitManager
would do something like

   if chrome install
     ManagerCore(args);
   else
      call CertReader on first trigger;

OnCertAvailable would look like

  add principal, if found, to current trigger item
  if more triggers
     call CertReader on the next one
  else
     ManagerCore(args)

There are no threading worries with keeping track of which trigger item we're
on: nsXPInstallManager is not a service, a new one is created for each install
attempt. A single trigger could span multiple packages though, and that's the
cause of the complication.

Comment 15

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 106752 [details] [diff] [review]
Includes everything

Please change the return value checking at nsJAR.cpp line 574 to check for no
error before setting mGlobalStatus to Valid, since we now return a principal
even if the file didn't verify - otherwise you're opening up a hole in signed
JS.

Dan, do you see any heap allocation issues in Doug's expand function, similar
to the ones you fixed recently? I want to make sure we don't repeat any of
those problems.

One nit: your new XPInstall patch still uses "principle" in nsXPITriggerInfo.
This should probably be "principal" for consistency's sake.

r=mstoltz if you make that change in nsJAR.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 106752 [details] [diff] [review]
Includes everything

good thinking Mitch, horribly dense on my part to forget to check that
after fixing two recent security bugs with essentially the same problem.

>+    PRUint32 orgSize = xtolong ((unsigned char *) ziplocal->orglen);

>+    unsigned char* orgData = (unsigned char*) malloc(orgSize);

The potential problem is that the zip archive might be hacked, with
an orgSize smaller than the data really uncompresses into.
Might even be zero, and a malloc(0) suceeds on some platforms.
(A hacked size might be one way to get your "garbage" strcpy to
overwrite something, but that's not exploitable by itself.)

But in this case the deflate should handle it fine since you'll have
an "avail_out" of 0. Watch out for it, though, when you add support
for "stored" (not compressed) certs and use memcpy() or something.

Might as well check for an orgSize of 0 and error out before calling malloc.
A directory entry, by the way, will have an orgSize of 0 and it's not
inconceivable you'd run across one of those as the first entry (in
an unsigned archive). But otherwise I don't see any security
problems here.

Comment 17

[Doug Turner (:dougt)]

Attachment: moves cert reading code into xpi manager per dveditz

this just includes changes to xpinstall/src

Comment 18

[Doug Turner (:dougt)]

Attachment: xpinstall signing v.2

This includes everything.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 107184 [details] [diff] [review]
xpinstall signing v.2

>+     * @param packageList   For each install package there will be three strings,
>+     *                      a display name, a source URL, and a the name of the
>+     *                      organization signed this install.  Note that the name

"organization _that_ signed"					    

>+     *                      of the signer is not verified.  Verification happens
>+     *                      when the the install has completely downloaded. Your
>+     *                      user interface should only suggest that the install 
>+     *                      may be signed by this organization name.   

Mention that an unsigned archive is indicated by an empty string.

>      * @param count         The number of strings in the packageList. This
>      *                      will always be even (twice the number of packages)

This will always be three times the number of packages to install.

>+error-260=Signing could not be verified.

"Signature" instead of "signing"? Or is this error really about the signature
and more about the signed content?

>+  err = inflate(&d_stream, Z_NO_FLUSH);
>+  
>+  if (err != Z_OK && err != Z_STREAM_END) { 
>+    inflateEnd(&d_stream);
>+    return -1;
>+  }
>+  
>+  err = inflateEnd(&d_stream);
>+  if (err != Z_OK) { 
>+    return -1;
>+  }
>+  return 0;

nit: the duplication of inflateEnd bugs me... more compact as

  int err  = inflate(&d_stream, Z_NO_FLUSH);
  int err2 = inflateEnd(&d_stream);

  if ((err != Z_OK && err != Z_STREAM_END) || err2 != Z_OK)
      return -1;
  return 0;

>+CertReader::OnDataAvailable(nsIRequest *request, 
....
>+  while (aLength) 
....

>+    ZipLocal_* ziplocal = (ZipLocal_*) caret;
>+    
>+    if (xtolong(ziplocal->signature) != LOCALSIG)
>+        return NS_ERROR_FAILURE;

Technically need something like 
      if (mLeftoverBuffer.Length() < ZIPLOCAL_SIZE)
	 continue:
first, although we're probably pretty sure to get more than ZIPLOCAL_SIZE bytes
in the first chunk

check for size > 32K here before doing anything more because a signature file
should never be that big. The ones I see in test signed files are about 1800
bytes compressed, 2800 uncompressed. I know sigs can have extra stuff in them
so it can't hurt to pad to be on the safe side (8K is probably safe, too)

      // In an unsigned package the first file might be huge, we don't want to
      // download it all only to fail the signature check later.
#define MAX_SIG_SIZE  0x4000
>+    PRUint32 cSize   = xtolong ((unsigned char *) ziplocal->size);
      if (size > MAX_SIG_SIZE)
	 return NS_ERROR_FAILURE;

>+    // we don't really need the filename since we only look at the first file.
>+    nsCString filename;
>+    filename.Assign((const char*) (caret + ZIPLOCAL_SIZE), 
>+                    xtoint(ziplocal->filename_len));

If we don't need the name let's skip this. I think the size check above is
sufficient for sanity.

>     if ( !mTriggers || mTriggers->Size() == 0 )
>     {
>         rv = NS_ERROR_INVALID_POINTER;
>         NS_RELEASE_THIS();
>         return rv;
>     }
> 
>+    mParentWindow = do_QueryInterface(aGlobalObject);
>+    mOutstandingCertLoads = mTriggers->Size();

At this point you should say

   if (mChromeType != NOT_CHROME)
      return InitManagerInternal();

since you can skip the cert reads because chrome installs won't have certs.
Make sure you do it after setting mParentWindow first though.

>+    nsXPITriggerItem *item = mTriggers->Get(mOutstandingCertLoads-1);
>+    mOutstandingCertLoads--;

nit: is this clearer and more efficient as a single line (repeated later as
well)?

      nsXPITriggerItem *item = mTriggers->Get(--mOutstandingCertLoads);

>+    NS_NewURI(getter_AddRefs(uri), NS_ConvertUCS2toUTF8(item->mURL.get()).get());
>+    nsIStreamListener* listener = new CertReader(uri, nsnull, this);
>+    NS_ADDREF(listener);
>+    rv = NS_OpenURI(listener, nsnull, uri);
>+    NS_RELEASE(listener);
>+    return rv;

The install manager owns itself at this point because we can't afford to hang
up
the trigger code on the main thread waiting to see if it should release us. So
we need to be careful here to make sure we don't leak.

If listener is null we'll never hear when the URI is loaded. If URI is null, or
some other error, NS_OpenURI() will return an error and our listener probably
won't get called. So in those error cases you'll have to call NS_RELEASE_THIS()

You could, I suppose, just go ahead and call InitManagerInternal() instead and
let
it treat the archive(s) as unsigned or handle the after-error cleanup.

Or you could call OnCertAvailable() with errors, and let it try the next one
and fall through into InitManagerInternal() at the end.

>+nsXPInstallManager::OnCertAvailable(nsIURI *aURI, 
>+                                    nsISupports* context, 
>+                                    nsresult aStatus, 
>+                                    nsIPrincipal *aPrincipal)
>+{
>+    if (NS_FAILED(aStatus)) {
>+        // do something useful like tell the user that we couldn't download part of the file.
>+        return NS_ERROR_FAILURE;
>+    }

when/how do you get a failing status here? Will those NS_ERROR_FAILURE returns
in OnDataAvailable
cause that? Maybe those should be changed to NS_BINDING_ABORTED (which I assume
counts as
a successful status).

If you fail here what if there are others to be downloaded? Again with the
cleanup
either we need to try each one or barge into InitManagerInternal() or call
NS_RELEASE_THIS()
to cleanup.  Making sure InitManagerInternal() is eventually called in all
cases would be my
preference because it also handles the error reporting callback.

Same error-checking concerns in the rest of this routine as at the end of
InitManager().

Comment 20

[Doug Turner (:dougt)]

Attachment: xpinstall/src diff

Fixes up OnCertAvailable error handling and incorporates dveditz concerns. 
Note that this patch is just for xpinstall/src

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 107915 [details] [diff] [review]
xpinstall/src diff

Thanks for the tons of context in the patches, made things
a lot easier to review.

This patch appears to have only the 'src' directory in it, not the idl, chrome,
stub, etc changes. Since my previous review asked for changes in some of those
files I'd like to see 'em again. I had a few questions you forgot to answer,
too.

My biggest concern at this point is that you still have no check to prevent
downloading a huge file on an unsigned cert. Given the expense of certs I think
we have to assume most XPI files will continue to be unsigned, and on a
language pack
if the main .jar happens to be first that's nearly 1Mb. I checked a browser.xpi
I got and the first file there was the > 1Mb comm.jar

We just can't do without a max size (and possibly file extension) check.


>+  strcpy((char*)uncompr, "garbage");

I'm still not keen on this. You've ensured the buffer is big
enough but it's still going to trip all our automated scanning
tools like flawfinder, wasting lots of people's time.

  *uncompr = '\0'; 

ought to be fine for initialization. I won't hold up a checkin
for this though.

>+    if (mLeftoverBuffer.Length() <= MIN_SIGNATURE_SIZE)
>+      continue;

nit: if MIN_SIGNATURE_SIZE is accurate you only need '<'

more impt: I can't find it, is that your #define or not?
Does it include the size of the zip headers and filename
and potential "extra" headers that might be in the archive?

I guess it doesn't matter all that much since you do
a more accurate check a few lines down (does this early
return save enough to be worth the double-checking?).

>+    ZipLocal_* ziplocal = (ZipLocal_*) caret;
>+
>+    // did we read the entire file entry into memory?
>+    PRUint32 fileEntryLen = (ZIPLOCAL_SIZE + 
>+                             xtoint(ziplocal->filename_len) +
>+                             xtoint(ziplocal->extrafield_len) +
>+                             xtolong(ziplocal->size));

You have to validate this is really a zip file before you start believing the
zip fields. As I mentioned in a previous review check ziplocal->signature
against LOCALSIG

>+    unsigned char* orgData = (unsigned char*) malloc(orgSize);
>+          
>+    if (!orgData)
>+      return NS_BINDING_ABORTED;

Stylistically I'd prefer checking whether an operation failed
not be separated by a blank line from the operation itself,
at least not for simple checks like this and an rv success check
earlier.

Is NS_BINDING_ABORTED a special meaning in Necko code? Elsewhere
you return NS_ERROR_FAILURE, and here might be NS_ERROR_OUT_OF_MEMORY.

>Index: Makefile.in

I don't see any Mac magic here... people will hammer you when
the mac classic builds die.

>+
>+            SIGNING_NOT_VALID           = -260,

I'd prefer INVALID_SIGNATURE or SIGNATURE_VERIFICATION (the "error in"
is implied by this being an error code).

Whatever you choose, though, needs to be added to install_constants[]
in nsJSInstall.cpp

>Index: nsSoftwareUpdateRun.cpp
>===================================================================
>+nsresult VerifySigning(nsIZipReader* hZip, nsIPrincipal* aPrincipal)

>+    nsCOMPtr<nsIPrincipal> principal;
>+    nsresult rv = jar->GetCertificatePrincipal(nsnull, getter_AddRefs(principal));
>+    if (NS_FAILED(rv))
>+        return rv;

Add a comment along the lines "See if the archive is signed at all
as a quick out".

Will GetCertificatePrincipal() always fail if there isn't a cert,
or would it return a null principal instead? Looks like it can return
success without a principal if there is no signature verifier or if
there is no manifest. I suppose you've also got checks for those later,
but those are just the cases I found right off. Looks like several jar
errors are merely reported to the console, with NS_OK returned (and
no principal).

So I guess you also need to check for a null principal or you might as
well skip this check. And you might as well check that it equals
aPrincipal while you're at it.

>+        rv = jar->GetCertificatePrincipal(name, getter_AddRefs(principal));
>+        if (NS_FAILED(rv) || !principal) return NS_ERROR_FAILURE;

Add a comment above this "Each entry must be signed"

>+        rv = principal->Equals(aPrincipal, &equal);
>+        if (NS_FAILED(rv) || !equal) return NS_ERROR_FAILURE;

And a comment above this "And it's principal must match the one
the user approved."

>+    if (entryCount != manifestEntryCount)
>+        return NS_ERROR_FAILURE;  // some files were delete from archive

Need 'd' on the end of deleted

It'd really be nice to skip directory entries, which would be
entries whose name ends in '/' and a size of 0. A little bit of
code here, save a lot of hassle for people creating these things.

>+    rv = NS_OpenURI(listener, nsnull, uri);
>+    NS_RELEASE(listener);
>+    return rv;

If this fails you need to release 'this' (see existing code).

>-    PRUint32 numStrings = 2 * numTriggers;
>+    PRUint32 numStrings = 3 * numTriggers;

You need to update the comments in the IDL files.

>+nsXPInstallManager::OnCertAvailable(nsIURI *aURI, 


>+    item = mTriggers->Get(mOutstandingCertLoads-1);
>+    mOutstandingCertLoads--;

Why not Get(--mOutstandingCertLoads) as above?

Comment 22

[Doug Turner (:dougt)]

> more impt: I can't find it, is that your #define or not?

Yes, it is 32k.  All cert data should fit within this block size.  See the top
of CertReader.cpp for this define.

>Is NS_BINDING_ABORTED a special meaning in Necko code? 

Yes.  The difference between this error code and other is that Binding Aborted
indicates that CertReader is done with the file stream while the other error
codes indicate a major problem was encountered.

>  GetCertificatePrincipal() always fail if there isn't a cert, or would it
return a null principal instead?

if the file is not a cert, then it will fail.  ie. UI should not suggest that
the install has been signed in the install prompts.

I didn't fix up the directory entries.

New patch coming up which will contain everything under xpinstall (including the
mac changes)

Comment 23

[Doug Turner (:dougt)]

Attachment: xpinstall patch

Comment 24

[Doug Turner (:dougt)]

also, i don't validate the zip file before passing it to VerifySig.  I don't
think that I have to do this since VerifySig does this for me.  Thoughts?

Comment 25

[Mitchell Stoltz (not reading bugmail)]

What do you mean "validate?" Do you mean validating against a checksum of some
kind? If so, then you are correct, the cryptographic verification can take the
place of the checksum. What about unsigned XPIs, though - do they get validated?
They probably should.

Comment 26

[Doug Turner (:dougt)]

Attachment: xpinstall patch

includes more dveditz feedback

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 109453 [details] [diff] [review]
xpinstall patch

>      * @param count         The number of strings in the packageList. This
>-     *                      will always be even (twice the number of packages)
>+     *                      will always be odd (three times the number of 
>+     *                      packages)

Er, that's not an improvement :-) Just "will always be three times the number
of packages"

Great job, thanks!
 sr=dveditz

Comment 28

[Doug Turner (:dougt)]

? src/x
? x
Checking in macbuild/xpinstall.xml;
/cvsroot/mozilla/xpinstall/macbuild/xpinstall.xml,v  <--  xpinstall.xml
new revision: 1.4; previous revision: 1.3
done
Checking in macbuild/xpinstallIDL.xml;
/cvsroot/mozilla/xpinstall/macbuild/xpinstallIDL.xml,v  <--  xpinstallIDL.xml
new revision: 1.4; previous revision: 1.3
done
Checking in public/Makefile.in;
/cvsroot/mozilla/xpinstall/public/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
Checking in public/nsISoftwareUpdate.h;
/cvsroot/mozilla/xpinstall/public/nsISoftwareUpdate.h,v  <--  nsISoftwareUpdate.h
new revision: 1.33; previous revision: 1.32
done
Checking in public/nsIXPIDialogService.idl;
/cvsroot/mozilla/xpinstall/public/nsIXPIDialogService.idl,v  <-- 
nsIXPIDialogService.idl
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/xpinstall/public/nsPICertNotification.idl,v
done
Checking in public/nsPICertNotification.idl;
/cvsroot/mozilla/xpinstall/public/nsPICertNotification.idl,v  <-- 
nsPICertNotification.idl
initial revision: 1.1
done
Checking in res/content/institems.js;
/cvsroot/mozilla/xpinstall/res/content/institems.js,v  <--  institems.js
new revision: 1.11; previous revision: 1.10
done
Checking in res/content/institems.xul;
/cvsroot/mozilla/xpinstall/res/content/institems.xul,v  <--  institems.xul
new revision: 1.25; previous revision: 1.24
done
Checking in res/content/xpistatus.js;
/cvsroot/mozilla/xpinstall/res/content/xpistatus.js,v  <--  xpistatus.js
new revision: 1.12; previous revision: 1.11
done
Checking in res/locale/en-US/xpinstall.properties;
/cvsroot/mozilla/xpinstall/res/locale/en-US/xpinstall.properties,v  <-- 
xpinstall.properties
new revision: 1.12; previous revision: 1.11
done
RCS file: /cvsroot/mozilla/xpinstall/src/CertReader.cpp,v
done
Checking in src/CertReader.cpp;
/cvsroot/mozilla/xpinstall/src/CertReader.cpp,v  <--  CertReader.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpinstall/src/CertReader.h,v
done
Checking in src/CertReader.h;
/cvsroot/mozilla/xpinstall/src/CertReader.h,v  <--  CertReader.h
initial revision: 1.1
done
Checking in src/Makefile.in;
/cvsroot/mozilla/xpinstall/src/Makefile.in,v  <--  Makefile.in
new revision: 1.63; previous revision: 1.62
done
Checking in src/nsInstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsInstall.cpp,v  <--  nsInstall.cpp
new revision: 1.205; previous revision: 1.204
done
Checking in src/nsInstall.h;
/cvsroot/mozilla/xpinstall/src/nsInstall.h,v  <--  nsInstall.h
new revision: 1.90; previous revision: 1.89
done
Checking in src/nsJSInstall.cpp;
/cvsroot/mozilla/xpinstall/src/nsJSInstall.cpp,v  <--  nsJSInstall.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in src/nsSoftwareUpdate.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.cpp,v  <--  nsSoftwareUpdate.cpp
new revision: 1.100; previous revision: 1.99
done
Checking in src/nsSoftwareUpdate.h;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.h,v  <--  nsSoftwareUpdate.h
new revision: 1.41; previous revision: 1.40
done
Checking in src/nsSoftwareUpdateRun.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp,v  <-- 
nsSoftwareUpdateRun.cpp
new revision: 1.79; previous revision: 1.78
done
Checking in src/nsXPITriggerInfo.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.cpp,v  <--  nsXPITriggerInfo.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in src/nsXPITriggerInfo.h;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.h,v  <--  nsXPITriggerInfo.h
new revision: 1.15; previous revision: 1.14
done
Checking in src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.110; previous revision: 1.109
done
Checking in src/nsXPInstallManager.h;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.h,v  <--  nsXPInstallManager.h
new revision: 1.35; previous revision: 1.34
done
Checking in stub/xpistub.cpp;
/cvsroot/mozilla/xpinstall/stub/xpistub.cpp,v  <--  xpistub.cpp
new revision: 1.46; previous revision: 1.45
done

changes landed on the trunk.  note that we still wanna adjust the maximun cert
define.

Comment 29

[Jimmy Lee]

Current TRUNK builds have support.  The test cases at
http://www.mozilla.org/projects/xpinstall/signed/testcases/index.html are
supported.  Marking Verified!