Create HVCI table for Windows Device Guard #5426

finnstra · 2019-02-08T19:49:52Z

I created a new table to track HVCI settings on a machine, utilizing the WMI class Win32_DeviceGuard. This is useful in determining if a host is configured to use Device Guard or running, and displays various code integrity settings enforced on the machine. In addition, this table returns the machine's unique Instance Identifier.

Currently, this table does not return the available or required security properties of a machine.

This is my first pull request for osquery (and GitHub!) so please let me know if there is a better way to implement this.

Context:

HVCI Settings

muffins

An initial pass. Some of our styling may have changed since I've been reviewing actively, so we'll wanna make sure we get someone else from the core team to also stamp this before we get too far along :)

muffins · 2019-02-27T21:42:34Z

specs/windows/hvci_status.table

+  Column("version", TEXT, "The version number of the Device Guard build."),
+  Column("instance_identifier", TEXT, "The instance ID of Device Guard."),
+  Column("vbs_status", TEXT, "The status of the virtualization based security settings."),
+  Column("code_integirty_policy_enforcement_status", TEXT, "The status of the code integirty policy enforcement settings."),


s/integirty/integrity/

muffins · 2019-02-27T21:43:25Z

osquery/tables/system/windows/hvci_status.cpp

+                                (BSTR)L"ROOT\\MICROSOFT\\WINDOWS\\DEVICEGUARD");
+  const std::vector<WmiResultItem>& wmiResults = wmiSystemReq.results();
+  if (wmiResults.empty()) {
+    LOG(WARNING) << "Error retreiving information from WMI.";


I think for data loss we prefer LOG(ERROR)

muffins · 2019-02-27T21:48:20Z

osquery/tables/system/windows/hvci_status.cpp

+                 umcipolicymethod);
+
+    std::string vbsmethod_str;
+    std::map<long, std::string> vbsmethods;


Why does this need to be a std::map? Perhaps this could just be a statically defined array or vector of std::string, so:

std::vector<std::string> vbs_methods { "VBS_NOT_ENABLED", "VBS_ENABLED_AND_NOT_RUNNING", "VBS_ENABLED_AND_RUNNING", };

Let's also move this up outside of the for loop, so that we're not redefining it every pass.

I was mapping this because the WMI output returned a number, that correlated to a particular setting status (0 = this, 1 = that), but they seem to be in order from 0 to n, so this seems far simpler.

osquery/tables/system/windows/hvci_status.cpp

muffins · 2019-02-27T21:49:45Z

osquery/tables/system/windows/hvci_status.cpp

+    std::map<long, std::string> vbsmethods;
+
+    std::string codepolicystatusmethod_str;
+    std::map<long, std::string> codepolicystatusmethods;


Same note as vbs_methods for both this and the umcipolicymethods.

muffins · 2019-02-27T21:50:26Z

osquery/tables/system/windows/hvci_status.cpp

