Fix/startup items parsing #8536
Conversation
zwass
left a comment
There was a problem hiding this comment.
I like the approach but please see my concern with returning empty values.
| windows/windows_eventlog_tests.cpp | ||
| windows/windows_optional_features_tests.cpp | ||
| windows/windows_update_history_tests.cpp | ||
| windows/startup_items_tests.cpp |
| while (!pathExists(path)) { | ||
| path = boost::join(tmp_path, " "); | ||
| tmp_path = std::vector<std::string>{std::begin(args), --end}; | ||
|
|
||
| if (std::begin(args) == end) { | ||
| return false; |
There was a problem hiding this comment.
I can see why you decided to do this, but it does have the side effect of returning an empty string for path if no valid path is detected. I wonder if it's better to just return the entire entry in this case?
There was a problem hiding this comment.
If the path is detected as invalid we skip producing a row entirely. Do you mean you prefer having the Row even if the path is detected as invalid or do you mean something else ? My reasoning was that an invalid path should not produce any row, but I understand that we might actually want to have a Row for invalid paths because it could be a concern.
There was a problem hiding this comment.
Yes I misunderstood but I do think an incomplete/invalid entry still makes sense to return -- since those values are actually configured on the system, osquery should return them (IMO).
There was a problem hiding this comment.
I have just remembered why I decided to not return anything: we cannot guarantee the path in the entry is double quoted when it includes spaces, so we are always going to split it into multiple components (the bug we are trying to fix). i.e. for the entry C:\Program Files\some space\file.exe is going to be split into C:\Program and Files\some and space\file.exe. And as far as I know it is impossible to reliably "understand" when we have a path unless the file exists on the filesystem, which is not the case when the entry is invalid/incomplete as the case we are trying to fix here. It is a catch 22.
Hence the decision of not including the entry in the result set.
There was a problem hiding this comment.
Huh... That behavior sounds horrific.
zwass
left a comment
There was a problem hiding this comment.
Okay thanks for addressing my concerns. I wish there was a better approach here, but I agree we should go ahead with what you did. Thank you!
Fixes 6402
Made
parseStartupPathas a free function rather than private in order to test it.I am not expanding paths because according to my tests they were coming already expanded.
As per comments on the issue report, I left the entries that didn't make much sense in order to keep the same behaviour.