Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix driver image path in drivers table #7444

Merged
merged 14 commits into from
Apr 27, 2022

Conversation

iko1
Copy link
Contributor

@iko1 iko1 commented Jan 12, 2022

This PR fixes the issue in #7262

I fix the actual driver image path by using boost::filesystem::path::join to join between two paths.
The current code doesn't join correctly between two paths, because of that the driver path is partial.

before:

+---------------------+--------------------------------------------------------+---------+
| path                | name                                                   | source  |
+---------------------+--------------------------------------------------------+---------+
| C:\WINDOWS\system32 | PCI Express Root Port                                  | drivers |
| C:\WINDOWS\system32 | PCI Express Root Port                                  | drivers |
| C:\WINDOWS\system32 | Disk drive                                             | drivers |
| C:\WINDOWS\system32 | LSI Adapter, SAS 3000 series, 8-port with 1068         | drivers |
| C:\WINDOWS\system32 | PCI Express Root Port                                  | drivers |
| C:\WINDOWS\system32 | CD-ROM Drive                                           | drivers |
| C:\WINDOWS\system32 | Standard SATA AHCI Controller                          | drivers |
| C:\WINDOWS\system32 | USB Root Hub                                           | drivers |
| C:\WINDOWS\system32 | Standard Enhanced PCI to USB Host Controller           | drivers |

after:

@iko1 iko1 requested review from a team as code owners January 12, 2022 13:22
@directionless directionless added this to the 5.3.0 milestone Jan 18, 2022
@mike-myers-tob mike-myers-tob added the ready for review Pull requests that are ready to be reviewed by a maintainer label Jan 19, 2022
Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iko1 Thanks for this PR.

While this makes the drivers table appear to work correctly, I think we should attempt to fix this at the core.

As far as I can see the problem is the registry table which is used via queryMultipleRegistryPaths, which is not filtering correctly the results it returns via the path constraint:

osquery> select count(key) from registry where path LIKE "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\%\ImagePath";
osquery planner: xBestIndex Evaluating constraints for table: registry [index=0 column=1 term=1 usable=1]
osquery planner: xBestIndex Evaluating constraints for table: registry [index=1 column=1 term=2 usable=1]
osquery planner: xBestIndex Evaluating constraints for table: registry [index=2 column=1 term=3 usable=1]
osquery planner: xBestIndex Adding index constraint for table: registry [column=path arg_index=1 op=65]
osquery planner: xBestIndex Recording constraint set for table: registry [cost=1.000000 size=1 idx=2]
osquery planner: xOpen Opening cursor (2) for table: registry
osquery planner: xFilter Filtering called for table: registry [constraint_count=1 argc=1 idx=2]
osquery planner: xFilter Adding constraint to cursor (2): path LIKE HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\%\ImagePath
osquery planner: Scanning rows for cursor (2)
osquery planner: xFilter registry generate returned row count:6792 <--------------- this should be the same as the count below
osquery planner: Closing cursor (2)
+------------+
| count(key) |
+------------+
| 642        |
+------------+

As you can see the table is generating 6792 rows and then sqlite is filtering them down to 642 when it finally processes the results, and the rows are only the ones with name = "ImagePath", because the path ends with ImagePath.

When using that function instead, sqlite is not involved (as it should be), but this means that there's nothing doing the final filtering.

I think we have two choices here:

  1. Do not use the registry table to retrieve the registry keys (which is a good thing, in general we should extract APIs that could be used by all tables, but that do not use another table to do so, do not use a Row to store the data or involve sqlite for filtering).
    Then on a separate work, fix the registry table.

  2. Fix the registry table filtering and move the decoupling work with separate APIs for later.

@Smjert
Copy link
Member

Smjert commented Jan 29, 2022

I've filed a bug report for the registry table issue meanwhile, to keep it tracked: #7466

@iko1
Copy link
Contributor Author

iko1 commented Jan 29, 2022

@Smjert, Thanks for the explanation. the query planer is super useful.
Due to you recommend to use API call instead of accessing another table(on this case Registry), I choose option (1) to make this PR valuable.

@iko1
Copy link
Contributor Author

iko1 commented Feb 2, 2022

osquery> select image from drivers;
osquery planner: xBestIndex Recording constraint set for table: drivers [cost=1000000.000000 size=0 idx=1]
osquery planner: xOpen Opening cursor (1) for table: drivers
osquery planner: xFilter Filtering called for table: drivers [constraint_count=1 argc=0 idx=1]
osquery planner: Scanning rows for cursor (1)
osquery planner: xFilter drivers generate returned row count:211
osquery planner: Closing cursor (1)

I've changed the code to access directly Windows Registry instead of querying another table to retrieve the image path of a driver.

@iko1 iko1 requested a review from Smjert February 2, 2022 07:13
@iko1 iko1 force-pushed the fix/image-path-drivers-table branch 3 times, most recently from 49845fa to 972a235 Compare February 5, 2022 08:23
@iko1 iko1 force-pushed the fix/image-path-drivers-table branch from 58e8cad to 6ef20c0 Compare February 5, 2022 16:09
Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!
This starts to look cleaner, this should be the last round of changes I think!

@iko1 iko1 requested a review from Smjert February 16, 2022 07:39
@Smjert Smjert added bug and removed ready for review Pull requests that are ready to be reviewed by a maintainer labels Feb 17, 2022
@Smjert Smjert self-assigned this Mar 30, 2022
@Smjert
Copy link
Member

Smjert commented Mar 30, 2022

I'll have a look at this in the weekend, sorry for the delay!

Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @iko1 for moving this forward.
I've better tested this and found additional inconsistencies. After these are fixed we should be hopefully good to go!

EDIT: I also suggest to rebase this PR on latest master, otherwise the ReadTheDocs step will fail.

Co-authored-by: Stefano Bonicatti <smjert@gmail.com>
@iko1 iko1 requested a review from Smjert April 11, 2022 17:04
@iko1
Copy link
Contributor Author

iko1 commented Apr 15, 2022

Thanks @iko1 for moving this forward. I've better tested this and found additional inconsistencies. After these are fixed we should be hopefully good to go!

EDIT: I also suggest to rebase this PR on latest master, otherwise the ReadTheDocs step will fail.

Thanks for your review. I've fixed the inconsistencies. I hope that this PR is good to go now.

Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for updating; final changes, then I'll approve since everything is working.

iko1 and others added 3 commits April 25, 2022 15:26
Co-authored-by: Stefano Bonicatti <smjert@gmail.com>
Co-authored-by: Stefano Bonicatti <smjert@gmail.com>
Co-authored-by: Stefano Bonicatti <smjert@gmail.com>
@mike-myers-tob mike-myers-tob merged commit 3ac4807 into osquery:master Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants