1220559 - RC builds on beta channel warn "Firefox may damage your computer" on OS X El Capitan

Status: NEW

(Firefox :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

sheppy reported elsewhere that for a machine with
 * Firefox on the beta channel
 * Mac OS X 10.11.1
 * account with parental controls (presumably non-admin)
a message ""Firefox may damage your computer" is shown on startup. Definitely not expected behaviour.

This may be a signing bug, so I'm filing it here.

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

I don't see this locally, which is the same setup except for the limited account, so hopefully that limits the scope a little.

It would be really good if we could test a fresh install of the release build on the broken machine, that's at 
    http://archive.mozilla.org/pub/firefox/candidates/42.0-candidates/build2/mac/en-US/Firefox%2042.0.dmg

sheppy, could you test ?

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Moving to Firefox:General to get access to tracking flags, and get it on RelMan's radar.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Here are some preliminary ideas about what's going on. The way to test signing is open a terminal and run
   codesign -vvv /Applications/FirefoxBeta.app; echo $?

For 42.0b7 and b8 that exits with code 0, so it's fine there. If I take one of those betas and update it 42.0 RC2 (which we offer predominantly to test crashiness on windows, but update all platforms) then the codesign output ends with

 FirefoxBeta.app/: a sealed resource is missing or invalid
 file modified: /Applications/FirefoxBeta.app/Contents/Resources/defaults/pref/channel-prefs.js
 file modified: /Applications/FirefoxBeta.app/Contents/Resources/update-settings.ini
 1

channel-prefs.js contains the usual beta content of:
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

pref("app.update.channel", "beta");
------

Likewise update-settings.ini has:
; If you modify this file updates may fail.
; Do not modify this file.

[Settings]
ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-beta,firefox-mozilla-release
------

Modifying those to release values ('release' channel, and 'ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release') results in the signing check passing.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

I had thought we were excluding all of Resources, but that's not right according to 
  http://hg.mozilla.org/build/tools/file/e02a5001edb2/lib/python/signing/utils.py#l285

We upgraded a couple of signing servers from 10.9.5 to 10.10.5 in bug 1203332. Not sure yet if codesign comes with the OS or with XCode, but in puppet it looks like we have XCode 5.0-commandline for 10.9, and 6.1-cmdline for 10.10.

We used one of the new machines for 42.0 RC2 (aka build2). But we used an old one for 41.0, and you can make that fail codesign by modifying one of the files above. Doesn't mean OS X will refuse to launch it though, but this error could have shown up last release cycle too. Maybe El Capitan is enforcing Gatekeeper differently ?

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Now that we've shipped 43.0b1 it would also be interesting to update the beta you have (really 42.0 RC2) to that and see if the dire warning goes away. I expect it probably will.

Comment 6

[Eric Shepherd [:sheppy]]

I'm unable to reproduce at the moment, although I had been reproducing it easily at one point. I'm a little confused and continue to try to figure this out.

Comment 7

[Eric Shepherd [:sheppy]]

And just now, my daughter exclaimed loudly that I hadn't fixed this on her iMac yet, so I guess I remember where it was happening. She's on a 42 beta from October 28th at the moment. Trying to download 42 release but it's estimating about 3.5 hours to complete for some reason.

It's worth pointing out that she's on a non-admin account.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Setting the tracking flag that I believe Nick intended to set.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

[Tracking Requested - why for this release]:
See comment #4

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 44. This is a bad one, we should try to fix it asap. Florin, would you be able to confirm if this is indeed happening on 43 RC builds (if those are still available).

Comment 11

[Bogdan Maris, Desktop QA]

(In reply to Ritu Kothari (:ritu) from comment #10)
> Tracked for 44. This is a bad one, we should try to fix it asap. Florin,
> would you be able to confirm if this is indeed happening on 43 RC builds (if
> those are still available).

I tested on two different iMac machines with El Capitan (10.11.1 and 10.11 Beta (15A279b)), and I was NOT able to reproduce using Firefox 42 beta 7. 
Running 'codesign -vvv /Applications/Firefox.app; echo $?' for 42 beta 7 the output ends with code 0, after editing beta channel to 'release-localtest' (since 'release' was closed) in 'channel-prefs.js' and deleting 'firefox-mozilla-beta,' from 'update-settings.ini' the final code was 1.
I then installed Firefox 43.0.3 and ran the signing command and the final code was 1 as well, without editing a thing.

Is this the right behavior here?

So bottom line is that I did not received any 'Firefox may damage your computer' notification. If there is anything I can help with here please needinfo me.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Chris: Florin from SV was unable to repro this. Is this still a concern? If not, I'd like to resolve this as wfm/invalid. If it is a real issue, Beta44 is a week away from entering RC and we should try to get a fix ready. Thanks!

Comment 13

[Chris AtLee [:catlee]]

This still worries me, but given that we can't reproduce, and there aren't other reports of this, I think we could go ahead. This isn't unique to 44.

:sheppy, can you reproduce still?

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #11)
> (In reply to Ritu Kothari (:ritu) from comment #10)
> > Tracked for 44. This is a bad one, we should try to fix it asap. Florin,
> > would you be able to confirm if this is indeed happening on 43 RC builds (if
> > those are still available).
> 
> I tested on two different iMac machines with El Capitan (10.11.1 and 10.11
> Beta (15A279b)), and I was NOT able to reproduce using Firefox 42 beta 7. 
> Running 'codesign -vvv /Applications/Firefox.app; echo $?' for 42 beta 7 the
> output ends with code 0, after editing beta channel to 'release-localtest'
> (since 'release' was closed) in 'channel-prefs.js' and deleting
> 'firefox-mozilla-beta,' from 'update-settings.ini' the final code was 1.
> I then installed Firefox 43.0.3 and ran the signing command and the final
> code was 1 as well, without editing a thing.
> 
> Is this the right behavior here?

It sounds like this confirms the codesign tests that Nick did in comment #3 - modifying update-settings.ini or channel-prefs.js breaks the signature.

> So bottom line is that I did not received any 'Firefox may damage your
> computer' notification. If there is anything I can help with here please
> needinfo me.

AFAICT, we've only seen this dialog on Sheppy's machine when running Firefox as a Limited User w/ parental controls enabled. I'm not sure if that's coincidence, or if that dialog truly only is shown for accounts like that.

Either way, we need to make sure we're not invalidating signatures when we ship an RC to Beta. I'm hoping this will get rid of the "Firefox may damage your computer" message too.

Stephen, any thoughts on what the right thing to do here is? The tl;dr is that we have a valid use case for modifying update-settings.ini and channel-prefs.js when we ship RCs to the beta channel. These files are not updated as part of the MAR (to avoid changing the user's channel), but whatever file contains their checksums is. Can we exclude them from the signature somehow?

Comment 15

[Stephen A Pohl [:spohl]]

(In reply to Ben Hearsum (:bhearsum) from comment #14)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #11)
> > (In reply to Ritu Kothari (:ritu) from comment #10)
> > > Tracked for 44. This is a bad one, we should try to fix it asap. Florin,
> > > would you be able to confirm if this is indeed happening on 43 RC builds (if
> > > those are still available).
> > 
> > I tested on two different iMac machines with El Capitan (10.11.1 and 10.11
> > Beta (15A279b)), and I was NOT able to reproduce using Firefox 42 beta 7. 
> > Running 'codesign -vvv /Applications/Firefox.app; echo $?' for 42 beta 7 the
> > output ends with code 0, after editing beta channel to 'release-localtest'
> > (since 'release' was closed) in 'channel-prefs.js' and deleting
> > 'firefox-mozilla-beta,' from 'update-settings.ini' the final code was 1.
> > I then installed Firefox 43.0.3 and ran the signing command and the final
> > code was 1 as well, without editing a thing.
> > 
> > Is this the right behavior here?
> 
> It sounds like this confirms the codesign tests that Nick did in comment #3
> - modifying update-settings.ini or channel-prefs.js breaks the signature.

This will indeed break the signature.

> > So bottom line is that I did not received any 'Firefox may damage your
> > computer' notification. If there is anything I can help with here please
> > needinfo me.
> 
> AFAICT, we've only seen this dialog on Sheppy's machine when running Firefox
> as a Limited User w/ parental controls enabled. I'm not sure if that's
> coincidence, or if that dialog truly only is shown for accounts like that.
> 
> Either way, we need to make sure we're not invalidating signatures when we
> ship an RC to Beta. I'm hoping this will get rid of the "Firefox may damage
> your computer" message too.
> 
> Stephen, any thoughts on what the right thing to do here is? The tl;dr is
> that we have a valid use case for modifying update-settings.ini and
> channel-prefs.js when we ship RCs to the beta channel. These files are not
> updated as part of the MAR (to avoid changing the user's channel), but
> whatever file contains their checksums is. Can we exclude them from the
> signature somehow?

I'm not sure how we ship RCs to the beta channel. Can we sign them separately for the beta channel so that the MAR will have the proper checksums for the update-settings.ini and channel-prefs.js files? Excluding files from the signature is no longer possible with Apple's v2 signing.

Comment 16

[[e-mail] (:bhearsum)]

(In reply to Stephen A Pohl [:spohl] from comment #15)
> (In reply to Ben Hearsum (:bhearsum) from comment #14)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #11)
> > > (In reply to Ritu Kothari (:ritu) from comment #10)
> > > So bottom line is that I did not received any 'Firefox may damage your
> > > computer' notification. If there is anything I can help with here please
> > > needinfo me.
> > 
> > AFAICT, we've only seen this dialog on Sheppy's machine when running Firefox
> > as a Limited User w/ parental controls enabled. I'm not sure if that's
> > coincidence, or if that dialog truly only is shown for accounts like that.
> > 
> > Either way, we need to make sure we're not invalidating signatures when we
> > ship an RC to Beta. I'm hoping this will get rid of the "Firefox may damage
> > your computer" message too.
> > 
> > Stephen, any thoughts on what the right thing to do here is? The tl;dr is
> > that we have a valid use case for modifying update-settings.ini and
> > channel-prefs.js when we ship RCs to the beta channel. These files are not
> > updated as part of the MAR (to avoid changing the user's channel), but
> > whatever file contains their checksums is. Can we exclude them from the
> > signature somehow?
> 
> I'm not sure how we ship RCs to the beta channel. Can we sign them
> separately for the beta channel so that the MAR will have the proper
> checksums for the update-settings.ini and channel-prefs.js files? Excluding
> files from the signature is no longer possible with Apple's v2 signing.

It's technically possible to resign things for the beta channel, but it somewhat negates the point of shipping the RC to Beta -- we want to test the _exact_ bits we'd ship to release users.

Looks like we talked about this a bit when we first implemented these changes, in bug 1046306, comments 15-17. bug 1047728 also has some talk of it. It looks like we knew about this up front, and considered putting these files into the root of the bundle to avoid the signature, put didn't end up doing that.

Comment 17

[[e-mail] (:bhearsum)]

Just to be clear: I'm not certain that this channel-prefs.js/update-settings.ini subthread is the cause of the "Firefox may damage your computer" message, but it's a leading suspect.

Comment 18

[Stephen A Pohl [:spohl]]

(In reply to Ben Hearsum (:bhearsum) from comment #16)
> Looks like we talked about this a bit when we first implemented these
> changes, in bug 1046306, comments 15-17. bug 1047728 also has some talk of
> it. It looks like we knew about this up front, and considered putting these
> files into the root of the bundle to avoid the signature, put didn't end up
> doing that.

Right, Gatekeeper failed to verify the root of the app bundle when we first heard about v2 signing requirements. However, Apple closed that loophole before releasing v2 signing, so this was no longer an option. I couldn't find the exact bug and comment where we first discovered that Apple had closed this loophole, but bug 1046306 comment 26 and bug 1047728 comment 13 refer to it.

(In reply to Ben Hearsum (:bhearsum) from comment #17)
> Just to be clear: I'm not certain that this
> channel-prefs.js/update-settings.ini subthread is the cause of the "Firefox
> may damage your computer" message, but it's a leading suspect.

Agreed. I would bet money on this being the culprit. Could we confirm with a resigned RC build?

Unfortunately, I can't see a way to solve this without resigning the RC builds for beta.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

SV team was unable to repro this issue, 44 is now live. Though this may come up for Fx45.