708690 - MAR files do not protect against applying an update for the wrong product and channel, nor against version downgrades

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ian Melven :imelven]

Although MAR files will be signed as part of bug 699700, the specific product to update is not part of the signed MAR file itself. This means a legitimately signed but older MAR file for the wrong product could be installed. There are other checks around MAR files - a SHA-512 hash of the MAR is downloaded over SSL and used to verify the MAR file downloaded over http was not tampered with. Circumventing this protection would require compromising the update server's SSL session via either attacking the server and its private key directly, or forging/hijacking a cert that would be trusted for the SSL session.

Comment 1

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

Each product should have their own certificate embedded in the updater so this should be already handled.

catlee: we aren't planning on using the same certificates for multiple products are we?

I'm not sure if you are asking for this bug to also handle the second part regarding SSL, downloading the mar, etc. which already has several checks that security has asked for. If so, please check if there is already a bug filed to change what security asked to have implemented and if none of those account for this then please file a new bug and close any of those pre-existing bugs that the new bug supercedes.

Comment 2

[Ian Melven :imelven]

(In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> Each product should have their own certificate embedded in the updater so
> this should be already handled.

cool, if this turns out to be the case this can be withdrawn as INVALID imo. Will wait for confirmation from catlee.

> 
> I'm not sure if you are asking for this bug to also handle the second part
> regarding SSL, downloading the mar, etc. which already has several checks
> that security has asked for. If so, please check if there is already a bug
> filed to change what security asked to have implemented and if none of those
> account for this then please file a new bug and close any of those
> pre-existing bugs that the new bug supercedes.

no, i'm just stating the current understanding of the remote attacker case and the mitigations we have in place for that currently and what would have to happen to enable that case - the MAR signing work is a good mitigation in addition to the ones in place already even if the SSL connection is compromised (don't need to just rely on the hash being correct in that case, the signature must be correct also).

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> Each product should have their own certificate embedded in the updater so
> this should be already handled.

Is that a requirement that RelEng is aware of and QA will test for? Is it documented clearly enough in the right place(s) so that in a few years when someone new is getting our second or third batch of replacement certs they don't "optimize" the process to all use the same?

Although that all can work, wouldn't it be safer and clearer to simply add the appropriate meta-data to the .MAR file format itself? We get a bunch of useful information from AUS's update.xml that is separate from the .MAR file, that we use as part of the update decision, but that can only be trusted by the application that downloaded update.xml and isn't securely passed on to the actual updater (which might not run until a later time).

Comment 4

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

Possibly but as I essentially stated elsewhere, the possibility of a user of a system (note: the guest account is disabled by default) doing what this bug suggests to another user is to say the least rather unlikely. I really don't think the additional work to mitigate this along with risk / reward makes this something we should be all that concerned about. I am also less concerned about this since only admins will be able to update for now per bug 708688.

Comment 5

[Brian R. Bondy [:bbondy]]

RelEng had no major concerns about this task by the way, only the channel one.

Comment 6

[Brian R. Bondy [:bbondy]]

catlee suggested we use the application ID if we are doing this task.
I found we can get at that with MOZ_APP_ID.

Comment 7

[Brian R. Bondy [:bbondy]]

I updated the documentation for the MAR file format to describe the new section and exactly how additional sections will be added in the future if any.

The new section contains the product, channel and version information.

https://wiki.mozilla.org/Software_Update:MAR

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

The new file format is on the wiki, I can do a diff here if you want.

This is the base libmar code for writing a product information block inside libmar and reading that info out. 
It obtains the info by using: MOZ_APP_ID, MOZ_UPDATE_CHANNEL, and MOZ_APP_VERSION.

The sign/verify code doesn't change at all which is nice.
I'll post separate patches for using this info inside updater itself.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: File format documentation changes. v1.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Documentation. Patch v2.

Removed field I expected to have needed from documentation but didn't need after all.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v1.

This patch makes updater.exe return an error if the product doesn't match exactly, channel doesn't match exactly, or version is older than the current version.

RelEng has some concerns with the channel not yet addressed, but this is a good first step to getting the work complete and to spur discussion.

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v2.

Added export to mar.h, adjusted a couple comments.

Comment 13

[Ian Melven :imelven]

Comment on attachment 589773 [details] [diff] [review]
Documentation.  Patch v2.

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

looks good, a few minor comments :)

::: modules/libmar/MAR_FILE_FORMAT.txt
@@ +12,3 @@
>  
>  SIGNATURES
> +  8 bytes : FileSize - size in bytes of the entire MAR file

why did FileSize move into SIGNATURES and out of the header ?

@@ +25,5 @@
> +  ''NumAdditionalSections ADDITIONAL_SECTION_ENTRY elements''
> +
> +ADDITIONAL_SECTION_ENTRY (Only present if NumAdditionalSections is more than 0)
> +  4 bytes: BlockSize - (Z)
> +  4 bytes: BlockIdentifier - Used to identify the optional section

please document existing possible values that have been implemented, this is done below but may be clearer to also put that 1 = product info block here

@@ +29,5 @@
> +  4 bytes: BlockIdentifier - Used to identify the optional section
> +  (Z-8) bytes: The rest of the block.
> +
> +CONTENT
> +  Y bytes : content data interpreted as per the INDEX_ENTRY elements.

where does Y come from ? 

i assume it's total mar size minus header, signature blocks and additional sections ?

@@ +90,5 @@
> +PRODUCT INFORMATION BLOCK
> +
> +  4 bytes: Block size
> +  4 bytes: Block identifier with a value of 1
> +  <64 bytes: ApplicationID (such as from MOZ_APP_ID) Example: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}

this is for the product i assume

Comment 14

[Brian R. Bondy [:bbondy]]

Thanks for looking at the doc. 

> why did FileSize move into SIGNATURES and out of the header ?

Instead of calling things old mar and new mar, since the mar format is changing to another new mar. I thought it was easier to discuss having a signature block, and having additional blocks.  It makes no difference for the code, just helps discussing/naming variables.  Otherwise we'd have to say: has signature block yes/no? Has mar file size yes/no? Has additional blocks yes/no?.  Now we can remove the one question about MAR file size because it's inclusive to the other question about signature block.

> > +  <64 bytes: ApplicationID (such as from MOZ_APP_ID) Example: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> this is for the product i assume

Correct.

Regarding the other comments I'll update the doc.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Documentation. Patch v3.

Comment 16

[Ian Melven :imelven]

Comment on attachment 589937 [details] [diff] [review]
Documentation. Patch v3.

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

LGTM

Comment 17

[Ian Melven :imelven]

Comment on attachment 589878 [details] [diff] [review]
libmar code. Patch v2.

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

a few small commments/questions.

::: modules/libmar/src/mar_cmdline.h
@@ +54,5 @@
>             cannot be determined.
>   */
> +int get_mar_file_info(const char *path, 
> +                      int *has_signature_block,
> +                      int *has_product_info_block);

comment block above function needs to be updated

::: modules/libmar/src/mar_create.c
@@ +166,5 @@
> +  product_info_block_size = sizeof(product_info_block_size) +
> +    sizeof(additional_block_id) +
> +    strlen(product_info->applicationID) +
> +    strlen(product_info->channelName) + 
> +    strlen(product_info->productVersion) + 3;

maybe add a comment that per spec the appID, channelName and productVersion are all
NULL terminated in the MAR, hence the + 3

@@ +175,5 @@
> +  if (fwrite(&product_info_block_size, 
> +      sizeof(product_info_block_size), 1, fp) != 1) {
> +    return 1;
> +  }
> +  product_info_block_size = ntohl(product_info_block_size);

why do we have to flip this back ? doesn't look like it's used again or returned ?

@@ +183,5 @@
> +  if (fwrite(&additional_block_id, 
> +      sizeof(additional_block_id), 1, fp) != 1) {
> +    return 1;
> +  }
> +  additional_block_id = ntohl(additional_block_id);

same question as above

@@ +254,5 @@
> +  if (fwrite(&num_additional_sections, 
> +             sizeof(num_additional_sections), 1, fp) != 1) {
> +    goto failure;
> +  }
> +  num_additional_sections = ntohl(num_additional_sections);

same question as above about needing to do this

::: modules/libmar/src/mar_read.c
@@ +289,5 @@
> +                          sizeof(additionalBlockSize) - 
> +                          sizeof(additionalBlockID);
> +    /* This block must be at most 168 bytes.
> +       application ID < 64bytes, channel name < 64 bytes, and product
> +       version < 32 bytes + 3 NULL terminator bytes. */

a little confusing - the block DATA minus size and id can be at most 160 bytes - the
entire block including the size and id can be 168. maybe clarify this.

@@ +310,5 @@
> +        return -1;
> +      }
> +
> +      /* Extract the application ID from the buffer.  For now we
> +         point to the stack allocated buffer but we will correct

by correct you really mean copy via strdup it seems :)

@@ +313,5 @@
> +      /* Extract the application ID from the buffer.  For now we
> +         point to the stack allocated buffer but we will correct
> +         this if we are within bounds of each fields max length. */
> +      location = buf;
> +      len = strlen(location);

what if this a malicious MAR and this field isn't null terminated in the file ? did we already check the signature before reading the additional info block ?

@@ +356,5 @@
> +      }
> +    }
> +  }
> +
> +  /* If we had a product info bloc we would have already returned */

nit: block

