Closed
Bug 1369911
Opened 7 years ago
Closed 7 years ago
gather telemetry on the prevalence of 3rd party PKCS#11 modules
Categories
(Core :: Security: PSM, enhancement, P1)
Core
Security: PSM
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
mozreview-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/#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 4•7 years ago
|
||
mozreview-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+
Updated•7 years ago
|
Attachment #8874496 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8874496 -
Flags: review+ → review?(cykesiopka.bmo)
Comment 8•7 years ago
|
||
mozreview-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 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+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa9ad4ea3d8c
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/76a620a287bf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•