721758 - Beta builds should be able to apply release mars

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Chris AtLee [:catlee]]

In bug 708697 we're adding a channel value to signed mars, and restricting the updater service and updater to apply only mars that match its channel. This works for most use cases, but one place it fails is being able to serve beta users our final release mars. Up until recently this was the regular process for releases. e.g. the 8.0 beta users would receive 8.0 final ('release' builds) first, and then be moved onto 9.0b1 when that was ready. This scenario is no longer possible with signed mars as specified in bug 708697.

I propose we allow the updater to be configured with a list of allowable channels. beta builds would be configured to be allowed to update to 'beta,release', and release builds only 'release'

Comment 1

[Brian R. Bondy [:bbondy]]

bsmith and imelven, before I begin work on this, please confirm that you are OK with a beta build being able to consume release MARs.  This task is to avoid having to do the work of repackaging MAR files for a common use case.

Comment 2

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> bsmith and imelven, before I begin work on this, please confirm that you are
> OK with a beta build being able to consume release MARs.  This task is to
> avoid having to do the work of repackaging MAR files for a common use case.

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=708697#c39

Can we mark (in the example in comment #0 above) the 8.0 final build as still being on 'beta' channel in the MAR ? Then the user is always on the 'beta' channel (as they are in fact) even though they have applied the same MAR that was sent to the release channel as well. From the changes bbondy has made it seems like with the mar tool, the signature can be stripped, the product info block in the MAR updated to 'beta' (or 'release' depending on which was built first) and then re-signed - this can all be automated using the tool.

Comment 3

[Chris AtLee [:catlee]]

(In reply to Ian Melven :imelven from comment #2)
> (In reply to Brian R. Bondy [:bbondy] from comment #1)
> > bsmith and imelven, before I begin work on this, please confirm that you are
> > OK with a beta build being able to consume release MARs.  This task is to
> > avoid having to do the work of repackaging MAR files for a common use case.
> 
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=708697#c39
> 
> Can we mark (in the example in comment #0 above) the 8.0 final build as
> still being on 'beta' channel in the MAR ? Then the user is always on the
> 'beta' channel (as they are in fact) even though they have applied the same
> MAR that was sent to the release channel as well. From the changes bbondy
> has made it seems like with the mar tool, the signature can be stripped, the
> product info block in the MAR updated to 'beta' (or 'release' depending on
> which was built first) and then re-signed - this can all be automated using
> the tool.

RelEng's concern is that automating this is non-trivial for the several hundred mar files that are involved in a release.

Comment 4

[Ian Melven :imelven]

after discussing with catlee, this seems ok to me. dveditz also is on board.

Comment 5

[Brian R. Bondy [:bbondy]]

As per catlee's suggestion I'll implement this via 2 settings in the confvars.sh.  One will be the default to use for MAR creation.  The other will be a comma separated list of acceptable channel names that the updater can consume.

By the way. this implies that MAR channel names cannot contain ",".  Such strings will be interpreted as multiple different MAR channel values.  

The mar-channel.ini file (which will be renamed to update-settings.ini) will only list the comma separated list define.  There is no need for it to have the value for the MAR default since that value is only used by the mar command line utility.

All configs will have both values in the .ini file the same except for the beta channel which will have 2 values in the comma separated list.

Comment 6

[Daniel Veditz [:dveditz]]

Rating this "not a security exploit" since it's just an improvement to bug 708697 and not a vulnerability in itself. Still hidden while that bug is hidden.

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patc h v1. Add ability to configure updater to accept multiple channels

Note for Rob: 
This is probably the lowest priority of the remaining silent update reviews.

Comment 8

[Brian R. Bondy [:bbondy]]

Also wanted to mention that in the patch, the actual value of MOZ_ACCEPTED_MAR_CHANNELS in confvars.sh will be normalized before landing.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Add ability to configure updater to accept multiple channels. Patch v2.

Rebased after moving code out of readstrings from another patch.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Add ability to configure updater to accept multiple channels. Patch v3.

Rebased for new mar define name.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Add ability to configure updater to accept multiple channels. Patch v4.

Missed renaming the accepted MAR ID to have "firefox-" prefix

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Add ability to configure updater to accept multiple channels. Patch v5.

Rebased for ini change

Comment 13

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

Comment on attachment 595264 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v5.

>diff --git a/build/update-settings.ini.in b/build/update-settings.ini.in
>--- a/build/update-settings.ini.in
>+++ b/build/update-settings.ini.in
>@@ -36,9 +36,9 @@
> ;
> ; ***** END LICENSE BLOCK *****
> #endif
> ; If you modify this file updates may fail.
> ; Do not modify this file.
> 
> #filter substitution
> [Channel]
>-MAR_CHANNEL=@MAR_CHANNEL_ID@
>+ACCEPTED_MAR_CHANNELS=@MOZ_ACCEPTED_MAR_CHANNELS@
ACCEPTED_MAR_CHANNEL_IDS

>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
>@@ -164,16 +164,18 @@ ArchiveReader::VerifySignature()
> 
> /**
>  * 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.
>+ *                       If a comma separated list of values is passed then
>+ *                       only one value must match.
s/only //

>  * @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.
>@@ -194,18 +196,30 @@ ArchiveReader::VerifyProductInformation(
>                                        &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;
>+    // Check for at least one match in the comma separated list of values.
>+    const char *delimiter = " ,\t\r\n";
It doesn't seem like this should delimit on space, tab, return, and newline

Should be a quick second pass.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: dd ability to configure updater to accept multiple channels. Patch v6.

> It doesn't seem like this should delimit on space, tab, return, and newline

Having the whitespace there has the nice side effect that strtok will automatically trim your tokens of whitespace.  I removed the \r and \n and I added a a couple comments into confvars.sh:

> + # This should usually be the same as the value MAR_CHANNEL_ID.
> + # If more than one ID is needed, then you should use a comma separated list
> + # of values.
>   ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-central
> + # The MAR_CHANNEL_ID must not contain the following 3 characters: ",\t "  
>   MAR_CHANNEL_ID=firefox-mozilla-central

Comment 15

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/d74612c58245

Comment 16

[Brian R. Bondy [:bbondy]]

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

Comment 17

[u279076]

Is this something QA can test?

Comment 18

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> Is this something QA can test?

Taking silence as a 'no'.