1552600 - Local policies.json should augment system policy (not override)

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P3)

Description

[m]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.157 Safari/537.36

Steps to reproduce:

create a distribution folder in the main firefox folder. create a policies.json file inside the distribution folder. insert text into policies.json file:

{
"policies": {
"DisableAppUpdate": true
}
}

start firefox. go to about, help.

Actual results:

firefox updated itself (in previous ver 66.0.4). if in 66.0.5, it checks then says "Firefox is up to date"

Expected results:

it shouldn't update.

Comment 1

[Mike Kaply [:mkaply]]

What platform was this on?

Comment 2

[m]

windows 10 pc v1803 build 17134.765

Comment 3

[Mike Kaply [:mkaply]]

I've verified this is working for me.

Is it possible you have other policies applied via GPO? If so, policies.json is not read.

Can you go to about:policies and check?

Comment 4

[m]

i have nothing showing under about:policies except:

Certificates Install "C:\ProgramData\AVAST Software\Avast\wscert.der"

Comment 5

[Mike Kaply [:mkaply]]

So that's the problem.

Avast has added a policy to your machine, so changes to policies.json aren't picked up.

Comment 6

[m]

is there a way to prevent avast from injecting itself into firefox?

Comment 7

[Mike Kaply [:mkaply]]

is there a way to prevent avast from injecting itself into firefox?

Unfortunately not. We don't approve of how they are doing it and will let them know, but as long as they are installed, they will keep adding the entry.

Comment 8

[Mike Kaply [:mkaply]]

What version of Windows are you using? Home or PRo?

Comment 9

[m]

darn, that's just disappointing to hear that there's no workaround and that avast is not playing nice with firefox.

windows 10 pro

thanks for looking into it anyway.

Comment 10

[Mike Kaply [:mkaply]]

I'm going to morph this bug.

This has been discussed before.

Local policies.json should be added to existing policy (but never overriding).

And this should only happen with top level policies.

Comment 11

[Mike Kaply [:mkaply]]

Attachment: Bug 1552600 - Allow policies.json to augment platform policy.

Comment 12

[Mike Kaply [:mkaply]]

I know the first question for this is going to be "what about tests?"

Software/Policies can't be written without admin privileges, so we can't create a test for this (as far as I know).

I'm investigating.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Mike Kaply [:mkaply] from comment #12)

Software/Policies can't be written without admin privileges, so we can't create a test for this (as far as I know).

If HKCU/Software/Policies key requires the admin priv, why does HKLM take precedence over HKCU? The current order does not match other Microsoft policies AFAIK.

Comment 14

[Mike Kaply [:mkaply]]

If HKCU/Software/Policies key requires the admin priv, why does HKLM take precedence over HKCU? The current order does not match other Microsoft policies AFAIK.

From https://github.com/mozilla/policy-templates/issues/286#issuecomment-436886413

"By design not all registry entries in HKEY_CURRENT_USER are writable from user.
The specific key HKEY_CURRENT_USER\Software\Policies is writable only from administrators otherwise any users or processes running in the same security context could modify settings and it would be too easy to bypass security and organization policies.
The same for HKEY_LOCAL_MACHINE\Software\Policies."

HKLM takes precedence over HKCU because that's the way it is defined to work. This was researched and tested.

Comment 15

[m]

