1239166 - platform work to support Microsoft Family Safety functionality

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

See https://wiki.mozilla.org/SecurityEngineering/Untrusted_Certificates_in_Windows_Child_Mode

In short, the purpose of this bug is to implement functionality that will allow accounts with Family Safety enabled (i.e. a local MiTM provided by the operating system) to continue functioning with Firefox.

This will be implemented behind a pref to begin with. If enabled, Firefox will attempt to detect if the account it is running on has Family Safety enabled (see https://msdn.microsoft.com/en-us/library/windows/desktop/jj155495%28v=vs.85%29.aspx ). If so, it will attempt to import the certificate associated with the traffic interceptor (see https://support.microsoft.com/en-us/kb/2965142#bookmark-2 ).

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

Review commit: https://reviewboard.mozilla.org/r/30611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30611/

Comment 2

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review28223

::: security/manager/ssl/nsNSSComponent.cpp:408
(Diff revision 1)
> +  WCHAR lpAccountName[256]; // TODO: buffer sizes, etc.

I'm not certain, but you might have a slightly easier time getting the SID of the current user from GetTokenInformation as described in http://stackoverflow.com/a/251267/1508094.

::: security/manager/ssl/nsNSSComponent.cpp:487
(Diff revision 1)
> +      rv = parentalControlsRegKey->ReadIntValue(

This checks whether the overall parental controls system is enabled for an account, but Family Safety is just one feature of that and can be separately disabled.
It looks like the registry value for just Family Safety is "[SID]\Web\Filter On".

::: security/manager/ssl/nsNSSComponent.cpp:583
(Diff revision 1)
> +static const wchar_t* WindowsDefaultRootStoreName = L"Root";

nit: I doubt it matters, but the documented string value is all caps.

::: security/manager/ssl/nsNSSComponent.cpp:1301
(Diff revision 1)
> +  if (AccountHasFamilySafetyEnabled()) {

It appears the version of Family Safety that's available for Vista and Win7 doesn't use the certificate, so you might want to skip the whole procedure for < Win8.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review28223

Thanks for taking a look!

> I'm not certain, but you might have a slightly easier time getting the SID of the current user from GetTokenInformation as described in http://stackoverflow.com/a/251267/1508094.

Thanks for the pointer. Code-wise, it looks like that might end up being longer due to needing to get various handles (unless I'm misunderstanding the APIs).

> This checks whether the overall parental controls system is enabled for an account, but Family Safety is just one feature of that and can be separately disabled.
> It looks like the registry value for just Family Safety is "[SID]\Web\Filter On".

Good catch.

> It appears the version of Family Safety that's available for Vista and Win7 doesn't use the certificate, so you might want to skip the whole procedure for < Win8.

Good call.

Comment 4

[Molly Howell (she/her) (mostly inactive)]

https://reviewboard.mozilla.org/r/30611/#review28223

> Thanks for the pointer. Code-wise, it looks like that might end up being longer due to needing to get various handles (unless I'm misunderstanding the APIs).

Yeah, I was afraid of that. It's fine how it is, in that case.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30611/diff/1-2/

Comment 6

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review29471

Comment 7

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review29689

LGTM

::: security/manager/ssl/nsNSSComponent.cpp:532
(Diff revision 2)
> +  // TODO: check this works on 32-bit windows

Does it?

Comment 8

[Richard Barnes [:rbarnes]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review30789

Overall, this looks fine.  Thanks for figuring out the nuances of how this thing works.

My only major comment is that I would like to have a little more resolution on the plan with regard to telemery and enabling by default.

::: security/manager/ssl/nsNSSComponent.cpp:598
(Diff revision 2)
> +                                      L"Logging Required", 1, loggingRequired);

Nit: It might be slightly nicer if the various strings in this method were constants.

::: security/manager/ssl/nsNSSComponent.cpp:738
(Diff revision 2)
> +UnloadFamilySafetyRoot()

Since this method and `LoadFamilySafetyRoot` can fail, you should return something other than void, if only so that we can collect telemetry on how well this is working.

::: security/manager/ssl/nsNSSComponent.cpp:1453
(Diff revision 2)
> +  HandleFamilySafety();

Nit: `HandleFamilySafety` is a little generic.  Maybe `EnableFamilySafetyCompatibility`?

::: security/manager/ssl/nsNSSComponent.cpp:1707
(Diff revision 2)
> +      HandleFamilySafety();

What's the criterion for enabling this pref by default?  Should we go ahead and enable *detection* of Family Safety, even if we don't take an action based on it?  Or have a multi-level pref, e.g., with 0=nothing, 1=detection, 2=detection+remediation.

::: security/manager/ssl/nsNSSComponent.cpp:1707
(Diff revision 2)
> +      HandleFamilySafety();

Telemetry would be good here -- How often do we enter HandleFamilySafety?  How often is it on?  How often do we successfully load the root?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review29689

> Does it?

¯\_(ツ)_/¯
The plan is to have Matt et. al. test a build before this actually lands.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review30789

> Nit: It might be slightly nicer if the various strings in this method were constants.

It might, but unfortunately that introduces a morass of casting and whatnot. In any case, the duplicate uses of the strings are for logging output only, so there can be typos there and it wouldn't really matter.

> Since this method and `LoadFamilySafetyRoot` can fail, you should return something other than void, if only so that we can collect telemetry on how well this is working.

Sounds good.

> Nit: `HandleFamilySafety` is a little generic.  Maybe `EnableFamilySafetyCompatibility`?

Well, the idea was that it wouldn't unconditionally enable it. I went with "MaybeEnableFamilySafetyCompatibility".

> What's the criterion for enabling this pref by default?  Should we go ahead and enable *detection* of Family Safety, even if we don't take an action based on it?  Or have a multi-level pref, e.g., with 0=nothing, 1=detection, 2=detection+remediation.

That sounds like a good plan.

> Telemetry would be good here -- How often do we enter HandleFamilySafety?  How often is it on?  How often do we successfully load the root?

I added telemetry.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30611/diff/2-3/

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

r? to :vladan for the added telemetry probe, which is intended to be mostly diagnostic, so it's fine if it's opt-in for release populations.

Comment 13

[Richard Barnes [:rbarnes]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review31345

Sorry, thought I had sent this.

::: security/manager/ssl/nsNSSComponent.cpp:782
(Diff revisions 2 - 3)
> +// 0-2: the value of the Family Safety mode pref

It would be a cleaner to have this as a separate histogram.  Otherwise you'll have to recompute the percentages after subtracting out bins 0-2.

Comment 14

[Vladan Djeric (:vladan)]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review35403

Sorry about the delay, I stopped getting my bugzilla notifications for a while

::: toolkit/components/telemetry/Histograms.json:287
(Diff revision 3)
> +    "alert_emails": ["[e-mail]"],

add a bug_numbers field

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review35489

::: security/manager/ssl/nsNSSComponent.cpp:756
(Diff revision 3)
> +  nsresult rv = certDB->FindCertByDBKey(dbKey, getter_AddRefs(cert));

Note to self: FindCertByDBKey can return NS_OK even if it didn't find a certificate (as in, cert might be null here).

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review31345

> It would be a cleaner to have this as a separate histogram.  Otherwise you'll have to recompute the percentages after subtracting out bins 0-2.

I feel like we can address this by doing whatever processing we need to with a custom dashboard.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review35403

> add a bug_numbers field

Ok - added.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30611/diff/3-4/

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/30611/#review38259

::: security/manager/ssl/nsNSSComponent.cpp:537
(Diff revision 4)
> +  }
> +  // TODO: check this works on 32-bit windows
> +  uint32_t flags = nsIWindowsRegKey::ACCESS_READ | nsIWindowsRegKey::WOW64_64;
> +  // TODO: check this works on Windows 8, 8.1, and 10
> +  NS_NAMED_LITERAL_STRING(familySafetyPath,
> +    "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Parental Controls\\Users");

When I tested on Windows 8 "Users" wasn't created until there was actually a user with parental controls enabled. I imagine Windows 8.1 behaves similarly.

::: security/manager/ssl/nsNSSComponent.cpp:798
(Diff revision 4)
> +static void
> +MaybeEnableFamilySafetyCompatibility()
> +{
> +#ifdef XP_WIN
> +  UnloadFamilySafetyRoot();
> +  if (!IsWin8OrLater()) {

The Family Safety feature on Windows 8 doesn't use a MitM certificate, so this doesn't apply. Since the feature also doesn't apply to Firefox in Windows 7 or 10, we should really only run this code for Windows 8.1.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30611/diff/4-5/

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I fixed a number of issues. In particular, we can't use nsIX509CertDB while nsNSSComponent is initializing. Mark, if you could have a look at the interdiff and make sure those changes are ok, that would be great. Another issue was changing this feature to only target Windows 8.1. Nathan, if you could review the changes to mfbt/WindowsVersion.h, that would be great (and any suggestions on what to call the new function would be appreciated).

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

https://reviewboard.mozilla.org/r/30611/#review38853

I assume this is for the mfbt change?  r=me for that.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8709725 [details]
MozReview Request: bug 1239166 - platform work to support Microsoft Family Safety functionality

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30611/diff/5-6/

Comment 24

[Mark Goodwin [:mgoodwin]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #21)
> Mark, if you could have a look at the interdiff and make sure those changes are ok, that would be great.

Still looks good to me.

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ok, I think this is ready to land. Here's the latest try run with some tests hopefully indicating that the nsNSSCertificate/DB changes didn't break things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc302efd3c1

Thank you everyone for the reviews!

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/201173cd6886

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/201173cd6886

Comment 28

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Windows_Child_Mode