Fix for gathering python packages from any user Python installations #7414
Conversation
There was a problem hiding this comment.
@iko1 sorry for the delay and thank you for debugging this and fixing it!
May I ask you to also add some tests for the populateSubkeys function here https://github.com/osquery/osquery/blob/master/osquery/tables/system/tests/windows/registry_tests.cpp?
This fix modifies the behavior of the function when the list of keys to check contains only non existing keys, since previously it would return with failure, now it will return with success and depending on the replaceKeys boolean it will return either an empty set or with the same set of input keys.
Can we test that:
- Given an input set of keys with only one key in it, which is known to exists and has some subkeys,
populateSubkeyscalled withreplaceKeysto false returns with success and in the set you have the same input key + some additional keys (no need to check which exactly they are) - Given an input set of keys with only one key in it, which is known to be an invalid key,
populateSubkeyscalled withreplaceKeysto false returns with failure. - Given an input set of keys with 3 keys in it, where the first and the last are known to exists and have subkeys, but the middle one it's known to be invalid,
populateSubkeyscalled withreplaceKeysto false returns with success and the set of keys now contains the input keys + at least one subkey per valid input key
Co-authored-by: Stefano Bonicatti <smjert@gmail.com>
e7dd150
to
c4ade62
Compare
|
Fixed the PR according to @Smjert comments. |
@iko1 Thank you very much for the tests! I don't think we should create a special case for "all the passed keys are not existing", and instead see that as success with an empty return if So to clarify, I would remove the |
It's possible to use gmock to overload the 'queryKey' helper function(what populateSubkeys uses).
I've committed the changes as you requested, please review. |
There was a problem hiding this comment.
Thank you again!
We will address the other API errors separately, because I suspect that it might make more sense to actually "ignore" all the keys that have errors, and do not interrupt the loop, so as to provide the most complete view possible.
But this might cause side effects around that need to be carefully evaluated.
This PR fixes the bug reported at #7350. It adds a check whether 'queryKey' returns major error(e.g. access denied) or path not found.
The "path not found" error should be handled when writing registry globs like "HKEY_USERS\%\SOFTWARE\Python\PythonCore\%\InstallPath" The first glob wildcard is trivial to be found, but the other once depends on whether user installs any Python software within its registry tree.
I tested the fix with installing latest Python version for local-user only. My Python installation is stored under "C:\Users\Amir\AppData\Local\Programs\Python\Python310\python.exe" it's not a system location( e.g. c:\program files) .
After my fix the python packages populate to the table.
any feedback is more than welcome, Thanks!