@@ +432,5 @@
> +  return rv;
> +}
> +
> +/**
> + * Determines if the MAR file is new or old.

not totally accurate any more ? :)

@@ +465,5 @@
> +     and numAdditionalBlocks must be non NULL. */
> +  if (!hasSignatureBlock && !hasAdditionalBlocks ||
> +      hasAdditionalBlocks && (!offsetAdditionalBlocks || 
> +                              !numAdditionalBlocks)) {
> +    return 1;

might be clearer with more parens here, but i tend to use them to death where not strictly needed...

@@ +547,5 @@
> +
> +  if (ftell(fp) == offsetToContent) {
> +    *hasAdditionalBlocks = 0;
> +  } else {
> +    /* We have a signature block so read in the number of signatures and 

this is not the signature block right, it's the product info/additional block ?

::: modules/libmar/tool/mar.c
@@ +167,5 @@
> +           productInfoBlock.channelName,
> +           productInfoBlock.productVersion);
> +    free(productInfoBlock.applicationID);
> +    free(productInfoBlock.channelName);
> +    free(productInfoBlock.productVersion);

this is a real nit here but what if reading the block fails or there's no block in the file - i suppose the worst problem is a crash in the command line util ?

Comment 18

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

One thing to consider. In bug 708697 we are considering putting the branch the build is allowed to update to in the updater and in the mar. If we do this it would be trivial to just include a product id in the same string and check both in one fell swoop instead of having two values to check. This will also make it simpler for 3rd parties since they will only have one value to set.

Comment 19

[Ian Melven :imelven]

Comment on attachment 589875 [details] [diff] [review]
Updater code.  Patch v1.

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

LGTM

::: toolkit/mozapps/update/updater/archivereader.cpp
@@ +154,5 @@
>  #endif
>  }
>  
> +/**
> + * Verifies that the MAR file matches the current product, channela nd version.

nit: typo

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v1'.

Fixed typo in comment.  I'll wait for plans to solidify in the channel check bug before asking for a review on this.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v3.

> > +  product_info_block_size = ntohl(product_info_block_size);
> why do we have to flip this back ? doesn't look like it's used again or returned ?
Thanks for the review Ian.  I'll wait to request another from rstrong until after we finalize channel decisions.

This is something that bsmith has been pretty strict on in the MAR signing task.  I think it is for protection in case we use it again in the future.

> > +      len = strlen(location);
> what if this a malicious MAR and this field isn't null terminated in the file ?
> did we already check the signature before reading the additional info block ?

We do check for signature first.  To be safe though I made the buffer 161 chars and I zero out the buffer myself now.


> > +    free(productInfoBlock.applicationID);
> > +    free(productInfoBlock.channelName);
> > +    free(productInfoBlock.productVersion);

> this is a real nit here but what if reading the block fails or there's no 
> block in the file - i suppose the worst problem is a crash in the command line 
> util ?

It's safe to free a NULL pointer but you're right I don't always NULL these out on every failure condition.  So I only free them on function success now.

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Build config for new MAR channel value. Patch v1

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v4.

As per rs' suggestion.

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v5.

Fixed some variable names that were named_like_this (This module has some non Mozilla standard coding standards in place).

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v1.9

Updating current implementation for product/channel/version checking.

Just a minor thing remaining until I ask for review on this patch:

> // TODO: Read this from application.ini
>  const char *MARChannelName = MOZ_MAR_CHANNEL;
>  const char *productVersion = MOZ_APP_VERSION;

Comment 26

[Brian R. Bondy [:bbondy]]

rstrong, could you confirm if application.ini is an acceptable place for this new define for QA to change? I'll wait to do what was mentioned in Comment 25 until I hear back from you.

Comment 27

[Brian R. Bondy [:bbondy]]

Attachment: Documentation Patch v4.

Added documentation relating to optional unused data at the end of the product information block.  This is helpful to enforce using a whole product information block max size so that we don't need to rebuild a MAR when we want to repurpose it to another channel.  I.e. we can just seek to that position pre-signed and re-write the product information block.

Comment 28

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #27)
> Created attachment 591249 [details] [diff] [review]
> Documentation Patch v4.
> 
> Added documentation relating to optional unused data at the end of the
> product information block.  This is helpful to enforce using a whole product
> information block max size so that we don't need to rebuild a MAR when we
> want to repurpose it to another channel.  I.e. we can just seek to that
> position pre-signed and re-write the product information block.

when you say 'pre-signed' that means before signing the entire MAR file including the product info block right ? just checking :)

Comment 29

[Brian R. Bondy [:bbondy]]

Ya so basically the steps are:
- Create a MAR with one command (which auto embeds a product information block)
- Sign it with another command
- You can then strip the signature with another command
- Then you can refresh the product information block with another command
- Then you can resign it again.

Comment 30

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

(In reply to Brian R. Bondy [:bbondy] from comment #26)
> rstrong, could you confirm if application.ini is an acceptable place for
> this new define for QA to change? I'll wait to do what was mentioned in
> Comment 25 until I hear back from you.
Had to think about this for a bit. It needs to be in its own file since application.ini is updated and it would break QA partial update tests when they need to modify this information.

Comment 31

[Brian R. Bondy [:bbondy]]

k thanks, I canceled review and I'll submit a new patch tomorrow.

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v2

Updater code for product-channel and version checking.  The product-channel combined value is loaded from the mar-archive.ini file which gets installed into the installation directory.  That value must match the value in the MAR being applied.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Build config for new MAR channel value. Patch v2

Build config that uses mar-channel.ini instead of application.ini

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: XPCShell tests working with signed and product info block MARs. Patch v1.

This patch enables xpcshell tests to work with signed MARs and also does a product info block check.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: This patch simply contains the new MAR files. Patch v1.

I kept the MAR files separate so you wouldn't have to scroll as much through the patch file.  The MARs are the same as the old ones but they have a product info block and are signed now using the xpcshell cert.

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v6.

The extra output added to -t to give info about product info blocks and signing was causing problems with some build packaging scripts on elm.  So I moved the more verbose -t output to a -T option instead.  

Now -t remains the same with this patch.

Comment 37

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v3

- Fixed wrong product version compare, should have checked for -1 not 1
- Moved wrapper ini string reading function out of readstrings code since we are keeping that generic as per a review on a different bug.

Comment 38

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

Comment on attachment 591249 [details] [diff] [review]
Documentation Patch v4.

>...
>+=== Product Information Block ===
>+
>+The product information block identifies the product and channel this MAR file applies to.  It also includes the new version number to avoid downgrades. 
>+The product and channel are combined in the MARChannelName field.
>+
>+PRODUCT INFORMATION BLOCK
>+
>+  4 bytes: BlockSize - The size of the product information block.
>+  4 bytes: BlockIdentifier - The identifier of the product information block, with a value of 1
>+  <64 bytes: MARChannelName used to identify the product and channel (such as from MOZ_CHANNEL_NAME) Example: mozilla-central
Please use an example that won't be confused with the mozilla-central repo. Perhaps Firefox-Aurora

Comment 39

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

I suspect you are using the repo name due to other repos accidentally picking this up. How about firefox-mozilla-central? btw: I'm fine with lowercase, etc.

Comment 40

[Brian R. Bondy [:bbondy]]

firefox-mozilla-central sounds good.
I just used repo name since it should to be a unique value per repo.

Comment 41

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

Comment on attachment 591809 [details] [diff] [review]
Build config for new MAR channel value. Patch v2

>diff --git a/browser/confvars.sh b/browser/confvars.sh
>--- a/browser/confvars.sh
>+++ b/browser/confvars.sh
>@@ -54,11 +54,12 @@ MOZ_SERVICES_SYNC=1
> MOZ_APP_VERSION=$FIREFOX_VERSION
> MOZ_EXTENSIONS_DEFAULT=" gnomevfs"
> # MOZ_APP_DISPLAYNAME will be set by branding/configure.sh
> # Changing either of these values requires a clobber to ensure correct results,
> # because branding dependencies are broken.
> MOZ_BRANDING_DIRECTORY=browser/branding/nightly
> MOZ_OFFICIAL_BRANDING_DIRECTORY=browser/branding/official
> MOZ_APP_ID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
>+MOZ_MAR_CHANNEL=mozilla-central
firefox-mozilla-central per previous comment

> MOZ_PROFILE_MIGRATOR=1
> MOZ_EXTENSION_MANAGER=1
> MOZ_APP_STATIC_INI=1
>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -89,16 +89,17 @@
> ; [Base Browser Files]
> #ifndef XP_UNIX
> @BINPATH@/@MOZ_APP_NAME@.exe
> #else
> @BINPATH@/@MOZ_APP_NAME@-bin
> @BINPATH@/@MOZ_APP_NAME@
> #endif
> @BINPATH@/application.ini
>+@BINPATH@/mar-channel.ini
This should be update-settings... right? Is the rename in a different patch?

>diff --git a/build/Makefile.in b/build/Makefile.in
>--- a/build/Makefile.in
>+++ b/build/Makefile.in
>@@ -150,16 +151,20 @@ _LEAKTEST_FILES =    \
> 		$(topsrcdir)/build/pgo/blueprint/fancytype-screen.css \
> 		$(NULL)
> 
> leaktest.py: leaktest.py.in
> 	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $^ > $@
> 	chmod +x $@
> GARBAGE += leaktest.py
> 
>+mar-channel.ini: mar-channel.ini.in $(APP_INI_DEPS)
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $< > $@
>+GARBAGE += mar-channel.ini
I wonder if Kyle knows of a cleaner way to do this similar to EXTRA_PP_COMPONENTS?

>diff --git a/build/mar-channel.ini.in b/build/mar-channel.ini.in
>new file mode 100644
>--- /dev/null
>+++ b/build/mar-channel.ini.in
>@@ -0,0 +1,45 @@
>+#if MOZ_APP_STATIC_INI
>+; If you modify this file updates may fail.
>+; Do not modify this file.
>+#endif
What is the purpose of MOZ_APP_STATIC_INI?

>+#filter substitution
>+[Channel]
>+MAR_CHANNEL=@MOZ_MAR_CHANNEL@
I think there might be a better name for this. Perhaps MAR_CHANNEL_ID so it is a little clearer that this isn't the same as the update channel?

Minusing mainly due to the questions and the next pass should be r+

Comment 42

[Brian R. Bondy [:bbondy]]

Attachment: Build config for new MAR channel value. Patch v3

> This should be update-settings... right? Is the rename in a different patch?

Sorry, already had this updated locally but not in this uploaded patch.

> What is the purpose of MOZ_APP_STATIC_INI?

It is to control what will appear in the generated ini file. 
If it is defined then you get that output in the generated ini file.

> +MAR_CHANNEL=@MOZ_MAR_CHANNEL@
> I think there might be a better name for this. Perhaps MAR_CHANNEL_ID so it is a little 
> clearer that this isn't the same as the update channel?
> Minusing mainly due to the questions and the next pass should be r+

OK, warning that you may be paying the price with some rebasing emails though :)

Comment 43

[Brian R. Bondy [:bbondy]]

Attachment: Documentation Patch v5.

Updated documentation for new ID name.  Carrying forward r+.

Comment 44

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v7.

Updated with new define name.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v4.

Updated to new MAR ID define name.

Comment 46

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

(In reply to Brian R. Bondy [:bbondy] from comment #42)
> Created attachment 595027 [details] [diff] [review]
> Build config for new MAR channel value. Patch v3
> 
> > This should be update-settings... right? Is the rename in a different patch?
> 
> Sorry, already had this updated locally but not in this uploaded patch.
> 
> > What is the purpose of MOZ_APP_STATIC_INI?
> 
> It is to control what will appear in the generated ini file. 
> If it is defined then you get that output in the generated ini file.
Yes but why is it needed?

Comment 47

[Brian R. Bondy [:bbondy]]

> Yes but why is it needed?

It probably isn't needed, I just copied the same pattern as application.ini.in had.

Comment 48

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

That was added by Bug 686466 which is mainly if not entirely about Android. Since Android uses apk's I don't think it is needed and this file shouldn't be present on Android.

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Build config for new MAR channel value. Patch v4.

Removed the unneeded define in the update-settings.ini file.

Comment 50

[Brian R. Bondy [:bbondy]]

Sorry I should have done an hg annotate at the point of Comment 47.

Comment 51

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

Comment on attachment 595029 [details] [diff] [review]
libmar code. Patch v7.

>diff --git a/modules/libmar/src/mar.h b/modules/libmar/src/mar.h
>--- a/modules/libmar/src/mar.h
>+++ b/modules/libmar/src/mar.h
>@@ -41,16 +41,21 @@
> 
> /* We use NSPR here just to import the definition of PRUint32 */
> #include "prtypes.h"
> 
> #ifdef __cplusplus
> extern "C" {
> #endif
> 
>+struct ProductInformationBlock {
>+  const char *MARChannelName;
How about MARChannelID as noted previously here and elsewhere?

>diff --git a/modules/libmar/src/mar_cmdline.h b/modules/libmar/src/mar_cmdline.h
>--- a/modules/libmar/src/mar_cmdline.h
>+++ b/modules/libmar/src/mar_cmdline.h
>@@ -41,24 +41,41 @@
> /* We use NSPR here just to import the definition of PRUint32 */
> #include "prtypes.h"
> 
> #ifdef __cplusplus
> extern "C" {
> #endif
> 
> /**
>- * Determines if the MAR file is new or old.
>- * 
>- * @param path   The path of the MAR file to check.
>- * @param oldMar An out parameter specifying if the MAR file is new or old.
>+ * Determines the type of MAR file we're dealing with.
"Determines the type of MAR" doesn't make any sense to me since this function gets the mar file info.

>+ *
>+ * @param path                   The path of the MAR file to check.
>+ * @param hasSignatureBlock      Optional out parameter specifying if the MAR
>+ *                               file is has a signature block or not.
>+ * @param numSignatures          Optional out parameter for storing the number
>+ *                               of signatures in the MAR file.
>+ * @param hasAdditionalBlocks    Optional out parameter specifying if the MAR
>+ *                               file has additional blocks or not.
>+ * @param offsetAdditionalBlocks Optional out parameter for the offset to the 
>+ *                               first additional block. Value is only valid if
>+ *                               has_additional_blocks
>+ *                               is not equal to 0.
>+ * @param numAdditionalBlocks    Optional out parameter for the number of
>+ *                               additional blocks.  Value is only valid if
>+ *                               has_additional_blocks is not euqal to 0.
>  * @return A non-zero value if an error occurred and the information 
>-           cannot be determined.
>+ *         cannot be determined.
>  */
>-int is_old_mar(const char *path, int *oldMar);
>+int get_mar_file_info(const char *path, 
>+                      int *hasSignatureBlock,
>+                      int *numSignatures,
>+                      int *hasAdditionalBlocks,
>+                      int *offsetAdditionalBlocks,
>+                      int *numAdditionalBlocks);

Comment 52

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

Comment on attachment 595029 [details] [diff] [review]
libmar code. Patch v7.

>diff --git a/modules/libmar/src/mar_create.c b/modules/libmar/src/mar_create.c
>--- a/modules/libmar/src/mar_create.c
>+++ b/modules/libmar/src/mar_create.c
>@@ -119,20 +121,206 @@ static int mar_concat_file(FILE *fp, con
>       break;
>     }
>   }
> 
>   fclose(in);
>   return rv;
> }
> 
>-int mar_create(const char *dest, int num_files, char **files) {
>+/**
>+ * Writes out the product information block to the specified file.
>+ *
>+ * @param fp           The opened MAR file being created.
>+ * @param stack        A pointer to the MAR item stack being used to create 
>+ *                     the MAR
>+ * @param product_info The product info block to store in the file.
>+ * @return 0 on success.
>+*/
>+static int
>+mar_concat_product_info_block(FILE *fp, 
>+                              struct MarItemStack *stack,
>+                              struct ProductInformationBlock *productInfoBlock)
>+{
>+  char buf[PIB_MAX_MAR_CHANNEL_SIZE + PIB_MAX_PRODUCT_VERSION_SIZE];
>+  PRUint32 additionalBlockID = 1, productInfoBlockSize, unused;
>+  if (!fp || !productInfoBlock || 
>+    !productInfoBlock->MARChannelName ||
>+    !productInfoBlock->productVersion) {
nit: alignment

>+    return 1;
>+  }
>+ 
>+  /* The MAR channel name must be < 64 bytes per the spec */
>+  if (strlen(productInfoBlock->MARChannelName) > PIB_MAX_MAR_CHANNEL_SIZE) {
>+    return 1;
>+  }
>+
>+  /* The product version must be < 32 bytes per the spec */
>+  if (strlen(productInfoBlock->productVersion) > PIB_MAX_PRODUCT_VERSION_SIZE) {
>+    return 1;
>+  }
>+
>+  /* Although we don't need the product information block size to include the
>+     maximum MAR channel name and product version, we allocate the maximum
>+     amount to make it easier to modify the MAR file for repurposing MAR files
>+     to different MAR channels. + 2 is for the NULL terminators. */
>+  productInfoBlockSize = sizeof(productInfoBlockSize) +
>+                         sizeof(additionalBlockID) +
>+                         PIB_MAX_MAR_CHANNEL_SIZE +
>+                         PIB_MAX_PRODUCT_VERSION_SIZE + 2;
>+  if (stack) {
>+    stack->last_offset += productInfoBlockSize;
>+  }
>+
>+  /* Write out the product info block size */
>+  productInfoBlockSize = htonl(productInfoBlockSize);
>+  if (fwrite(&productInfoBlockSize, 
>+      sizeof(productInfoBlockSize), 1, fp) != 1) {
>+    return 1;
>+  }
>+  productInfoBlockSize = ntohl(productInfoBlockSize);
>+
>+  /* Write out the product info block ID */
>+  additionalBlockID = htonl(additionalBlockID);
>+  if (fwrite(&additionalBlockID, 
>+      sizeof(additionalBlockID), 1, fp) != 1) {
>+    return 1;
>+  }
>+  additionalBlockID = ntohl(additionalBlockID);
>+
>+  /* Write out the channel name and NULL terminator */
>+  if (fwrite(productInfoBlock->MARChannelName, 
>+      strlen(productInfoBlock->MARChannelName) + 1, 1, fp) != 1) {
>+    return 1;
>+  }
>+
>+  /* Write out the product version string and NULL terminator */
>+  if (fwrite(productInfoBlock->productVersion, 
>+      strlen(productInfoBlock->productVersion) + 1, 1, fp) != 1) {
>+    return 1;
>+  }
>+
>+  /* Write out the rest of the block that is unused */
>+  unused = productInfoBlockSize - (sizeof(productInfoBlockSize) +
>+                                   sizeof(additionalBlockID) +
>+                                   strlen(productInfoBlock->MARChannelName) + 
>+                                   strlen(productInfoBlock->productVersion) + 2);
>+  memset(buf, 0, sizeof(buf));
>+  if (fwrite(buf, unused, 1, fp) != 1) {
>+    return 1;
>+  }
>+  return 0;
>+}
>+
>+/** 
>+ * Refreshes the product information block with the new information.
>+ * The input MAR must not be signed or the function call will fail.
Please lest releng know about this limitation since they will need to keep an usigned mar around if they want to repurpose it.

>+ * 
>+ * @param path             The path to the MAR file who's product info block
>+ *                         should be refreshed.
>+ * @param productInfoBlock Out parameter for where to store the result to
>+ * @return 0 on success, -1 on failure
You also return 1 on failure below. I also find it somewhat weird that you return 1 on failure in mar_concat_product_info_block and -1 for the most part everywhere else.

>+*/
>+int
>+refresh_product_info_block(const char *path,
>+                           struct ProductInformationBlock *productInfoBlock)
>+{
>+  FILE *fp ;
>+  int rv;
>+  PRUint32 hasSignatureBlock, numSignatures, additionalBlocks, 
>+    additionalBlockSize, additionalBlockID, offsetAdditionalBlocks, 
>+    numAdditionalBlocks, i;
>+  PRInt64 oldPos;
>+
>+  rv = get_mar_file_info(path, 
>+                         &hasSignatureBlock,
>+                         &numSignatures,
>+                         &additionalBlocks,
>+                         &offsetAdditionalBlocks,
>+                         &numAdditionalBlocks);
>+  if (rv) {
>+    fprintf(stderr, "ERROR: Could not obtain MAR information.\n");
>+    return -1;
>+  }
>+
>+  if (hasSignatureBlock && numSignatures) {
>+    fprintf(stderr, "ERROR: Cannot refresh a signed MAR\n");
>+    return -1;
>+  }
>+
>+  fp = fopen(path, "r+b");
>+  if (!fp) {
>+    fprintf(stderr, "ERROR: could not open target file: %s\n", path);
>+    return -1;
>+  }
>+
>+  if (fseeko(fp, offsetAdditionalBlocks, SEEK_SET)) {
>+    fprintf(stderr, "ERROR: could not seek to additional blocks\n");
>+    fclose(fp);
>+    return -1;
>+  }
>+
>+  for (i = 0; i < numAdditionalBlocks; ++i) {
>+    /* Get the position of the start of this block */
>+    oldPos = ftello(fp);
>+
>+    /* Read the additional block size */
>+    if (fread(&additionalBlockSize, 
>+              sizeof(additionalBlockSize), 
>+              1, fp) != 1) {
>+      return -1;
>+    }
>+    additionalBlockSize = ntohl(additionalBlockSize);
>+
>+    /* Read the additional block ID */
>+    if (fread(&additionalBlockID, 
>+              sizeof(additionalBlockID), 
>+              1, fp) != 1) {
>+      return -1;
>+    }
>+    additionalBlockID = ntohl(additionalBlockID);
>+
>+    if (PRODUCT_INFO_BLOCK_ID == additionalBlockID) {
>+      if (fseeko(fp, oldPos, SEEK_SET)) {
>+        fprintf(stderr, "Could not seek back to Product Information Block\n");
>+        fclose(fp);
>+        return 1;
>+      }
>+
>+      if (mar_concat_product_info_block(fp, NULL, productInfoBlock)) {
>+        fprintf(stderr, "Could not concat Product Information Block\n");
>+        fclose(fp);
>+        return 1;
>+      }
>+
>+      fclose(fp);
>+      return 0;
>+    } else {
>+      /* This is not the additional block you're looking for. Move along. */
>+      if (fseek(fp, additionalBlockSize, SEEK_CUR)) {
>+        fprintf(stderr, "ERROR: Could not seek past current block.\n");
>+        fclose(fp);
>+        return -1;
>+      }
>+    }
>+  }
>+
>+  /* If we had a product info block we would have already returned */
>+  fclose(fp);
>+  fprintf(stderr, "ERROR: Could not refresh because block does not exist\n");
>+  return 1;
>+}
>+

Could you add a function comment while you are here?
>+int mar_create(const char *dest, int 
>+               num_files, char **files, 
>+               struct ProductInformationBlock *productInfoBlock) {
>   struct MarItemStack stack;
>-  PRUint32 offset_to_index = 0, size_of_index, num_signatures;
>-  PRUint64 size_of_entire_MAR = 0;
>+  PRUint32 offset_to_index = 0, size_of_index, 
>+    numSignatures, numAdditionalSections;
>+  PRUint64 sizeOfEntireMAR = 0;
>   struct stat st;
>   FILE *fp;
>   int i, rv = -1;
> 
>   memset(&stack, 0, sizeof(stack));
> 
>   fp = fopen(dest, "wb");
>   if (!fp) {

Comment 53

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

Comment on attachment 595029 [details] [diff] [review]
libmar code. Patch v7.

>diff --git a/modules/libmar/src/mar_private.h b/modules/libmar/src/mar_private.h
>--- a/modules/libmar/src/mar_private.h
>+++ b/modules/libmar/src/mar_private.h
>@@ -73,18 +73,26 @@ PR_STATIC_ASSERT(MAX_SIZE_OF_MAR_FILE < 
>    bytes per BLOCKSIZE bytes */
> PR_STATIC_ASSERT(sizeof(BLOCKSIZE) < \
>   (SIGNATURE_BLOCK_OFFSET + sizeof(PRUint32)));
> 
> /* The maximum size of any signature supported by current and future
>    implementations of the signmar program. */
> #define MAX_SIGNATURE_LENGTH 2048
> 
>+/* Each additional block has a unique ID.  
>+   The product information block has an ID of 1. */
>+#define PRODUCT_INFO_BLOCK_ID 1
>+
> #define MAR_ITEM_SIZE(namelen) (3*sizeof(PRUint32) + (namelen) + 1)
> 
>+/* Product Information Block (PIB) constants */
>+#define PIB_MAX_MAR_CHANNEL_SIZE 63
Perhaps PIB_MAX_MAR_CHANNEL_ID_SIZE

>diff --git a/modules/libmar/src/mar_read.c b/modules/libmar/src/mar_read.c
>--- a/modules/libmar/src/mar_read.c
>+++ b/modules/libmar/src/mar_read.c
>@@ -230,16 +230,132 @@ void mar_close(MarFile *mar) {
>...
>+/** 
>+ * Reads the product info block from the MAR file's additional block section.
>+ * The caller is responsible for freeing the fields in productInfoBlock
>+ * if the return is successful.
>+ *
>+ * @param productInfoBlock Out parameter for where to store the result to
>+ * @return 0 on success, -1 on failure
>+*/
>+int
>+mar_read_product_info_block(MarFile *mar, 
>+                            struct ProductInformationBlock *productInfoBlock)
>+{
>+  int i, hasAdditionalBlocks, offset, 
>+    offsetAdditionalBlocks, numAdditionalBlocks,
>+    additionalBlockSize, additionalBlockID;
>+  char buf[97] = { '\0' };
Add a comment regarding why 97 was chosen similar to the one below

>+  int ret = get_mar_file_info_fp(mar->fp, NULL, NULL,
>+                                 &hasAdditionalBlocks, 
>+                                 &offsetAdditionalBlocks, 
>+                                 &numAdditionalBlocks);
>+  for (i = 0; i < numAdditionalBlocks; ++i) {
>+    /* Read the additional block size */
>+    if (fread(&additionalBlockSize, 
>+              sizeof(additionalBlockSize), 
>+              1, mar->fp) != 1) {
>+      return -1;
>+    }
>+    additionalBlockSize = ntohl(additionalBlockSize) - 
>+                          sizeof(additionalBlockSize) - 
>+                          sizeof(additionalBlockID);
>+
>+    /* Read the additional block ID */
>+    if (fread(&additionalBlockID, 
>+              sizeof(additionalBlockID), 
>+              1, mar->fp) != 1) {
>+      return -1;
>+    }
>+    additionalBlockID = ntohl(additionalBlockID);
>+
>+    if (PRODUCT_INFO_BLOCK_ID == additionalBlockID) {
>+      const char *location;
>+      int len;
>+
>+      /* This block must be at most 104 bytes.
>+          MAR channel name < 64bytes, and product version < 32 bytes + 3 NULL 
>+          terminator bytes. We only check for 96 though because we remove 8 bytes
>+          above from the additionaBlockSize */
s/additionaBlockSize/additionalBlockSize

Call out where the 8 bytes are removed.

>+      if (additionalBlockSize > 96) {
>+        return -1;
>+      }
>+
>+    if (fread(buf, additionalBlockSize, 1, mar->fp) != 1) {
>+        return -1;
>+      }
>+
>+      /* Extract the MAR channel name from the buffer.  For now we
>+         point to the stack allocated buffer but we strdup this
>+         if we are within bounds of each field's max length. */
>+      location = buf;
>+      len = strlen(location);
>+      productInfoBlock->MARChannelName = location;
>+      location += len + 1;
>+      if (len >= 64) {
>+        productInfoBlock->MARChannelName = NULL;
>+        return -1;
>+      }
>+
>+      /* Extract the version from the buffer */
>+      len = strlen(location);
>+      productInfoBlock->productVersion = location;
>+      location += len + 1;
>+      if (len >= 32) {
>+        productInfoBlock->MARChannelName = NULL;
>+        productInfoBlock->productVersion = NULL;
>+        return -1;
>+      }
>+      productInfoBlock->MARChannelName = 
>+        strdup(productInfoBlock->MARChannelName);
>+      productInfoBlock->productVersion = 
>+        strdup(productInfoBlock->productVersion);
>+      return 0;
>+    } else {
>+      /* This is not the additional block you're looking for. Move along. */
>+      if (fseek(mar->fp, additionalBlockSize, SEEK_CUR)) {
>+        return -1;
>+      }
>+    }
>+  }
>+
>+  /* If we had a product info block we would have already returned */
>+  return 1;
Check you're using the documented return values as previously noted here and elsewhere.

>+}
>+
> const MarItem *mar_find_item(MarFile *mar, const char *name) {
>   PRUint32 hash;
>   const MarItem *item;
> 
>   hash = mar_hash_name(name);
> 
>   item = mar->item_table[hash];
>   while (item && strcmp(item->name, name) != 0)
>@@ -279,72 +395,176 @@ int mar_read(MarFile *mar, const MarItem
>     nr = bufsize;
> 
>   if (fseek(mar->fp, item->offset + offset, SEEK_SET))
>     return -1;
> 
>   return fread(buf, 1, nr, mar->fp);
> }
> 
>-/**
>- * Determines if the MAR file is new or old.
>- * 
>- * @param path   The path of the MAR file to check.
>- * @param oldMar An out parameter specifying if the MAR file is new or old.
>- * @return A non-zero value if an error occurred and the information 
>-           cannot be determined.
>- */
>-int is_old_mar(const char *path, int *oldMar)
>+

Could you add a function comment?

>+int get_mar_file_info(const char *path, 
>+                      int *hasSignatureBlock,
>+                      int *numSignatures,
>+                      int *hasAdditionalBlocks,
>+                      int *offsetAdditionalBlocks,
>+                      int *numAdditionalBlocks)
> {
>-  PRUint32 offsetToIndex, offsetToContent, oldPos;
>-  FILE *fp;
>-  
>-  if (!oldMar) {
>-    return -1;
>-  }
>-
>-  fp = fopen(path, "rb");
>+  int rv;
>+  FILE *fp = fopen(path, "rb");
>   if (!fp) {
>     return -1;
>   }
> 
>-  oldPos = ftell(fp);
>+  rv = get_mar_file_info_fp(fp, hasSignatureBlock, 
>+                            numSignatures, hasAdditionalBlocks,
>+                            offsetAdditionalBlocks, numAdditionalBlocks);
>+
>+  fclose(fp);
>+  return rv;
>+}
>+
>+/**
>+ * Determines the type of MAR file we're dealing with.
"Determines the type of MAR" doesn't make any sense to me since this function gets the mar file info.

>+ *
>+ * @param path                   The path of the MAR file to check.
>+ * @param hasSignatureBlock      Optional out parameter specifying if the MAR
>+ *                               file is has a signature block or not.
>+ * @param numSignatures          Optional out parameter for storing the number
>+ *                               of signatures in the MAR file.
>+ * @param hasAdditionalBlocks    Optional out parameter specifying if the MAR
>+ *                               file has additional blocks or not.
>+ * @param offsetAdditionalBlocks Optional out parameter for the offset to the 
>+ *                               first additional block. Value is only valid if
>+ *                               has_additional_blocks
>+ *                               is not equal to 0.
>+ * @param numAdditionalBlocks    Optional out parameter for the number of
>+ *                               additional blocks.  Value is only valid if
>+ *                               has_additional_blocks is not euqal to 0.
>+ * @return A non-zero value if an error occurred and the information 
>+ *         cannot be determined.
I'd just go with the following here and elsewhere:
0 on success and non-zero on failure.

>+ */
>+int get_mar_file_info_fp(FILE *fp, 
>+                         int *hasSignatureBlock,
>+                         int *numSignatures,
>+                         int *hasAdditionalBlocks,
>+                         int *offsetAdditionalBlocks,
>+                         int *numAdditionalBlocks)
>+{
>+  PRUint32 offsetToIndex, offsetToContent, signatureCount, signatureLen;
>+  int i;
>+  
>+  /* One of hasSignatureBlock or hasAdditionalBlocks must be non NULL */
>+  if (!hasSignatureBlock && !hasAdditionalBlocks) {
>+    return 1;
>+  }
>+
> 
>   /* Skip to the start of the offset index */
>   if (fseek(fp, MAR_ID_SIZE, SEEK_SET)) {
>     return -1;
>   }
> 
>   /* Read the offset to the index. */
>   if (fread(&offsetToIndex, sizeof(offsetToIndex), 1, fp) != 1)
>     return -1;
>   offsetToIndex = ntohl(offsetToIndex);
> 
>+  if (numSignatures) {
>+     /* Skip past the MAR file size field */
>+    if (fseek(fp, sizeof(PRUint64), SEEK_CUR)) {
>+      return -1;
>+    }
>+
>+    /* Read the offset to the index. */
>+    if (fread(numSignatures, sizeof(*numSignatures), 1, fp) != 1)
>+      return -1;
nit: braces for consistency.

Comment 54

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

Comment on attachment 595029 [details] [diff] [review]
libmar code. Patch v7.

>diff --git a/modules/libmar/tool/mar.c b/modules/libmar/tool/mar.c
>--- a/modules/libmar/tool/mar.c
>+++ b/modules/libmar/tool/mar.c
>@@ -55,28 +55,31 @@ int NSSInitCryptoContext(const char *NSS
> 
> int mar_repackage_and_sign(const char *NSSConfigDir,
>                            const char *certName, 
>                            const char *src, 
>                            const char * dest);
> 
> static void print_usage() {
>   printf("usage:\n");
>-  printf("  mar [-C workingDir] {-c|-x|-t} archive.mar [files...]\n");
>+  printf("  mar [-H MARChannelName] [-V ProductVersion] [-C workingDir] "
>+         "{-c|-x|-t|-T} archive.mar [files...]\n");
> #ifndef NO_SIGN_VERIFY
>   printf("  mar [-C workingDir] -d NSSConfigDir -n certname -s "
>          "archive.mar out_signed_archive.mar\n");
> 
> #if defined(XP_WIN) && !defined(MAR_NSS)
>   printf("  mar [-C workingDir] -D DERFilePath -v signed_archive.mar\n");
> #else 
>   printf("  mar [-C workingDir] -d NSSConfigDir -n certname "
>     "-v signed_archive.mar\n");
Could you fix the indentation while you are here?

> #endif
> #endif
>+  printf("  mar [-H MARChannelName] [-V ProductVersion] [-C workingDir] "
>+         "-i unsigned_archive_to_refresh.mar\n");

Would be nice to add more details to the usage. File a followup bug?

>@@ -93,33 +96,36 @@ static int mar_test(const char *path) {
> 
>   mar_close(mar);
>   return 0;
> }
> 
> int main(int argc, char **argv) {
>   char *NSSConfigDir = NULL;
>   char *certName = NULL;
>+  char *MARChannelName = MAR_CHANNEL_ID;
>+  char *productVersion = MOZ_APP_VERSION;
So, this will be build specific? Could it just use update-settings.ini when creating the mar to determine these values instead?

>@@ -135,32 +141,89 @@ int main(int argc, char **argv) {
>       NSSConfigDir = argv[2];
>       argv += 2;
>       argc -= 2;
>      /* -n certName */
>     } else if (argv[1][0] == '-' && argv[1][1] == 'n') {
>       certName = argv[2];
>       argv += 2;
>       argc -= 2;
>-    } else {
>+    /* MAR channel name*/
channel ID

>+    } else if (argv[1][0] == '-' && argv[1][1] == 'H') {
>+      MARChannelName = argv[2];
>+      argv += 2;
>+      argc -= 2;
>+    /* Product Version */
>+    } else if (argv[1][0] == '-' && argv[1][1] == 'V') {
>+      productVersion = argv[2];
>+      argv += 2;
>+      argc -= 2;
>+    }
>+    else {
>       print_usage();
>       return -1;
>     }
>   }
> 
>   if (argv[1][0] != '-') {
>     print_usage();
>     return -1;
>   }
> 
>   switch (argv[1][1]) {
>-  case 'c':
>-    return mar_create(argv[2], argc - 3, argv + 3);
>+  case 'c': {
>+    struct ProductInformationBlock infoBlock;
>+    infoBlock.MARChannelName = MARChannelName;
>+    infoBlock.productVersion = productVersion;
>+    return mar_create(argv[2], argc - 3, argv + 3, &infoBlock);
Have you verified this doesn't break other apps besides Firefox (I saw that it returns 0 when no info is provided... I just want to make sure)?

>+  }
>+  case 'i': {
>+    struct ProductInformationBlock infoBlock;
>+    infoBlock.MARChannelName = MARChannelName;
>+    infoBlock.productVersion = productVersion;
>+    return refresh_product_info_block(argv[2], &infoBlock);
>+  }
>+  case 'T': {
>+    int rv;
>+    struct ProductInformationBlock productInfoBlock;
nit: unless there is a reason, use infoBlock as you have previously or change the others to productInfoBlock.

>@@ -186,22 +249,23 @@ int main(int argc, char **argv) {
>       CloseHandle(certFile);
>       free(certBuffer);
>       return -1;
>     }
>     CloseHandle(certFile);
> 
>     if (mar_verify_signature(argv[2], certBuffer, fileSize, NULL)) {
>       int oldMar = 0;
oldMar is no longer used.

>-      free(certBuffer);
> 
>       /* Determine if the source MAR file has the new fields for signing or not */
>-      if (is_old_mar(argv[2], &oldMar)) {
>+      int hasSignatureBlock;
>+      free(certBuffer);
>+      if (get_mar_file_info(argv[2], &hasSignatureBlock, NULL, NULL, NULL, NULL)) {
>         fprintf(stderr, "ERROR: could not determine if MAR is old or new.\n");
>-      } else if (oldMar) {
>+      } else if (!hasSignatureBlock) {
>         fprintf(stderr, "ERROR: The MAR file is in the old format so has"
>                         " no signature to verify.\n");
>       }
>       return -1;
>     }
> 
>     free(certBuffer);
>     return 0;

Comment 55

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

Comment on attachment 595247 [details] [diff] [review]
Build config for new MAR channel value. Patch v4.

>diff --git a/build/Makefile.in b/build/Makefile.in
>--- a/build/Makefile.in
>+++ b/build/Makefile.in
>@@ -61,17 +61,17 @@ TEST_DIRS += mobile/sutagent/android \
>           mobile/sutagent/android/watcher \
>           mobile/sutagent/android/ffxcp \
>           mobile/sutagent/android/fencp \
>           mobile/robocop \
>           $(NULL)
> endif
> 
> ifdef MOZ_APP_BASENAME
>-DIST_FILES = application.ini
>+DIST_FILES = application.ini update-settings.ini
Not sure if this is the case but this file shouldn't be created for Android.

>diff --git a/build/update-settings.ini.in b/build/update-settings.ini.in
>new file mode 100644
>--- /dev/null
>+++ b/build/update-settings.ini.in
>...
>+#filter substitution
>+[Channel]
>+MAR_CHANNEL=@MAR_CHANNEL_ID@
Why not keep the version in this file as well?

Holding off on r+'ing until the above is answered?

Comment 56

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

After thinking about it a bit more I guess we can't add version to the update-settings.ini yet and it might not be worth it. I would like to move the locale into this file and perhaps the channel as well.

>diff --git a/build/update-settings.ini.in b/build/update-settings.ini.in
>new file mode 100644
>--- /dev/null
>+++ b/build/update-settings.ini.in
>+#filter substitution
>+[Channel]
>+MAR_CHANNEL=@MAR_CHANNEL_ID@
How about:
[MARSettings] or similar
MAR_CHANNEL_ID=@MAR_CHANNEL_ID@
or 
CHANNEL_ID=@MAR_CHANNEL_ID@

Or just
[Settings]
MAR_CHANNEL_ID=@MAR_CHANNEL_ID@

Then it will be simple to add locale and channel name to the file

Comment 57

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

Comment on attachment 595031 [details] [diff] [review]
Updater code. Patch v4.

>diff --git a/toolkit/mozapps/update/updater/Makefile.in b/toolkit/mozapps/update/updater/Makefile.in
>--- a/toolkit/mozapps/update/updater/Makefile.in
>+++ b/toolkit/mozapps/update/updater/Makefile.in
>@@ -107,17 +107,20 @@ ifdef MOZ_DEBUG
> MOZ_WINCONSOLE = 1
> else
> MOZ_WINCONSOLE = 0
> endif
> endif
> 
> include $(topsrcdir)/config/rules.mk
> 
>-DEFINES += -DNS_NO_XPCOM
>+DEFINES += -DNS_NO_XPCOM \
>+  -DMAR_CHANNEL_ID='"$(MAR_CHANNEL_ID)"' \
>+  -DMOZ_APP_VERSION='"$(MOZ_APP_VERSION)"' \
Why the unusual quoting?

Used 1 time in the tree
-DMOZ_APP_VERSION="$(MOZ_APP_VERSION)"

Used 9 times in the tree
-DMOZ_APP_VERSION=$(MOZ_APP_VERSION

>diff --git a/toolkit/mozapps/update/updater/archivereader.cpp b/toolkit/mozapps/update/updater/archivereader.cpp
>--- a/toolkit/mozapps/update/updater/archivereader.cpp
>+++ b/toolkit/mozapps/update/updater/archivereader.cpp
>@@ -129,27 +135,95 @@ VerifyLoadedCert(MarFile *archive, int n

>+/**
>+ * Verifies that the MAR file matches the current product, channel, and version
>+ * 
>+ * @param MARChannelName The MAR channel name to use, only updates from MARs
>+ *                       with a matching MAR channel name will succeed.
>+ *                       If an empty string is passed, no check will be done
>+ *                       for the channel name in the product information block.
>+ * @param appVersion     The application version to use, only MARs with an
>+ *                       application version >= to appVersion will be applied.
>+ * @return OK on success
>+ *         COULD_NOT_READ_PRODUCT_INFO_BLOCK if the product info block 
>+ *                                           could not be read.
>+ *         MARCHANNEL_MISMATCH_ERROR         if the channel name for this 
>+ *                                           updater doesn't match the MAR
>+ *                                           file's channel name.
as provided by update-settings.ini

>+ *         VERSION_DOWNGRADE_ERROR           if the application version for
>+ *                                           this updater is newer than the
>+ *                                           one in the MAR.
>+ */
>+int
>+ArchiveReader::VerifyProductInformation(const char *MARChannelName, 
>+                                        const char *appVersion)
>+{
>+  if (!mArchive) {
>+    return ARCHIVE_NOT_OPEN;
>+  }
>+
>+  ProductInformationBlock productInfoBlock;
>+  int rv = mar_read_product_info_block(mArchive, 
>+                                       &productInfoBlock);
>+  if (rv != OK) {
>+    return COULD_NOT_READ_PRODUCT_INFO_BLOCK_ERROR;
>+  }
>+
>+  // Only check the MAR channel name if specified, it should be passed in from
>+  // the update-settings.ini file.
>+  if (MARChannelName && strlen(MARChannelName)) {
>+    if (OK == rv && strcmp(MARChannelName, productInfoBlock.MARChannelName)) {
>+      rv = MAR_CHANNEL_MISMATCH_ERROR;
>+    }
>+  }
>+
>+  if (OK == rv) {
nit: consistency... elsewhere you have >+  if (rv == OK) {

>diff --git a/toolkit/mozapps/update/updater/archivereader.h b/toolkit/mozapps/update/updater/archivereader.h
>--- a/toolkit/mozapps/update/updater/archivereader.h
>+++ b/toolkit/mozapps/update/updater/archivereader.h
>@@ -52,16 +52,18 @@
> class ArchiveReader
> {
> public:
>   ArchiveReader() : mArchive(NULL) {}
>   ~ArchiveReader() { Close(); }
> 
>   int Open(const NS_tchar *path);
>   int VerifySignature();
>+  int VerifyProductInformation(const char *MARChannelName, 
MARChannelID here and elsewhere

>@@ -186,16 +187,25 @@ public:
>   FILE* get() {
>     return mFile;
>   }
> 
> private:
>   FILE* mFile;
> };
> 
>+struct MARChannelStringTable {
>+  MARChannelStringTable() 
>+  {
>+    MARChannel[0] = '\0';
MARChannelID here and elsewhere

>+  }
>+
>+  char MARChannel[MAX_TEXT_LEN];
>+};
>+
> //-----------------------------------------------------------------------------
> 
> typedef void (* ThreadFunc)(void *param);
> 
> #ifdef XP_WIN
> #include <process.h>
> 
> class Thread
>@@ -1511,16 +1521,39 @@ IsUpdateStatusSucceeded(bool &isSucceede
>   fread(buf, sizeof(buf), 1, file);
> 
>   const char kSucceeded[] = "succeeded";
>   isSucceeded = strncmp(buf, kSucceeded, 
>                         sizeof(kSucceeded) - 1) == 0;
>   return true;
> }
> 
>+/**
>+ * This function reads in the MAR_CHANNEL_ID from update-settings.ini
>+ *
>+ * @param path    The path to the ini file that is to be read
>+ * @param results A pointer to the location to store the read strings
>+ * @return OK on success
>+ */
>+static int
>+ReadMARChannelStrings(const NS_tchar *path, MARChannelStringTable *results)
ReadMARChannelID or ReadMARChannelIDs if we are going to support more than one channel ID.

>@@ -1542,16 +1575,32 @@ UpdateThreadFunc(void *param)
> 
>   #ifdef MOZ_VERIFY_MAR_SIGNATURE
>   if (rv == OK) {
>     rv = gArchiveReader.VerifySignature();
>   }
>   #endif
> 
>   if (rv == OK) {
>+    NS_tchar MARChannelPath[MAX_TEXT_LEN];
nit: updateSettingsPath

I didn't call out directly all of the places where MARChannelName should be fixed.

Comment 58

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

Comment on attachment 591810 [details] [diff] [review]
XPCShell tests working with signed and product info block MARs. Patch v1.

>diff --git a/toolkit/mozapps/update/updater/updater.rc b/toolkit/mozapps/update/updater/updater.rc
>--- a/toolkit/mozapps/update/updater/updater.rc
>+++ b/toolkit/mozapps/update/updater/updater.rc
>@@ -38,16 +38,17 @@ IDI_DIALOG ICON "updater.ico"
> 
> /////////////////////////////////////////////////////////////////////////////
> //
> // Embedded certificates for allowed MARs
> //
> 
> IDR_PRIMARY_CERT TYPE_CERT "primaryCert.der"
> IDR_BACKUP_CERT TYPE_CERT  "backupCert.der"
>+IDR_XPCSHELL_CERT TYPE_CERT "xpcshellCert.der"
I haven't spent much time thinking about the security implications of adding this. Please run it by imelven

Comment 59

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

Comment on attachment 591810 [details] [diff] [review]
XPCShell tests working with signed and product info block MARs. Patch v1.

>diff --git a/toolkit/mozapps/update/test/unit/head_update.js.in b/toolkit/mozapps/update/test/unit/head_update.js.in
>--- a/toolkit/mozapps/update/test/unit/head_update.js.in
>+++ b/toolkit/mozapps/update/test/unit/head_update.js.in
>@@ -85,16 +85,18 @@ const URL_PATH = "data";
> 
> const APPLY_TO_DIR_SUFFIX = "_applyToDir/";
> const HELPER_BIN_FILE = "TestAUSHelper" + BIN_SUFFIX;
> const MAR_COMPLETE_FILE = "data/complete.mar";
> const MAR_PARTIAL_FILE = "data/partial.mar";
> const UPDATER_BIN_FILE = "updater" + BIN_SUFFIX;
> const MAINTENANCE_SERVICE_BIN_FILE = "maintenanceservice.exe";
> const MAINTENANCE_SERVICE_INSTALLER_BIN_FILE = "maintenanceservice_installer.exe";
>+const MAR_INI_FILE = "mar-channel.ini";
update-settings.ini

>+const MAR_INI_CONTENTS = "[Channel]\nMAR_CHANNEL=xpcshell-test\n"
etc.

>@@ -438,16 +440,20 @@ function runUpdate() {
>   let cwdPath = callbackApp.parent.path;
>   if (/ /.test(cwdPath))
>     cwdPath = '"' + cwdPath + '"';
> 
>   let callbackAppPath = callbackApp.path;
>   if (/ /.test(callbackAppPath))
>     callbackAppPath = '"' + callbackAppPath + '"';
> 
>+  let MARINIFile = getApplyDirFile(null, true);
nit: updateSettingsIni

>+  MARINIFile.append(MAR_INI_FILE);
>+  writeFile(MARINIFile, MAR_INI_CONTENTS);
>+
>   let args = [updatesDirPath, applyToDirPath, 0, cwdPath, callbackAppPath].
>              concat(gCallbackArgs);
>   let process = AUS_Cc["@mozilla.org/process/util;1"].
>                 createInstance(AUS_Ci.nsIProcess);
>   process.init(updateBin);
>   process.run(true, args, args.length);
>   return process.exitValue;
> }
>@@ -744,16 +750,20 @@ function runUpdateUsingService(aInitialS
> 
>   // The service will execute maintenanceservice_installer.exe and
>   // will copy maintenanceservice.exe out of the same directory from
>   // the installation directory.  So we need to make sure both of those
>   // bins always exist in the installation directory.
>   copyBinToApplyToDir(MAINTENANCE_SERVICE_BIN_FILE);
>   copyBinToApplyToDir(MAINTENANCE_SERVICE_INSTALLER_BIN_FILE);
> 
>+  let MARINIFile = getApplyDirFile(null, true);
nit: updateSettingsIni

>+  MARINIFile.append(MAR_INI_FILE);
>+  writeFile(MARINIFile, MAR_INI_CONTENTS);
>+
>   // Firefox does not wait for the service command to finish, but
>   // we still launch the process sync to avoid intermittent failures with
>   // the log file not being written out yet.
>   // We will rely on watching the update.status file and waiting for the service
>   // to stop to know the service command is done.
>   process.run(true, args, args.length);
> 
>   resetEnvironment();
>diff --git a/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js b/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js
>--- a/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js
>+++ b/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js
>@@ -99,16 +99,20 @@ function run_test() {
>   let updaterIniContents = "[Strings]\n" +
>                            "Title=Update Test\n" +
>                            "Info=Application Update XPCShell Test - " +
>                            "test_0200_general.js\n";
>   updaterIni = processDir.clone();
>   updaterIni.append(FILE_UPDATER_INI);
>   writeFile(updaterIni, updaterIniContents);
> 
>+  let MARINIFile = processDir.clone();
nit: updateSettingsIni

Comment 60

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

Comment on attachment 591810 [details] [diff] [review]
XPCShell tests working with signed and product info block MARs. Patch v1.

>diff --git a/toolkit/mozapps/update/common/updatehelper.cpp b/toolkit/mozapps/update/common/updatehelper.cpp
>--- a/toolkit/mozapps/update/common/updatehelper.cpp
>+++ b/toolkit/mozapps/update/common/updatehelper.cpp
>@@ -34,16 +34,17 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include <windows.h>
> #include <stdio.h>
> #include "shlobj.h"
> #include "updatehelper.h"
>+#include "pathhash.h"
> 
> // Needed for PathAppendW
> #include <shlwapi.h>
> // Needed for CreateToolhelp32Snapshot
> #include <tlhelp32.h>
> #pragma comment(lib, "shlwapi.lib") 
> 
> WCHAR* MakeCommandLine(int argc, WCHAR **argv);
>@@ -341,17 +342,17 @@ LaunchServiceSoftwareUpdateCommand(DWORD
> {
>   // The service command is the same as the updater.exe command line except 
>   // it has 2 extra args: 1) The Path to udpater.exe, and 2) the command 
>   // being executed which is "software-update"
>   LPCWSTR *updaterServiceArgv = new LPCWSTR[argc + 2];
>   updaterServiceArgv[0] = L"MozillaMaintenance";
>   updaterServiceArgv[1] = L"software-update";
> 
>-  for (int i = 0; i < argc; ++i) {
>+  for (DWORD i = 0; i < argc; ++i) {
Why are you changing this to a DWORD?

Comment 61

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

Comment on attachment 591811 [details] [diff] [review]
This patch simply contains the new MAR files. Patch v1.

Nothing to review. Just r=rs when the other patches are r+'d

Comment 62

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

Comment on attachment 595247 [details] [diff] [review]
Build config for new MAR channel value. Patch v4.

>diff --git a/build/Makefile.in b/build/Makefile.in
>--- a/build/Makefile.in
>+++ b/build/Makefile.in
>@@ -61,17 +61,17 @@ TEST_DIRS += mobile/sutagent/android \
>           mobile/sutagent/android/watcher \
>           mobile/sutagent/android/ffxcp \
>           mobile/sutagent/android/fencp \
>           mobile/robocop \
>           $(NULL)
> endif
> 
> ifdef MOZ_APP_BASENAME
>-DIST_FILES = application.ini
>+DIST_FILES = application.ini update-settings.ini
Not sure if this is the case but this file shouldn't be created for Android.

r=me if you verify that the file isn't created for Android, etc.

Comment 63

[Brian R. Bondy [:bbondy]]

>+  char *productVersion = MOZ_APP_VERSION;
> So, this will be build specific? Could it just use update-settings.ini when 
> creating the mar to determine these values instead?

This is just the default value if no value is specifically specified.  I'd prefer not to have a dependency on update-settings.ini for the mar and signmar program because signmar is copied out by releng.

Comment 64

[Brian R. Bondy [:bbondy]]

Thanks for the reviews!
Uploading patches shortly.

> >+  case 'c': {
> >+    struct ProductInformationBlock infoBlock;
> >+    infoBlock.MARChannelName = MARChannelName;
> >+    infoBlock.productVersion = productVersion;
> >+    return mar_create(argv[2], argc - 3, argv + 3, &infoBlock);

> Have you verified this doesn't break other apps besides Firefox 
> (I saw that it returns 0 when no info is provided... I just want to make sure)?

I think only the mar program uses mar_create to creates mars. In the mar program's
source code we always pass a product info block so this should be OK.


> >+#filter substitution
> >+[Channel]
> >+MAR_CHANNEL=@MAR_CHANNEL_ID@
> Why not keep the version in this file as well?

There is no need for this from releng or QA and it adds more code.
Also then we'd have to update the file on each update.

> After thinking about it a bit more I guess we can't add version to the 
> update-settings.ini yet and it might not be worth it. 
> I would like to move the locale into this file and perhaps the channel as well.

In a follow up bug right?


> >+  -DMAR_CHANNEL_ID='"$(MAR_CHANNEL_ID)"' \
> >+  -DMOZ_APP_VERSION='"$(MOZ_APP_VERSION)"' \
> Why the unusual quoting?
> 
> Used 1 time in the tree
> -DMOZ_APP_VERSION="$(MOZ_APP_VERSION)"
> 
> Used 9 times in the tree
> -DMOZ_APP_VERSION=$(MOZ_APP_VERSION

I think it is needed to stringify the value via the Makefile.
I tried without '" and also with just " and both ways it does not compile. 
I think I can do the stringify via the actual c++ code but I think Makefile should be fine.


> >-  for (int i = 0; i < argc; ++i) {
> >+  for (DWORD i = 0; i < argc; ++i) {
> Why are you changing this to a DWORD?

Just to get rid of a signed/unsigned mismatch warning.

Comment 65

[Brian R. Bondy [:bbondy]]

Attachment: Build config for new MAR channel value. Patch v5 [Checked in: Comment 82]

- Changed MAR_CHANNEL TO MAR_CHANNEL_ID in the ini file
- Only have update-settings.ini if not android now
Carrying forward r+

Comment 66

[Brian R. Bondy [:bbondy]]

Attachment: XPCShell tests working with signed and product info block MARs. Patch v2.

Implemented review comments.

Comment 67

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v8.

Implemented review comments.

Comment 68

[Brian R. Bondy [:bbondy]]

Attachment: Updater code. Patch v5 [Checked in: Comment 82]

Implemented review comments.

Comment 69

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

(In reply to Brian R. Bondy [:bbondy] from comment #64)
>...
> > After thinking about it a bit more I guess we can't add version to the 
> > update-settings.ini yet and it might not be worth it. 
> > I would like to move the locale into this file and perhaps the channel as well.
> 
> In a follow up bug right?
Yes and sorry I didn't say that in the original comment.

Comment 70

[Ian Melven :imelven]

(In reply to Robert Strong [:rstrong] (do not email) from comment #58)
> Comment on attachment 591810 [details] [diff] [review]
> XPCShell tests working with signed and product info block MARs. Patch v1.
> 
> >diff --git a/toolkit/mozapps/update/updater/updater.rc b/toolkit/mozapps/update/updater/updater.rc
> >--- a/toolkit/mozapps/update/updater/updater.rc
> >+++ b/toolkit/mozapps/update/updater/updater.rc
> >@@ -38,16 +38,17 @@ IDI_DIALOG ICON "updater.ico"
> > 
> > /////////////////////////////////////////////////////////////////////////////
> > //
> > // Embedded certificates for allowed MARs
> > //
> > 
> > IDR_PRIMARY_CERT TYPE_CERT "primaryCert.der"
> > IDR_BACKUP_CERT TYPE_CERT  "backupCert.der"
> >+IDR_XPCSHELL_CERT TYPE_CERT "xpcshellCert.der"
> I haven't spent much time thinking about the security implications of adding
> this. Please run it by imelven

bbondy, can you clarify what this cert is used for ? i assume it's for the tests based on the name - more details appreciated :)

Comment 71

[Brian R. Bondy [:bbondy]]

> bbondy, can you clarify what this cert is used for ? 
> i assume it's for the tests based on the name - more details appreciated :)

Right so basically we use that cert to verify the signed MAR if the fallback key exists and we're doing an xpcshell test. So like some other previous tasks, we depend on "does the fallback key exist" to determine if we should use the xpcshell cert or not.  If someone doesn't have the fallback key in HKLM they can't use the xpcshell cert.

Comment 72

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

Comment on attachment 595780 [details] [diff] [review]
XPCShell tests working with signed and product info block MARs. Patch v2.

># HG changeset patch
># Parent 4fbd9a8bc334a4fe2bac3f003eb902b74bc2671d
># User Brian R. Bondy <[e-mail]>
>Bug 708690 - XPCShell support fox signed / product /version checked MARs
>
>diff --git a/toolkit/mozapps/update/common/updatehelper.cpp b/toolkit/mozapps/update/common/updatehelper.cpp
>--- a/toolkit/mozapps/update/common/updatehelper.cpp
>+++ b/toolkit/mozapps/update/common/updatehelper.cpp
>@@ -341,17 +342,17 @@ LaunchServiceSoftwareUpdateCommand(DWORD
> {
>   // The service command is the same as the updater.exe command line except 
>   // it has 2 extra args: 1) The Path to udpater.exe, and 2) the command 
>   // being executed which is "software-update"
>   LPCWSTR *updaterServiceArgv = new LPCWSTR[argc + 2];
>   updaterServiceArgv[0] = L"MozillaMaintenance";
>   updaterServiceArgv[1] = L"software-update";
> 
>-  for (int i = 0; i < argc; ++i) {
>+  for (DWORD i = 0; i < argc; ++i) {
per discussion, just change the function param to an int instead.

r=rs with that

Comment 73

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #71)
> > bbondy, can you clarify what this cert is used for ? 
> > i assume it's for the tests based on the name - more details appreciated :)
> 
> Right so basically we use that cert to verify the signed MAR if the fallback
> key exists and we're doing an xpcshell test. So like some other previous
> tasks, we depend on "does the fallback key exist" to determine if we should
> use the xpcshell cert or not.  If someone doesn't have the fallback key in
> HKLM they can't use the xpcshell cert.

Got it. So it's along the same lines as the previous fallback discussions where an attacker would have to be able to create the HKLM key requiring admin/elevated privs. If they did so, they could have binaries be validated with the xpcshell cert, which they would already have had to compromise to sign something malicious with it. Sounds ok to me.

Comment 74

[Brian R. Bondy [:bbondy]]

Thanks for looking into it.  Yup it is logically equivalent to the previous discussions for the other tasks with fallback key checks.

Comment 75

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

Comment on attachment 595781 [details] [diff] [review]
libmar code. Patch v8.

>diff --git a/modules/libmar/src/mar.h b/modules/libmar/src/mar.h
>--- a/modules/libmar/src/mar.h
>+++ b/modules/libmar/src/mar.h
>@@ -92,79 +97,95 @@ MarFile *mar_wopen(const PRUnichar *path
>  * @param mar       The MarFile object to close.
>  */
> void mar_close(MarFile *mar);
> 
> /**
>  * Find an item in the MAR file by name.
>  * @param mar       The MarFile object to query.
>  * @param item      The name of the item to query.
>- * @returns         A const reference to a MAR item or NULL if not found.
>+ * @return          A const reference to a MAR item or NULL if not found.
>  */
> const MarItem *mar_find_item(MarFile *mar, const char *item);
> 
> /**
>  * Enumerate all MAR items via callback function.
>  * @param mar       The MAR file to enumerate.
>  * @param callback  The function to call for each MAR item.
>  * @param data      A caller specified value that is passed along to the
>  *                  callback function.
>- * @returns         Zero if the enumeration ran to completion.  Otherwise, any
>+ * @return          Zero if the enumeration ran to completion.  Otherwise, any
nit: for consistency with other comments s/Zero/0/

>... 
> /**
>  * Verifies the embedded signature for the specified mar file.
>  * We do not check that the certificate was issued by any trusted authority. 
>  * We assume it to be self-signed.  We do not check whether the certificate 
>  * is valid for this usage.
>  * 
>  * @param mar            The already opened MAR file.
>  * @param certData       The certificate file data.
>  * @param sizeOfCertData The size of the cert data.
>- * @return 0 on success
>+ * @retur  0 on success
@return

>diff --git a/modules/libmar/src/mar_create.c b/modules/libmar/src/mar_create.c
>--- a/modules/libmar/src/mar_create.c
>+++ b/modules/libmar/src/mar_create.c
>@@ -119,20 +121,216 @@ static int mar_concat_file(FILE *fp, con
>...
>+static int
>+mar_concat_product_info_block(FILE *fp, 
>+                              struct MarItemStack *stack,
>+                              struct ProductInformationBlock *infoBlock)
>+{

>...
>+  /* Write out the product version string and NULL terminator */
>+  if (fwrite(infoBlock->productVersion, 
>+      strlen(infoBlock->productVersion) + 1, 1, fp) != 1) {
>+    return -1;
>+  }
>+
>+  /* Write out the rest of the block that is unused */
>+  unused = infoBlockSize - (sizeof(infoBlockSize) +
>+                                   sizeof(additionalBlockID) +
>+                                   strlen(infoBlock->MARChannelID) + 
>+                                   strlen(infoBlock->productVersion) + 2);
nit: indentation

r=rs with that

Comment 76

[Brian R. Bondy [:bbondy]]

Attachment: libmar code. Patch v9 [Checked in: Comment 82]

Implemented nits, carrying forward r+.

Comment 77

[Brian R. Bondy [:bbondy]]

Attachment: XPCShell tests working with signed and product info block MARs. Patch v3.

Fixed nit: Changed LaunchServiceSoftwareUpdateCommand to take in int argc instead of DWORD.

Carrying forward r+.

Comment 78

[Brian R. Bondy [:bbondy]]

Attachment: XPCShell tests working with signed and product info block MARs. Patch v4.

Minor fix for browser chrome mochitest because it should have used the mar without the product info block. Carrying forward r+.  The extra change just changes the name of the MAR in the chrome mochitest makefile.

Comment 79

[Brian R. Bondy [:bbondy]]

Attachment: This patch simply contains the new MAR files. Patch v2.

Updated MAR files (signed with product info blocks).

Last time I updated them I failed to preserve the permissions because I re-generated them on Windows. This caused try results to fail on Linux/ OSX for the updater tests.  

Fixed with this v2 patch.   Carrying forward r+

Comment 80

[Brian R. Bondy [:bbondy]]

Attachment: This patch simply contains the new MAR files. Patch v3 [Checked in: Comment 82]

Had to re-sign the rest of the MAR files and update the xpcshell cert since I modified the MARs file contents permissions earlier today.

Comment 81

[Brian R. Bondy [:bbondy]]

Attachment: XPCShell tests working with signed and product info block MARs. Patch v5 [Checked in: Comment 82]

Comment 82

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/57ab36828a9e
http://hg.mozilla.org/mozilla-central/rev/4de2d63b21cf
http://hg.mozilla.org/mozilla-central/rev/c7f5af7dad8d
http://hg.mozilla.org/mozilla-central/rev/df3801e30225
http://hg.mozilla.org/mozilla-central/rev/52bd72d77fda

Comment 83

[Serge Gautherie (:sgautherie)]

[NB: I didn't read all comments nor all related bugs.]

(In reply to Brian R. Bondy [:bbondy] from comment #40)
> firefox-mozilla-central sounds good.
> I just used repo name since it should to be a unique value per repo.

How is this going to work for next cycles, when it merges in *-aurora/*-beta/*-release?
I would have thought about .mozconfig, but I think they are moving into main repo too.
That would leave (RelEng) build tree only...

Comment 84

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

It is set in confvars.sh which contains repo specific values already.

Comment 85

[Serge Gautherie (:sgautherie)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #84)
> It is set in confvars.sh which contains repo specific values already.

Sorry, I'm not familiar with this situation, as SeaMonkey has the same values on all 4 repos.
I see that Firefox has a different value for MOZ_BRANDING_DIRECTORY, for example.
How is confvars.sh maintained through cycle merges?

Comment 86

[Brian R. Bondy [:bbondy]]

It's my understanding that confvars.sh isn't merged at migration time.

Comment 87

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97

Comment 88

[u279076]

Is this something QA can test?

Comment 89

[Brian R. Bondy [:bbondy]]

Yes when you go to the about dialog it will download a mar file into your profile. 
It will be located here:
C:\Users\<usernamer>\AppData\Local\Mozilla\Firefox\Nightly\updates\0

While Nightly is still in the about dialog ready to apply an update do the same with aurora. Then copy the nightly signed mar to the aurora folder.  You should get an error 22 in the maintenance service log.

Comment 90

[Simona Badau, Desktop QA]

Followed the above steps to reproduce and the error 22 is get only when Aurora is updated having the nightly signed mar replaced in the aurora folder. 

When updating Firefox 12 beta 1 and Firefox 13 beta 1 having the same nightly mar into the beta's folder -> error 19 is get instead. 

Were the error messages changed between Firefox 12, 13 and Firefox 14,15? Is that the right behavior?

Comment 91

[Brian R. Bondy [:bbondy]]

Here are the error codes related to this:

> #define CERT_LOAD_ERROR 17
> #define CERT_HANDLING_ERROR 18
> #define CERT_VERIFY_ERROR 19
> #define ARCHIVE_NOT_OPEN 20
> #define COULD_NOT_READ_PRODUCT_INFO_BLOCK_ERROR 21
> #define MAR_CHANNEL_MISMATCH_ERROR 22
> #define VERSION_DOWNGRADE_ERROR 23

So you're seeing a cert error because the certs don't match up for different channels.  I think that is correct.

You can verify a downgrade maybe using an old MAR in the same manner but for the same channel.

Comment 92

[Simona Badau, Desktop QA]

When updating Firefox using an old MAR file from a previous version the update is done all the way and I don't get any errors at all.

I updated Firefox 13 beta 1 (having the MAR file from Firefox 12 beta 1) and Aurora 14a2 (having the Mar file from Firefox 12a2) and I did not get the error 23.

Is there another way to verify a downgrade?

Comment 93

[Brian R. Bondy [:bbondy]]

> I updated Firefox 13 beta 1 (having the MAR file from Firefox 12 beta 1) and ... and I did not get the error.

How did you obtain the MAR file for Firefox 12 beta 1?  Did you manually download the .mar file from the FTP?


If so could you link to the exact MAR you used? I'll check it's header info using the signmar tool.

Comment 94

[Simona Badau, Desktop QA]

(In reply to Brian R. Bondy [:bbondy] from comment #93)
> How did you obtain the MAR file for Firefox 12 beta 1?  Did you manually
> download the .mar file from the FTP?

Installed Firefox 12 beta 1, went to the about dialog and waited for the mar file to be downloaded into my profile.

Comment 95

[Brian R. Bondy [:bbondy]]

Ya that's why, updates always update to the latest build and not the next build. So you're actually getting the same MAR file as you would have gotten normally that way.  You'll have to manually download the MAR file from the FTP server for the build you want to test.

Comment 96

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0

Verified on Windows 7 and on Windows Vista x64 that when updating Firefox - Mar files protect against version downgrades - error 23 (#define VERSION_DOWNGRADE_ERROR) was obtained when updating Firefox 13 beta 4 having the 12 beta 1 mar file into the beta's folder.

Based on this and on Comment 91 - setting status-firefox 13 to verified.