Closed Bug 1429148 Opened 2 years ago Closed 2 years ago

Password policies: lock "remember passwords" and don't allow a master password to be set

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox60 --- verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(3 files)

Two different password-related policies requested for the MVP:

  - Don't remember passwords (signon.rememberSignons)
  - Don't allow a master password to be set*

*CCK2 only hides the dialog to set a master password from the Preferences dialog. This might be enough to do too on our MVP.  Fully disabling the master password functionality seems more work.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
For the first policy, originally I was gonna do as most other policies to be only about disabling it. However, there was a request in the enterprise mailing list about also enforcing this to remain true (because users were inadvertently changing this setting), and since this is a trivial change in implementation, I thought it would be nice to support it.
Summary: Password policies: don't remember passwords and don't allow a master password → Password policies: lock "remember passwords" and don't allow a master password to be set
Comment on attachment 8951121 [details]
Bug 1429148 - Policy: Enforce choice for the Remember Passwords setting.

https://reviewboard.mozilla.org/r/220374/#review226618

Thanks
Attachment #8951121 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8951122 [details]
Bug 1429148 - Add nsIPK11Token.isInternalKeyToken.

https://reviewboard.mozilla.org/r/220376/#review226626

r=me on the stuff outside /security/manager/

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_masterpassword.js:6
(Diff revision 1)
> +add_task(async function test_policy_remember_passwords() {
> +  await setupPolicyEngineWithJson({
> +    "policies": {
> +      "DisableMasterPassword": true
> +    }
> +  });
> +
> +  await BrowserTestUtils.withNewTab("about:preferences#privacy", async browser => {
> +    // eslint-disable-next-line mozilla/no-cpows-in-tests
> +    ok(browser.contentDocument.getElementById("useMasterPassword").disabled, "Master Password checkbox was disabled");

Ideally you would test what happens if a MP was already set (if the behaviour differs).

::: browser/components/preferences/in-content/privacy.js:1138
(Diff revision 1)
>      checkbox.checked = !noMP;
> +    checkbox.disabled = !Services.policies.isAllowed("masterPassword");
>    },

Should it only be disabled if `noMP` is true so that a user who already has a master password can still remove it?

::: security/manager/pki/resources/content/device_manager.js:167
(Diff revision 1)
>            }
>          }
>        }
> +
> +      if (!Services.policies.isAllowed("masterPassword") &&
> +          selected_token.tokenName == getNSSString("InternalToken")) {

I think it's preferred to compare the token to `tokendb.getInternalKeyToken()` and I'm not technically a peer of security/ so you should probably check with keeler. I've never read this code before.
Attachment #8951122 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #5)

> ::: browser/components/preferences/in-content/privacy.js:1138
> (Diff revision 1)
> >      checkbox.checked = !noMP;
> > +    checkbox.disabled = !Services.policies.isAllowed("masterPassword");
> >    },
> 
> Should it only be disabled if `noMP` is true so that a user who already has
> a master password can still remove it?

Good point.

