windows/certificates: Fix enumeration bugs, add columns #5631

offlinemark · 2019-07-08T21:22:51Z

This PR addresses a few bugs in the certificates table on Windows (which was somewhat broken), and also implements some additional functionality (new columns). This is the first of two PRs, the second of which improves the table's completeness with respect to the Personal certificate store.

Bugs Fixed

The store enumeration API (certEnumSystemStoreCallback) returned False during error handling, but this prematurely ends the enumeration of cert store locations and cert stores. The callback has been adjusted to always return true, which ensures complete enumeration. (https://docs.microsoft.com/en-us/windows/desktop/api/wincrypt/nc-wincrypt-pfn_cert_enum_system_store#return-value)
The CertOpenSystemStore API (https://docs.microsoft.com/en-us/windows/desktop/api/wincrypt/nf-wincrypt-certopensystemstorea) was used for opening system stores, however this is not the correct API, since it only opens stores in the Current User store location. The CertOpenStore API is now used, which is the correct functionality for opening arbitrary stores in arbitrary locations.
The table previously included deduplication functionality which actually resulted in certificates being hidden. If a certificate was found in multiple users' stores, it would only appear as one entry due to this deduplication. Deduplication has been removed.

Improvements Made

Fixing bug 1. reveals bugs in the handling of the const void* systemStore parameter that certEnumSystemStoreCallback receives. Specifically, that systemStore parameter could also contain information about the service or user store location that the store was in (previously, the code expected something like My, and now that parameter could contain something like S-1-5-18\My). Handling this required adding more parsing logic (parseSystemStoreString) and refactoring to use the additional provided information.
Various new columns have been added to the table to make using it more ergonomic for Windows. Previously, a variety of data was concatenated and put in the path column. That functionality has been preserved, but that information (username, SID, store location, store) has been broken out into individual columns so it can be queried for.

Testing Notes

Verbose debug logging has been added to show the certificate store enumeration process
select common_name, path, sid, username, store_location from certificates; is a good starting query
Unit tests have been added for one small string parsing routine, however, much of the table is untested. I took a look at restructuring the code to create a layer between the table and the system APIs and it was very difficult due to the way the system APIs use (nested) callback functions extensively.

Questions

The table currently does an internal SQL query to map a service's name to its SID. It caches these lookups, however the cache assumes that services will not restart under a different user while osquery is running. Is this too dangerous of an assumption?
Some files contain comments at the top with a Facebook copyright notice, since they were copied from other files. Should that be removed, and what should go in place of them, if anything?

Smjert · 2019-07-08T22:25:54Z

osquery/tables/system/tests/CMakeLists.txt

@@ -34,6 +34,10 @@ function(osqueryTablesSystemTestsMain)
    generateOsqueryTablesSystemTestsStartupitemstestsTest()
    generateOsqueryTablesSystemTestsSignaturetestsTest()
  endif()
+
+  if(DEFINED PLATFORM_WINDOWS)
+    generateOsqueryTablesSystemTestsCertificatestestsTest()


This name collides with an already existing function in the macOS case.
You might want to rename both and add the platform, like generateOsqueryTablesSystemTestsWindowsCertificatestestsTest and generateOsqueryTablesSystemTestsmacOSCertificatestestsTest

Smjert · 2019-07-08T22:31:04Z

Formatting the code is required (as shown by the Linux CI job), if you add the clang-format root folder in the PATH, something like C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\Common7\IDE\VC\vcpackages depending on what Visual Studio version have you installed, then you can run it.
Then you have to format each commit, as separate changes (unless you want to do the format at the end), but you need to have the files in the git stage area, or in the working tree, but not committed, by building the format target with cmake --build . --config <config> --target format.

packetzero · 2019-07-09T15:28:09Z

osquery/filesystem/windows/fileops.cpp

-    return Status(-1, "Invalid SID");
-  }
-
+Status getCurrentUserInfo(PTOKEN_USER& info) {


The getCurrentUserInfo() and isUserCurrentUser() are only used by tables. I wish there were separate libraries for table support routines like this, as it makes the core harder and harder to separate and whittle down. It appears that both of these routines are not used outside the certificates table, so perhaps it can be contained in there for now?

I somewhat disagree.

I think there's value in having some core table helper functions about. These sound like great candidates to me. (Certainly we have equivalents in our go tables)

@packetzero I see what you're saying- In this particular case, I feel like although the certificates table is the only caller, these functions are general enough that it makes sense to keep them in a general library of routines available to windows tables, including future ones, so I tend to agree with @directionless here.

yeah, not a huge deal. I'm all for common routines, we need to start thinking about the monolith that is osquery and how to separate core from tables at some point. When builds have stabilized, we can think about moving table-specific helpers into libs separate from core.

Fixes a bug where certificates for services that correspond to Local Service or Network Service may not have their sids appear correctly. This is because the services table is inconsistent with its user_account column.

Previously, the service name cache existed for the lifetime of the osquery process, which made it susceptible to stale reads if a service restarted under a different user during osquery's lifetime. Now the cache is created for each query. Also refactor it to directly map to the sid, rather than the account name, which removes the need to translate from account name to sid every row.

Previously, getCurrentUserInfo took a reference to data from a local vector, whose data is free'd after the function. This refactors the code to use a unique_ptr (similar to how getSidFromUsername) does it.

alessandrogario · 2019-07-16T15:28:27Z

Thanks for the review @packetzero! 😄

Smjert reviewed Jul 8, 2019

View reviewed changes

packetzero reviewed Jul 9, 2019

View reviewed changes

offlinemark added 9 commits July 9, 2019 12:39

init

d1b8210

Use case insensitive comparisons for all service names

e2b5898

Fixes a bug where certificates for services that correspond to Local Service or Network Service may not have their sids appear correctly. This is because the services table is inconsistent with its user_account column.

Fix comment

7256561

Don't use implicit conversion

b747412

Add tests

319134a

Add forgotten tests files

5af9023

Add ctest

8ec6350

Add fixme comment

08329d5

PR-Review: Deduplicate cmake targets

fca2e16

offlinemark force-pushed the mark/feature/win-certs-1-initial branch from 5bd26a6 to fca2e16 Compare July 9, 2019 22:16

packetzero previously approved these changes Jul 10, 2019

View reviewed changes

Correct licenses

db63328

offlinemark dismissed packetzero’s stale review via db63328 July 11, 2019 01:55

offlinemark added 5 commits July 11, 2019 13:27

Update tests with new prototype

5f6ca69

Fix reference to destroyed object

29e95e3

Previously, getCurrentUserInfo took a reference to data from a local vector, whose data is free'd after the function. This refactors the code to use a unique_ptr (similar to how getSidFromUsername) does it.

Add buck changes

f7d417a

Use single tests target

703bdea

alessandrogario approved these changes Jul 15, 2019

View reviewed changes

packetzero approved these changes Jul 16, 2019

View reviewed changes

alessandrogario merged commit a60b940 into osquery:master Jul 16, 2019

offlinemark mentioned this pull request Jul 16, 2019

windows/certificates: Improve table's coverage of Personal certificates #5640

Merged

offlinemark deleted the mark/feature/win-certs-1-initial branch July 23, 2019 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows/certificates: Fix enumeration bugs, add columns #5631

windows/certificates: Fix enumeration bugs, add columns #5631

offlinemark commented Jul 8, 2019 •

edited

Loading

Smjert Jul 8, 2019

Smjert commented Jul 8, 2019 •

edited

Loading

packetzero Jul 9, 2019

directionless Jul 9, 2019

offlinemark Jul 10, 2019

packetzero Jul 10, 2019

alessandrogario commented Jul 16, 2019

windows/certificates: Fix enumeration bugs, add columns #5631

windows/certificates: Fix enumeration bugs, add columns #5631

Conversation

offlinemark commented Jul 8, 2019 • edited Loading

Smjert Jul 8, 2019

Choose a reason for hiding this comment

Smjert commented Jul 8, 2019 • edited Loading

packetzero Jul 9, 2019

Choose a reason for hiding this comment

directionless Jul 9, 2019

Choose a reason for hiding this comment

offlinemark Jul 10, 2019

Choose a reason for hiding this comment

packetzero Jul 10, 2019

Choose a reason for hiding this comment

alessandrogario commented Jul 16, 2019

offlinemark commented Jul 8, 2019 •

edited

Loading

Smjert commented Jul 8, 2019 •

edited

Loading