Closed Bug 1369911 Opened 2 years ago Closed 2 years ago

gather telemetry on the prevalence of 3rd party PKCS#11 modules

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: keeler, Assigned: keeler)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

PKCS#11 modules can be a bit problematic for Firefox in that loading them causes 3rd-party binaries to be injected into Firefox's memory space. It would be great to run them in a separate (sandboxed?) process so that flaws in their code would be less likely to interfere with Firefox itself. However, it would be good to first measure how often Firefox users actually have 3rd party PKCS#11 modules. (Note that currently these modules are the only way to enable client certificate authentication with smartcard devices in Firefox.)
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

This is a data-review request to add the telemetry scalar "security.pkcs11_modules_loaded". It counts the number of PKCS#11 modules loaded by the browser. The majority of users will load 2 modules: the default NSS cryptography module and the built-in list of root certificates. Some users use smart card devices to authenticate to websites. To do this, they generally load a 3rd party PKCS#11 module, which essentially means loading external code in Firefox's memory space. Misbehaving or buggy modules can (and have) caused stability problems for users. The intent of this measurement is to determine how prevalent these are overall so we can make a decision on how much to invest in improving this situation for the users that need these modules.
Attachment #8874496 - Flags: feedback?(benjamin)
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

https://reviewboard.mozilla.org/r/145866/#review150224

Cool.

::: security/manager/ssl/PKCS11.cpp:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "PKCS11.h"
>  
>  #include "ScopedNSSTypes.h"
> +#include "mozilla/TelemetryScalarEnums.h"

`ScalarSet()` is defined in "mozilla/Telemetry.h", so we should probably include that as well.
(Telemetry.h includes TelemetryScalarEnums.h though, so maybe we can just include Telemetry.h.)

::: security/manager/ssl/tests/unit/test_pkcs11_module.js:65
(Diff revision 1)
>  
> +function checkModuleTelemetry(expectedNumberOfModules) {
> +  let telemetryService = Cc["@mozilla.org/base/telemetry;1"]
> +                           .getService(Ci.nsITelemetry);
> +  let telemetry = telemetryService.snapshotScalars(
> +    Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT)["parent"];

Looks like ESLint is unhappy with this line:
  65:53  error  ["parent"] is better written in dot notation.  dot-notation (eslint)
Attachment #8874496 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

https://reviewboard.mozilla.org/r/145866/#review150344

data-r=me

FWIW, I think you're under-collecting data: I feel like you could easily record the actual list of modules (DLL names or common name) either as a string or keyed boolean, to know actually what modules these are. We already collect the module list, so it's not like it's new data: we just know exactly which of these are PKCS modules.
Attachment #8874496 - Flags: review+
Attachment #8874496 - Flags: feedback?(benjamin)
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

https://reviewboard.mozilla.org/r/145866/#review150224

Thanks, although since this changed somewhat, I'm going to ask for a re-review.

> `ScalarSet()` is defined in "mozilla/Telemetry.h", so we should probably include that as well.
> (Telemetry.h includes TelemetryScalarEnums.h though, so maybe we can just include Telemetry.h.)

Ok - I went with just including Telemetry.h.

> Looks like ESLint is unhappy with this line:
>   65:53  error  ["parent"] is better written in dot notation.  dot-notation (eslint)

Ok - fixed.
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

https://reviewboard.mozilla.org/r/145866/#review150344

Thanks! Looking at bug 1330833, I decided to go with gathering the DLL names (with fallback to common name if the DLL name isn't present like for the built-in module).
Attachment #8874496 - Flags: review+ → review?(cykesiopka.bmo)
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

https://reviewboard.mozilla.org/r/145866/#review151384

Also looks good.

::: security/manager/ssl/PKCS11.cpp
(Diff revisions 1 - 2)
>    SECStatus srv = SECMOD_DeleteModule(moduleName.get(), &modType);
>    if (srv != SECSuccess) {
>      return NS_ERROR_FAILURE;
>    }
>  
> -  CountModules();

I guess the idea here is to ignore deletions so modules that are added and deleted quickly still show up in telemetry?

::: security/manager/ssl/PKCS11.cpp:81
(Diff revisions 1 - 2)
> +// Given a PKCS#11 module, determines an appropriate name to identify it for the
> +// purposes of gathering telemetry. For 3rd party PKCS#11 modules, this should
> +// be the name of the dynamic library that implements the module. For built-in
> +// NSS modules, it will be the common name of the module.
> +// Because the result will be used as a telemetry scalar key (which must be less
> +// than 70 characters), this function will also truncate the result if it

Heh, I was reading https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html#keyed-scalars and was confused why this said \<70 instead of \<=70, but reading TelemetryScalar.cpp tells me you are indeed correct. I wonder why there's this mismatch.
Attachment #8874496 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8874496 [details]
bug 1369911 - gather telemetry on the prevalence of 3rd party PKCS#11 modules  data-review=bsmedberg

https://reviewboard.mozilla.org/r/145866/#review151384

Thanks!

> I guess the idea here is to ignore deletions so modules that are added and deleted quickly still show up in telemetry?

I was basically going for "what modules are loaded ever?", so I decided it wasn't necessary to unset the scalar when they get removed.

> Heh, I was reading https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html#keyed-scalars and was confused why this said \<70 instead of \<=70, but reading TelemetryScalar.cpp tells me you are indeed correct. I wonder why there's this mismatch.

Yeah, I expect this is just an imprecise language issue.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76a620a287bf
gather telemetry on the prevalence of 3rd party PKCS#11 modules r=bsmedberg,Cykesiopka data-review=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/76a620a287bf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.