Closed Bug 1418131 Opened 2 years ago Closed 2 years ago

Add "Windows Security Center" data to about:support, telemetry environment, etc

Categories

(Core :: Widget: Win32, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(4 files)

Starting with Windows 8, we can use the IWSCProductList interface to enumerate registered AV/anti-spyware/firewall products that are installed on the system.

This would be useful into to add to about:support, crash report annotations, telemetry environment, etc.

CoCreateInstance using CLSID_WSCProductList.
Priority: -- → P3
I have some patches for this.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8935926 - Flags: review?(francois)
Comment on attachment 8935922 [details]
Bug 1418131: Part 1 - Add Windows Security Center info to nsSystemInfo;

https://reviewboard.mozilla.org/r/206782/#review212602

Looks good, I added some notes but I'll leave it up to you if you want to change anything.

::: xpcom/base/nsSystemInfo.cpp:261
(Diff revision 1)
> +    if (FAILED(hr)) {
> +      return hr;
> +    }
> +
> +    _bstr_t bName;
> +    hr = product->get_ProductName(bName.GetAddress());

Does the produce name include the version, or do we need to get the timestamp as well (the docs are pretty vague on this).

::: xpcom/base/nsSystemInfo.cpp:272
(Diff revision 1)
> +    hr = product->get_ProductState(&state);
> +    if (FAILED(hr)) {
> +      return hr;
> +    }
> +
> +    // We only care about products that are active

Do we? Maybe we should include it with an attribute. "foo av (snoozed)". On the other hand if we don't want it we could check this before getting the name.

::: xpcom/base/nsSystemInfo.cpp:282
(Diff revision 1)
> +
> +    if (!aOutput.IsEmpty()) {
> +      aOutput.AppendLiteral(u";");
> +    }
> +
> +    nsDependentString name((wchar_t*)bName, bName.length());

I *think* you can avoid the intermediary here and just use `Append((wchar_t*)bName, bName.length())`.

::: xpcom/base/nsSystemInfo.cpp:313
(Diff revision 1)
> +
> +  // 1. Antivirus
> +  RefPtr<IWSCProductList> prodList;
> +  HRESULT hr = ::CoCreateInstance(clsid, nullptr, CLSCTX_INPROC_SERVER, iid,
> +                                  getter_AddRefs(prodList));
> +  if (FAILED(hr)) {

Okay, so does this safely fail on Win7? I assume it does but just checking.

::: xpcom/base/nsSystemInfo.cpp:327
(Diff revision 1)
> +  hr = EnumWSCProductList(aAVInfo, WrapNotNull(prodList.get()));
> +  if (FAILED(hr)) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  // 2. Anti-spyware

I suppose you could fold this into a helper or just a loop over the provider types.

::: xpcom/base/nsSystemInfo.cpp:822
(Diff revision 1)
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return rv;
>      }
>    }
> +
> +  nsAutoString avInfo, antiSpyInfo, firewallInfo;

Differentiating b/w AV, anti-spyware, and firewalls seems kind of pointless to me. Would it make more sense to just get an array of security things, then munge them together here?

ie:

```
nsTArray<nsString> secItems;
GetWindowsSecurityCenterInfo(secItems);
nsString registeredSecItems = secItems.join(";"); // okay that's not real but you get the idea
```

It's fine if you see merit in differentiating, I don't feel too strongly about this.
Attachment #8935922 - Flags: review?(erahm) → review+
Comment on attachment 8935926 [details]
Data Review Request Form

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Only in the data review request form attached to this bug (I think).

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Aaron Klotz.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default-on.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent.
Attachment #8935926 - Flags: review?(francois) → review+
Comment on attachment 8935924 [details]
Bug 1418131: Part 3 - Add Windows Security Center info to telemetry environment;

https://reviewboard.mozilla.org/r/206786/#review212670

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1704
(Diff revision 1)
>        memoryMB,
>        virtualMaxMB: virtualMB,
>        cpu: this._getCpuData(),
>        os: this._getOSData(),
>        hdd: this._getHDDData(),
> +      sec: this._getSecurityAppData(),

This is Windows only?
Let's limit adding this to `platform === "win"`.

This addition will need:
- checking the tests: `mach test toolkit/components/telemetry`
- at least updating `test_TelemetryEnvironment.js`
- updating the docs: `toolkit/components/telemetry/docs/data/environment.rst`

For pipeline dependencies, we can flag :mreid after the client code is settled.
Attachment #8935924 - Flags: review?(gfritzsche)
Comment on attachment 8935922 [details]
Bug 1418131: Part 1 - Add Windows Security Center info to nsSystemInfo;

https://reviewboard.mozilla.org/r/206782/#review214178
Attachment #8935922 - Flags: review?(jmathies) → review+
Comment on attachment 8935923 [details]
Bug 1418131: Part 2 - Add security software section to about:support;

https://reviewboard.mozilla.org/r/206784/#review214180
Attachment #8935923 - Flags: review?(jmathies) → review+
Comment on attachment 8935924 [details]
Bug 1418131: Part 3 - Add Windows Security Center info to telemetry environment;

