The default bug view has changed. See this FAQ.

Beta builds should be able to apply release mars

RESOLVED FIXED in Firefox 12

Status

()

Toolkit
Application Update
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: catlee, Assigned: bbondy)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10 unaffected, firefox11 unaffected, firefox12 fixed, firefox13 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:nse][qa-])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
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

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

Comment 3

5 years ago
(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

5 years ago
after discussing with catlee, this seems ok to me. dveditz also is on board.
(Assignee)

Comment 5

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

Updated

5 years ago
Assignee: nobody → netzen
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.
Whiteboard: [sg:nse]
(Assignee)

Comment 7

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

Comment 8

5 years ago
Also wanted to mention that in the patch, the actual value of MOZ_ACCEPTED_MAR_CHANNELS in confvars.sh will be normalized before landing.
(Assignee)

Comment 9

5 years ago
Created attachment 594009 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v2.

Rebased after moving code out of readstrings from another patch.
Attachment #593884 - Attachment is obsolete: true
Attachment #593884 - Flags: review?(robert.bugzilla)
Attachment #594009 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 10

5 years ago
Created attachment 595032 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v3.

Rebased for new mar define name.
Attachment #594009 - Attachment is obsolete: true
Attachment #594009 - Flags: review?(robert.bugzilla)
Attachment #595032 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 11

5 years ago
Created attachment 595033 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v4.

Missed renaming the accepted MAR ID to have "firefox-" prefix
Attachment #595032 - Attachment is obsolete: true
Attachment #595032 - Flags: review?(robert.bugzilla)
Attachment #595033 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 12

5 years ago
Created attachment 595264 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v5.

Rebased for ini change
Attachment #595033 - Attachment is obsolete: true
Attachment #595033 - Flags: review?(robert.bugzilla)
Attachment #595264 - Flags: review?(robert.bugzilla)
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.
Attachment #595264 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 595719 [details] [diff] [review]
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
Attachment #595264 - Attachment is obsolete: true
Attachment #595719 - Flags: review?(robert.bugzilla)
Attachment #595719 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Updated

5 years ago
Depends on: 728935
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Assignee)

Comment 15

5 years ago
http://hg.mozilla.org/mozilla-central/rev/d74612c58245
Blocks: 730663
(Assignee)

Comment 16

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
status-firefox11: --- → unaffected
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Target Milestone: mozilla13 → mozilla12
status-firefox-esr10: --- → unaffected
status-firefox10: --- → unaffected
Is this something QA can test?
Whiteboard: [sg:nse] → [sg:nse][qa?]
Group: core-security
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> Is this something QA can test?

Taking silence as a 'no'.
Whiteboard: [sg:nse][qa?] → [sg:nse][qa-]
You need to log in before you can comment on or make changes to this bug.