new version of avast does not inject a certificate anymore so this bug can be closed.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ed3c01f009d
Allow policies.json to augment platform policy. r=Felipe

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acf3896e7d71
Backed out changeset 4ed3c01f009d for browser/components/enterprisepolicies/tests/* failures CLOSED TREE

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 4ed3c01f009d (Bug 1552600) for browser/components/enterprisepolicies/tests/* failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=4ed3c01f009d3f7a57acbbe4270b06287f578a9f&tochange=acf3896e7d71b2a929859090e943458136498df9&selectedJob=286038365

Backout link: https://hg.mozilla.org/integration/autoland/rev/acf3896e7d71b2a929859090e943458136498df9

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286038365&repo=autoland&lineNumber=12125

[task 2020-01-22T22:17:28.934Z] 22:17:28 INFO - TEST-START | browser/components/enterprisepolicies/tests/browser/disable_forget_button/browser_policy_disable_forgetbutton.js
[task 2020-01-22T22:17:28.934Z] 22:17:28 INFO - GECKO(6378) | Chrome file doesn't exist: /builds/worker/workspace/build/tests/mochitest/browser/browser/components/enterprisepolicies/tests/browser/disable_forget_button/head.js
[task 2020-01-22T22:17:28.937Z] 22:17:28 INFO - TEST-INFO | started process screentopng
[task 2020-01-22T22:17:29.863Z] 22:17:29 INFO - TEST-INFO | screentopng: exit 0
[task 2020-01-22T22:17:29.864Z] 22:17:29 INFO - Buffered messages logged at 22:17:28
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - Entering test bound test_policy_disable_forget_button
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - Buffered messages finished
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - TEST-UNEXPECTED-FAIL | browser/components/enterprisepolicies/tests/browser/disable_forget_button/browser_policy_disable_forgetbutton.js | Forget Button was not created - Didn't expect view, but got it
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - Stack trace:
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - chrome://mochikit/content/browser-test.js:test_isnot:1329
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - chrome://mochitests/content/browser/browser/components/enterprisepolicies/tests/browser/disable_forget_button/browser_policy_disable_forgetbutton.js:test_policy_disable_forget_button:8
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1062
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1097
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:925
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:808
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - Leaving test bound test_policy_disable_forget_button
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - GECKO(6378) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - GECKO(6378) | MEMORY STAT | vsize 2828MB | residentFast 370MB | heapAllocated 140MB
[task 2020-01-22T22:17:29.867Z] 22:17:29 INFO - TEST-OK | browser/components/enterprisepolicies/tests/browser/disable_forget_button/browser_policy_disable_forgetbutton.js | took 60ms

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

Another failure which seems to have started with these changes:
https://treeherder.mozilla.org/#/jobs?repo=autoland&tochange=acf3896e7d71b2a929859090e943458136498df9&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-8%2Cm-fis%28bc8%29&fromchange=49b97ea7f14de57a67c434a6aae5217b21f498a2&selectedJob=286037299

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286037299&repo=autoland&lineNumber=23922

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

Another failure which seems to have started with these changes:
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=03aa8dbc3ae91532a1f81b2d5286cef5764e032b&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-7%2Cm-fis%28bc7%29&tochange=acf3896e7d71b2a929859090e943458136498df9

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286038357&repo=autoland&lineNumber=6075

Comment 21

[Mike Kaply [:mkaply]]

I think that these are unrelated. I've done lots of local testing and I'm not seeing them. Are we sure there is nothing else that could have caused this?

I'll do some try runs tomorrow.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b57602767230
Allow policies.json to augment platform policy. r=Felipe

Comment 23

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b57602767230

Comment 24

[Mike Kaply [:mkaply]]

Comment on attachment 9067126 [details]
Bug 1552600 - Allow policies.json to augment platform policy.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Policy only.
• User impact if declined: Adding new policies in JSON don't work if policy specified via GPO. This is primarily an issue where antivirus has added policy (not enterprises). This does NOT allow a user to override enterprise policy, just augment
• Fix Landed on Version: 74
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Policy only, all existing tests work.
• String or UUID changes made by this patch:

Comment 25

[Mike Kaply [:mkaply]]

What's the best way to request a backout of this from trunk? This exposed some other issues in policy that need to be addressed first.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Ping in #sheriffs. I assume you'll want it backed out from Beta also?

Comment 27

[Andreea Pavel [:apavel]]

Central backout: https://hg.mozilla.org/mozilla-central/rev/1e56bbf0bc276bcae97fddd1302888bbe2d8fa8c
Beta backout: https://hg.mozilla.org/releases/mozilla-beta/rev/2277b58b1038c589ea5e7a143108c99c5b3c646a

Comment 28

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mkaply, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 29

[Mike Kaply [:mkaply]]

This is going to need a rewrite, so this patch isn't going in.

Comment 30

[Emil Ghitta, QA [:emilghitta]]

Removing qe-verify+ and ni? for now.

Comment 31

[Mike Kaply [:mkaply]]

Attachment: Bug 1552600 - Allow policies.json to augment platform policy. r?emalysz!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f19fab8bf1b
Allow policies.json to augment platform policy. r=emalysz

Comment 34

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7f19fab8bf1b

Comment 35

[Mike Kaply [:mkaply]]

Asking for verification from QA for eventual uplift.

Basically three scenarios to test.

1. If different policies are set in GPO and in policies.json, both activate.
2. If the same policy is set in GPO and in policies.json, GPO wins.
3. The failure in https://bugzilla.mozilla.org/show_bug.cgi?id=1616007

Thanks!

Comment 36

[Mike Kaply [:mkaply]]

Comment on attachment 9199326 [details]
Bug 1552600 - Allow policies.json to augment platform policy. r?emalysz!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is an enterprise specific feature related to policy, wanted for consistency with Firefox 87. I was waiting to ask for approval because I wanted QA to look, but we're out of time.
• User impact if declined: If an admin sets registry policy, policies.json isn't read. This patch allows policies.json to augment registry but not override.

This is probably our most requested feature

• Fix Landed on Version: 87
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): When this landed before, there was no way to test it. I implemented a way to test registry policy specifically so that we could test this patch.

I implemented multiple tests to try to cover all edgecases.

• String or UUID changes made by this patch:

Comment 37

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 87.0b7 (BuildId:20210307185839) on Windows 10 64bit.

Tested the following:

Scenario 1:
Used a policies.json file that contained the following code:
{ "policies": { "DisableAppUpdate": true } }And set a GPO level policy that sets Cryptomining to false

Result: Both policies are active.

Scenario 2:

Added the DisableTelemetry -> true inside the policies.json file and set the same policiy to false at GPO level.

Result: The value of DisableTelemetry is false -> GPO wins.

Scenario 3:

Entered the following invalid policies.json file:
{ "policies": { "DisableAppUpdate": true, "DisableTfwfawelemetry":true }

Result: The DisableAppUpdate policy is enabled and successfully applied alongside with the ones set at GPO level.

Scenario 4:

Forced a parsing JSON error by using
{ "policies": { "DisableAppUpdate": true, "DisableTelemetry":true }

Result: The GPO level policies are not affected (they are still successfully applied).

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9199326 [details]
Bug 1552600 - Allow policies.json to augment platform policy. r?emalysz!

Needed for release parity. Approved for 78.9esr.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/35632a8bfc99