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
Add UserAssist table #5539
Add UserAssist table #5539
Conversation
Is there anything I need to add or change for this to get reviewed/merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak to the implementation, but I think this should have tests. Possible a sample file for them.
|
Also, let's rebase against |
Rebased and target branch changed |
Could it be possible to add a more comprehensive test about the table functionality? Unfortunately the "integration" test that's already present it really only serve the purpose of exposing major issues with the table spec and/or "communication" between the different modules, but it doesn't tell if the table is really working as expected and showing correct values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this table, this is gunna be super nifty to have! I had a couple of notes, but overall it's looking good. Just a few small changes and I think we'll be good to land.
unsigned long long last_run = | ||
tryTo<unsigned long long>(last_run_string, 16).takeOr(0ull); | ||
if (last_run == 0ull) { | ||
LOG(WARNING) << "Failed to convert FILETIME to UNIX time."; | ||
return std::string(); | ||
} | ||
last_run = (last_run / 10000000) - 11644473600; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've already got a helper function for converting a FILETIME
to a unix timestamp here, mind using that? You'll also need to import <osquery/filesystem/fileops.h>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to helper function
|
||
// split reg path by \Count\ to get Key values | ||
std::size_t count_key = subkey.find("Count\\"); | ||
std::string value_key = subkey.substr(count_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you declare std::string
and std::size_t
and others quite a bit, could these be auto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to auto
if (time_str == "") { | ||
r["count"] = ""; | ||
} else { | ||
r["count"] = INTEGER(count); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like you could bring the function call on line 168 to get the count
value inside of your else
condition. If the time_str == ""
, then we should avoid making the call to execution_num
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved function
thanks for the review, changes should be applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm, I think we're good to merge. I'll leave this for a bit so others have a chance to look it over, but I think we're alright. Thanks for your work on this!
auto value_key = subkey.substr(count_key); | ||
auto value_key_reg = value_key.substr(6, std::string::npos); | ||
|
||
std::string decoded_value_key = rotDecode(value_key_reg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
N.B.: UserAssist
aren't guaranteed to be ROT-13 encoded. If NoEncrypt
is a DWORD
of value 1
under the UserAssist
registry key, the values are saved in plain text.
I'm not sure how common that is, though, so it might just be worth adding as a NOTE
or TODO
for the future in case someone runs into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats true if NoEncrypt is added to the Userassist key ROT13 would be turned off (though i tried adding that DWORD to my Windows 10 VM and it was still ROT13 encoding the values, but i may have doing something wrong). I think even if ROT13 is turned off, the existing values would still be encoded. So it would be easy to check to see if the NoEncrypt is set to 1. But I think it would be difficult to determine which data is ROT13 encoded and which is not?
Regardless ive never seen that added to any of the Windows systems ive looked at.
I can add NOTE/comment though mentioning the NoEncrypt setting
This PR adds a UsserAssist table for Windows systems.
The UserAssist Registry key tracks of what applications were executed on the system.
https://www.magnetforensics.com/blog/artifact-profile-userassist/
https://www.hecfblog.com/2013/08/daily-blog-45-understanding-artifacts.html
Let me know if additional edits need to be made