+    return results;
+  }
+  for (const auto& data : wmiResults) {
+    long vbsmethod;


Here and throughout, feel free to use snake_case for variable names to break things up a bit, so vbs_method, code_policy_status_method, umci_policy_method. Also, considering method is repeated maybe just omit it for brevity?

muffins · 2019-02-27T21:52:52Z

osquery/tables/system/windows/hvci_status.cpp

+  }
+  for (const auto& data : wmiResults) {
+    long vbsmethod;
+    long codepolicystatusmethod;


Here and throughout, the variable declaration is pretty far away from it's use. It can be a little clearer if the variable is right next to it's use point, so for something like this you could do:

long code_policy_status; data.GetLong("CodeIntegrityPolicyEnforcementStatus", code_policy_status); // do something with code_policy_status

muffins · 2019-02-27T21:56:50Z

osquery/tables/system/windows/hvci_status.cpp

+    umcipolicymethods[1] = "AUDIT_MODE";
+    umcipolicymethods[2] = "ENFORCED_MODE";
+
+    if (vbsmethods.find(vbsmethod) != vbsmethods.end()) {


Let's save this value so that you don't grab it twice. In fact you could just make this a ternary:

auto vbs_meth = vbsmethods.find(vbsmethod); r["vbs_status"] = vbs_meth != vbs_methods.end() ? vbs_meth->second : "UNKNOWN";

Here and throughout.

muffins · 2019-02-27T21:57:20Z

osquery/tables/system/windows/hvci_status.cpp

+    r["code_integirty_policy_enforcement_status"] = codepolicystatusmethod_str;
+    r["umci_policy_status"] = umcipolicymethod_str;
+
+    // stuff goes before here


Remove this

Fix typo with one of the umcipolicy, remove VectorOfStrings call. Fix"integrity" spelling issues. Remove map calls to use arrays. Switch to vectors instead of array methods. Add conditional statements to use "unknown" for out-of-range values. Remove extraneous comment.

…into hvci-table-actual

Fix typo with one of the umcipolicy, remove VectorOfStrings call. Fix"integrity" spelling issues. Remove map calls to use arrays. Switch to vectors instead of array methods. Add conditional statements to use "unknown" for out-of-range values. Remove extraneous comment.

…into hvci-table-actual

fmanco

You need to target experimental which will change a but the generation function. Whenever problem provide the valid values on the column description mentioning that UNKNOWN is returned on error. Finally please add tests similar to the other tests we have on the tests/integration directory.

fmanco · 2019-03-19T14:57:18Z

osquery/tables/system/windows/hvci_status.cpp

+namespace osquery {
+namespace tables {
+
+std::vector<std::string> vbs_methods = {"VBS_NOT_ENABLED",


Does it make sense to split this into enabled and running and make them booleans?

For this case we need to track the three distinctive states. It may be possible for VBS to be running, but the configurations may be incorrect, and thus not run. It's also part of the expected status from the Microsoft docs: https://docs.microsoft.com/en-us/windows/security/threat-protection/windows-defender-exploit-guard/enable-virtualization-based-protection-of-code-integrity#virtualizationbasedsecuritystatus

fmanco · 2019-03-19T14:59:24Z

osquery/tables/system/windows/hvci_status.cpp

+    "OFF", "AUDIT_MODE", "ENFORCED_MODE"};
+
+QueryData genHVCIStatus(QueryContext& context) {
+  Row r;


Move inside the loop.

If you're suggesting that I move 13-20 to line 30, I received previous feedback move most of my code outside the loop, since I would be recreating variables in each loop iteration.

#5426 (comment)

Actually, I think I'm gunna say my bad on this one and let's go with @fmanco's suggestion here :) Thinking through this, my concern would be if logic ever changes and we're not updating the values for every column in the row we might get stale data being set. That's my bad, sorry about that!

No worries, I just moved them back into the loop.

…into hvci-table-actual

finnstra · 2019-04-16T23:02:13Z

I've correct my PR to be compatible with Buck, and also included an integration test for the new table.

finnstra · 2019-07-02T15:17:11Z

Friendly bump on this PR :) Thanks!

linux-foundation-easycla · 2019-11-21T20:22:54Z

The committers are authorized under a signed CLA.

✅ Brad Thompson (78f78fe, 0e3f3f5, 909c4ac, 0097458, a32605e, 3387afa, 6f9e2fd, e1dc740, ca12bd6, 1eea3af, 47a8f22)

finnstra · 2020-01-23T18:52:52Z

Hello, could someone please take a look at my PR? Thank you :)

muffins

This lgtm, @nullbrx would you mind fixing the one comment to move the Row r back inside the loop? And we should be good to merge assuming CI goes through ok. Thanks for your patience with us!

muffins · 2020-01-23T19:00:45Z

osquery/tables/system/windows/hvci_status.cpp

+    "OFF", "AUDIT_MODE", "ENFORCED_MODE"};
+
+QueryData genHVCIStatus(QueryContext& context) {
+  Row r;


Actually, I think I'm gunna say my bad on this one and let's go with @fmanco's suggestion here :) Thinking through this, my concern would be if logic ever changes and we're not updating the values for every column in the row we might get stale data being set. That's my bad, sorry about that!

muffins

Requesting changes for the CI build failures and also for the not about moving Row r back into the loop :)

muffins · 2020-01-23T19:03:24Z

specs/BUCK

@@ -745,7 +745,11 @@ osquery_gentable_cxx_library(
            "windows",
        ),
        (
-            "windows/pipes.table",
+            _SPECS_LOCATION + "/windows/hvci_status.table",


Remove the _SPECS_LOCATION bit here, make it look like the ones above, I think CI is failing over this.

muffins · 2020-01-23T19:16:51Z

specs/BUCK

+            "windows",
+        ),        
+        (
+            _SPECS_LOCATION + "/windows/pipes.table",


I'm betting you might need to remove this _SPECS_LOCATION as well.

finnstra · 2020-01-23T21:38:13Z

@muffins I'm affecting the Linux build somehow - let me know if I need to sync some things up :)

Addressing `clang-format` nits, moving the `Row r` into the main loop.

muffins · 2020-01-23T23:43:48Z

@nullbrx I made the changes, looks like it was a clang-format issue, we'll see if they went through alright? If the build passes we should be good to merge :)

stale review.

Commit initial HVCI table and schema.

78f78fe

facebook-github-bot added the cla signed Automated label: Pull Request author has signed the osquery CLA label Feb 8, 2019

Fix typo with one of the umcipolicy, remove VectorOfStrings call.

0e3f3f5

muffins added virtual tables Windows labels Feb 23, 2019

muffins requested changes Feb 27, 2019

View reviewed changes

finnstra added 4 commits March 11, 2019 16:02

Merge branch 'hvci-table-actual' of https://github.com/nullbrx/osquery …

0097458

…into hvci-table-actual

Merge branch 'hvci-table-actual' of https://github.com/nullbrx/osquery …

3387afa

…into hvci-table-actual

finnstra force-pushed the hvci-table-actual branch from 0097458 to adfa790 Compare March 13, 2019 21:37

Create initial HVCI table and prefix.

6f9e2fd

finnstra force-pushed the hvci-table-actual branch from adfa790 to 6f9e2fd Compare March 13, 2019 21:40

fmanco previously requested changes Mar 19, 2019

View reviewed changes

Merge branch 'hvci-table-actual' of https://github.com/nullbrx/osquery …

e1dc740

…into hvci-table-actual

finnstra changed the base branch from master to experimental March 22, 2019 00:33

finnstra added 3 commits March 26, 2019 14:41

Commit initial tests.

ca12bd6

Fix table specs to include UNKNOWN reference

1eea3af

Fix dependency paths for Buck builds.

47a8f22

finnstra closed this Nov 21, 2019

finnstra reopened this Nov 21, 2019

Merge branch 'experimental' into hvci-table-actual

2a5cb9f

Smjert changed the base branch from experimental to master January 23, 2020 18:58

muffins approved these changes Jan 23, 2020

View reviewed changes

muffins requested changes Jan 23, 2020

View reviewed changes

Remove _SPECS_LOCATION mention.

b628833

muffins reviewed Jan 23, 2020

View reviewed changes

finnstra added 3 commits January 23, 2020 14:19

Move VBS and enforcement methods into loop.

328fd8f

Remove additional "_SPEC_" mention

cb024d5

Remove additional slash in pipes table declaration

6feaabf

Update hvci_status.cpp

3140584

Addressing `clang-format` nits, moving the `Row r` into the main loop.

muffins approved these changes Jan 24, 2020

View reviewed changes

muffins merged commit 0b2aa61 into osquery:master Jan 24, 2020

Create HVCI table for Windows Device Guard #5426

Create HVCI table for Windows Device Guard #5426

Conversation

finnstra commented Feb 8, 2019

muffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmanco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finnstra commented Apr 16, 2019

finnstra commented Jul 2, 2019

linux-foundation-easycla bot commented Nov 21, 2019 • edited Loading

finnstra commented Jan 23, 2020

muffins left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finnstra commented Jan 23, 2020

muffins commented Jan 23, 2020

linux-foundation-easycla bot commented Nov 21, 2019 •

edited

Loading

muffins left a comment •

edited

Loading