> 
> ::: security/manager/pki/resources/content/device_manager.js:167
> (Diff revision 1)
> >            }
> >          }
> >        }
> > +
> > +      if (!Services.policies.isAllowed("masterPassword") &&
> > +          selected_token.tokenName == getNSSString("InternalToken")) {
> 
> I think it's preferred to compare the token to
> `tokendb.getInternalKeyToken()` and I'm not technically a peer of security/
> so you should probably check with keeler. I've never read this code before.

I tried this but unfortunately the comparison doesn't work. I don't know why though
Attachment #8951122 - Flags: review?(dkeeler)
Keeler, can you take a look at the device_manager.js part of the patch. This is a policy for sysadmins to block users from setting a master password.
Comment on attachment 8951122 [details]
Bug 1429148 - Add nsIPK11Token.isInternalKeyToken.

https://reviewboard.mozilla.org/r/220376/#review226650

It would be nice to have some sort of test for the device manager (similar to the related tests in security/manager/ssl/tests/mochitest/browser). Also, what happens if the user has already set a password? It seems this would make it impossible to un-set it? (Maybe that's the intended behavior, and in any case it won't be a problem if the user's profile has this policy from the beginning.)
It looks like we'll need some way to check if a token is the internal token, so r- for now.

::: security/manager/pki/resources/content/device_manager.js:167
(Diff revision 1)
>            }
>          }
>        }
> +
> +      if (!Services.policies.isAllowed("masterPassword") &&
> +          selected_token.tokenName == getNSSString("InternalToken")) {

Unfortunately I don't think a simple equality check will work (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd have to do is call `PK11_IsInternalKeySlot(mSlot.get())`)

::: security/manager/pki/resources/content/device_manager.js:168
(Diff revision 1)
>          }
>        }
> +
> +      if (!Services.policies.isAllowed("masterPassword") &&
> +          selected_token.tokenName == getNSSString("InternalToken")) {
> +        pw_toggle = true;

I think this needs to be `"true"`, actually, given the rest of this code...
Attachment #8951122 - Flags: review?(dkeeler) → review-
(In reply to David Keeler [:keeler] (use needinfo) from comment #8)
> It would be nice to have some sort of test for the device manager (similar
> to the related tests in security/manager/ssl/tests/mochitest/browser). Also,

Ok, I'll find a way to test the device_manager part too. Shouldn't be too hard.


> what happens if the user has already set a password? It seems this would
> make it impossible to un-set it? (Maybe that's the intended behavior, and in
> any case it won't be a problem if the user's profile has this policy from
> the beginning.)

Matt also mentioned this in comment 5, so I'll leave the checkbox in about:preferences enabled if a master password already exists, to allow it to be unset.

Perhaps I'll also leave this button in the device_manager enabled if selected_token.hasPassword

Also, I'll change the policy name from "DisableMasterPassword" to "CreateMasterPassword" (which will expect a `false` parameter), to make it more explicit that this will only protect before a master password is set in a profile.

> 
> ::: security/manager/pki/resources/content/device_manager.js:167
> (Diff revision 1)
> >            }
> >          }
> >        }
> > +
> > +      if (!Services.policies.isAllowed("masterPassword") &&
> > +          selected_token.tokenName == getNSSString("InternalToken")) {
> 
> Unfortunately I don't think a simple equality check will work
> (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we
> should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd
> have to do is call `PK11_IsInternalKeySlot(mSlot.get())`)

Yeah, I can do it. However, I should note that when the user clicks in the "Change Password" button, the token is already passed by name (see https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/security/manager/pki/resources/content/device_manager.js#357), see this would offer no extra benefit AFAICT.

Let me know if I should make that change anyway or keep the string comparison.

> 
> ::: security/manager/pki/resources/content/device_manager.js:168
> (Diff revision 1)
> >          }
> >        }
> > +
> > +      if (!Services.policies.isAllowed("masterPassword") &&
> > +          selected_token.tokenName == getNSSString("InternalToken")) {
> > +        pw_toggle = true;
> 
> I think this needs to be `"true"`, actually, given the rest of this code...

Good catch. it still works with boolean true, but better to follow the rest of the code.
Needinfo'ing for the question below:

(In reply to :Felipe Gomes (needinfo me!) from comment #9)
> (In reply to David Keeler [:keeler] (use needinfo) from comment #8)
> > Unfortunately I don't think a simple equality check will work
> > (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we
> > should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd
> > have to do is call `PK11_IsInternalKeySlot(mSlot.get())`)
> 
> Yeah, I can do it. However, I should note that when the user clicks in the
> "Change Password" button, the token is already passed by name (see
> https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/security/manager/pki/resources/content/device_manager.js#357 ),
> so this would offer no extra benefit AFAICT.
> 
> Let me know if I should still add an isInternalKeyToken function, or keep the string
> comparison.
Flags: needinfo?(dkeeler)
Sorry, missed this.

(In reply to :Felipe Gomes (needinfo me!) from comment #10)
> Needinfo'ing for the question below:
> 
> (In reply to :Felipe Gomes (needinfo me!) from comment #9)
> > (In reply to David Keeler [:keeler] (use needinfo) from comment #8)
> > > Unfortunately I don't think a simple equality check will work
> > > (`getInternalKeyToken()` creates a new XPCOM object each time...). Maybe we
> > > should add an `isInternalKeyToken` attribute to nsIPK11Token? (All you'd
> > > have to do is call `PK11_IsInternalKeySlot(mSlot.get())`)
> > 
> > Yeah, I can do it. However, I should note that when the user clicks in the
> > "Change Password" button, the token is already passed by name (see
> > https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/security/manager/pki/resources/content/device_manager.js#357 ),
> > so this would offer no extra benefit AFAICT.

Honestly, this isn't ideal either, but since it already exists and since it's essentially going token -> get name -> get token by that same name, I think it's a little less likely to go wrong.

> > 
> > Let me know if I should still add an isInternalKeyToken function, or keep the string
> > comparison.

I still think it would be useful.
Flags: needinfo?(dkeeler)
Comment on attachment 8951122 [details]
Bug 1429148 - Add nsIPK11Token.isInternalKeyToken.

https://reviewboard.mozilla.org/r/220376/#review227894

Perfect - thanks.
Attachment #8951122 - Flags: review?(dkeeler) → review+
Comment on attachment 8952567 [details]
Bug 1429148 - Policy: Don't let a Master Password to be set.

https://reviewboard.mozilla.org/r/221818/#review227906

PSM parts look good to me. Thanks for adding the device manager test. I had a look at the PSM-specific parts of that, and they seem correct (although I'm not an expert on browser-chrome tests).
Attachment #8952567 - Flags: review?(dkeeler) → review+
Comment on attachment 8952567 [details]
Bug 1429148 - Policy: Don't let a Master Password to be set.

r=MattN. Matt had already given r+ with nits for this patch, but since I added a new patch in the queue, ReviewBoard got confused
Attachment #8952567 - Flags: review?(MattN+bmo) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c3184f918f0
Policy: Enforce choice for the Remember Passwords setting. r=MattN
https://hg.mozilla.org/integration/autoland/rev/ffe15fb2541c
Add nsIPK11Token.isInternalKeyToken. r=keeler,MattN
https://hg.mozilla.org/integration/autoland/rev/c9dc10752a47
Policy: Don't let a Master Password to be set. r=keeler
https://hg.mozilla.org/mozilla-central/rev/0c3184f918f0
https://hg.mozilla.org/mozilla-central/rev/ffe15fb2541c
https://hg.mozilla.org/mozilla-central/rev/c9dc10752a47
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Note for testing:

Two policies landed here:

"RememberPasswords"
 - this one can accept both *true* and *false* values, and it should enforce that value to the pref
   "signon.rememberSignons", and the corresponding checkbox in about:preferences ("Remember logins and
   passwords for websites")

"CreateMasterPassword"
 - this one can only accept *false* as a value, and if false, it should not allow a master password to be
   created.
   If a Master Password already exists prior to the policy being used, it *won't* block the master password
   checkbox, as a way to possibly allowing it to be unset
Thanks Felipe for your notes in comment 19.

We tested both policies using JSON file. We verified them manually as fixed.  
"RememberPasswords" policy is tested with values "true" and "false" and "CreateMasterPassword" with "false".


Detailed information about test steps and runs is available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The bug will also be retested with ADMX files when it is ready for testing.
We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.