https://reviewboard.mozilla.org/r/206786/#review217458

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1604
(Diff revision 2)
>  
>    /**
> +   * Get registered security product information.
> +   * @return Object containing the security product data
> +   */
> +  _getSecurityAppData() {

It looks like the data size here can be unbounded (array lengths, string sizes).
Can you
- put explicit bounds on it or
- document any existing bounds?

I missed that in the last pass.
Attachment #8935924 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> Comment on attachment 8935924 [details]
> Bug 1418131: Part 3 - Add Windows Security Center info to telemetry
> environment;
> 
> https://reviewboard.mozilla.org/r/206786/#review217458
> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1604
> (Diff revision 2)
> >  
> >    /**
> > +   * Get registered security product information.
> > +   * @return Object containing the security product data
> > +   */
> > +  _getSecurityAppData() {
> 
> It looks like the data size here can be unbounded (array lengths, string
> sizes).
> Can you
> - put explicit bounds on it or
> - document any existing bounds?
> 
> I missed that in the last pass.

There are not any explicit bounds. Can you clarify what you are looking for with respect to bounds? We don't do that for various other string properties pulled from nsSystemInfo (eg. binHDDModel)
Flags: needinfo?(gfritzsche)
We could generally be better about limits, yes. We started on some of the data but haven't completed this yet.

Here we pull in a list of entries, so i'd like us to be on the safe side:
- Limiting the number of entries to an arbitrary higher number.
- Use limitStringToLength() to be safe on the individual items.
Flags: needinfo?(gfritzsche)
Comment on attachment 8935924 [details]
Bug 1418131: Part 3 - Add Windows Security Center info to telemetry environment;

https://reviewboard.mozilla.org/r/206786/#review217458

> It looks like the data size here can be unbounded (array lengths, string sizes).
> Can you
> - put explicit bounds on it or
> - document any existing bounds?
> 
> I missed that in the last pass.

I decided to place a 256 character bound on the string *before* the split. In the unlikely event that something exceeds that bound, I think that will produce the cleanest result.
Comment on attachment 8935924 [details]
Bug 1418131: Part 3 - Add Windows Security Center info to telemetry environment;

https://reviewboard.mozilla.org/r/206786/#review218522

Thanks, this looks good to me.

If you need this available in Redash / sql.tmo / some dataset, please file a bug with the details:
https://bugzilla.mozilla.org/enter_bug.cgi?product=Data%20Platform%20and%20Tools&component=Datasets%3A%20General

If you are planning to access this through custom analysis, the data will available through ATMO as it comes in.

::: toolkit/components/telemetry/docs/data/environment.rst:205
(Diff revision 3)
>                  status: <string>,    // See the status codes above.
>                },
>              },
>            },
>          appleModelId: <string>, // Mac only or null on failure
> +        sec: { // This feature is Windows-only

"Windows 8+ only."
Attachment #8935924 - Flags: review?(gfritzsche) → review+
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5dd6963e2a12
Part 1 - Add Windows Security Center info to nsSystemInfo; r=erahm,jimm
https://hg.mozilla.org/integration/autoland/rev/73dca2043525
Part 2 - Add security software section to about:support; r=jimm
https://hg.mozilla.org/integration/autoland/rev/75591ea83b3c
Part 3 - Add Windows Security Center info to telemetry environment; r=gfritzsche
This should be fixed now. Relanding.
Flags: needinfo?(aklotz)
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d66bae3d5be1
Part 1 - Add Windows Security Center info to nsSystemInfo; r=erahm,jimm
https://hg.mozilla.org/integration/autoland/rev/625b2d120fd9
Part 2 - Add security software section to about:support; r=jimm
https://hg.mozilla.org/integration/autoland/rev/073eb94e1d21
Part 3 - Add Windows Security Center info to telemetry environment; r=gfritzsche
Requesting another backout -- mingw doesn't include the required headers, so I need to go back and disable this code for mingw.
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0cf39ae0b3d
Part 1 - Add Windows Security Center info to nsSystemInfo; r=erahm,jimm
https://hg.mozilla.org/integration/autoland/rev/a287c5db8be6
Part 2 - Add security software section to about:support; r=jimm
https://hg.mozilla.org/integration/autoland/rev/8962ed624e0c
Part 3 - Add Windows Security Center info to telemetry environment; r=gfritzsche
OK, this should be the final push. I've run a full try unit test suite.
Flags: needinfo?(aklotz)
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcad66c3d2e2
Part 1 - Add Windows Security Center info to nsSystemInfo; r=erahm,jimm
https://hg.mozilla.org/integration/autoland/rev/e825d7b57a4f
Part 2 - Add security software section to about:support; r=jimm
https://hg.mozilla.org/integration/autoland/rev/d47ec9fc2586
Part 3 - Add Windows Security Center info to telemetry environment; r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/fcad66c3d2e2
https://hg.mozilla.org/mozilla-central/rev/e825d7b57a4f
https://hg.mozilla.org/mozilla-central/rev/d47ec9fc2586
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1431195
Blocks: 1431198
Blocks: 1433149
Blocks: 1435773
No longer blocks: injecteject
Depends on: 1363586
You need to log in before you can comment on or make changes to this bug.