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

Windows Event Log support #3887

Merged
merged 16 commits into from Dec 12, 2017

Conversation

Projects
None yet
4 participants
@alessandrogario
Contributor

alessandrogario commented Oct 25, 2017

Testing the new logger plugin

  • Install the Windows Event Log manifest: manage-osqueryd.ps1 -install
  • Insert windows_event_log in the logger plugin section of the osquery configuration file.
  • Open the Windows Event Viewer and select the Facebook/osquery channel under the application logs.

Things to keep in mind

  • The manifest file (osquery.man) contains the full path to the executable image (C:\ProgramData\osquery\osqueryd\osqueryd.exe). This is necessary for the Windows Event Viewer to locate the manifest file inside the PE resources.
  • The osqueryd.exe file is sometimes locked by the OS if the manifest is registered. If you are going to overwrite the file (i.e. recompile) you should uninstall the manifest first (just use manage-osqueryd.ps1 -uninstall).

Any feedback is really appreciated!

@theopolis

This comment has been minimized.

Show comment
Hide comment
@theopolis

theopolis Oct 29, 2017

Contributor

Thanks for separating this out @alessandrogario! Is it possible to move all of the "tooling", aka, the generated files and binary content into a folder within "./tools", like ./tools/wel"?

Contributor

theopolis commented Oct 29, 2017

Thanks for separating this out @alessandrogario! Is it possible to move all of the "tooling", aka, the generated files and binary content into a folder within "./tools", like ./tools/wel"?

@muffins

As @theopolis mentioned, I think the last big component we'd need for this is to get more of the rendered files contained under the tools directory. How about making a new folder under tools called wel, and putting the manifest and rendered .bin files there, and then rename windows_event_log_manifest to be just windows_event_log and keep just implementation details and files in this directory to stay consistent with the other logger plugins.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 27, 2017

@alessandrogario has updated the pull request.

facebook-github-bot commented Nov 27, 2017

@alessandrogario has updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 27, 2017

@alessandrogario has updated the pull request. View: changes

facebook-github-bot commented Nov 27, 2017

@alessandrogario has updated the pull request. View: changes

@alessandrogario

This comment has been minimized.

Show comment
Hide comment
@alessandrogario

alessandrogario Nov 27, 2017

Contributor

I've moved the files in the tools/wel directory, but I still have an issue; the absolute path of the osquery executable must be written inside the manifest (see line 5 in tools/wel/osquery.man) before we compile everything.

Right now, I'm using the installation path for the chocolatey package, but it will not work with the MSI if it is being installed elsewhere.

Contributor

alessandrogario commented Nov 27, 2017

I've moved the files in the tools/wel directory, but I still have an issue; the absolute path of the osquery executable must be written inside the manifest (see line 5 in tools/wel/osquery.man) before we compile everything.

Right now, I'm using the installation path for the chocolatey package, but it will not work with the MSI if it is being installed elsewhere.

@muffins

This comment has been minimized.

Show comment
Hide comment
@muffins

muffins Nov 27, 2017

Contributor

@alessandrogario that sounds fine to me, I'd say let's place an assumption on it being in the C:\ProgramData\osquery\osqueryd\osqueryd.exe location for now.

Contributor

muffins commented Nov 27, 2017

@alessandrogario that sounds fine to me, I'd say let's place an assumption on it being in the C:\ProgramData\osquery\osqueryd\osqueryd.exe location for now.

Dismissing stale review.

@muffins

2 very small nits. I think that we're lookin good here after those changes hit. @theopolis do you have any other comments for this? I'd say we're good to merge.

Show outdated Hide outdated osquery/logger/plugins/windows_event_log.cpp
Show outdated Hide outdated tools/manage-osqueryd.ps1
Code review changes
 > Add missing if brackets
 > Fix the variable and command names in the manage-osqueryd.ps1
   script.
 > Update the documentation.
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 1, 2017

@alessandrogario has updated the pull request. View: changes

facebook-github-bot commented Dec 1, 2017

@alessandrogario has updated the pull request. View: changes

@muffins

muffins approved these changes Dec 4, 2017

@theopolis

This comment has been minimized.

Show comment
Hide comment
@theopolis

theopolis Dec 5, 2017

Contributor

Is there anyway to have unit tests for this code? If we by accident forget to include the .cpp file in our compilation will we be able to proactively find that the logger is no longer an option to use?

Contributor

theopolis commented Dec 5, 2017

Is there anyway to have unit tests for this code? If we by accident forget to include the .cpp file in our compilation will we be able to proactively find that the logger is no longer an option to use?

@alessandrogario

This comment has been minimized.

Show comment
Hide comment
@alessandrogario

alessandrogario Dec 5, 2017

Contributor

Hello Teddy!

I don't think it will compile if you forget to include it; the systemLog() function in include/osquery/logger.h is now wired to Windows Event Log (it wasn't implemented before for Windows).

I'm open to suggestions to improve this!

Contributor

alessandrogario commented Dec 5, 2017

Hello Teddy!

I don't think it will compile if you forget to include it; the systemLog() function in include/osquery/logger.h is now wired to Windows Event Log (it wasn't implemented before for Windows).

I'm open to suggestions to improve this!

@theopolis

This comment has been minimized.

Show comment
Hide comment
@theopolis

theopolis Dec 6, 2017

Contributor

Ok, good to know, I’m still worried about not having tests for it, but we don’t have tests for logging to syslog either unfortunately. :(

Contributor

theopolis commented Dec 6, 2017

Ok, good to know, I’m still worried about not having tests for it, but we don’t have tests for logging to syslog either unfortunately. :(

@muffins

This comment has been minimized.

Show comment
Hide comment
@muffins

muffins Dec 12, 2017

Contributor

I'm going to merge this PR, and we can use #3996 to track building out tests for this, as it'd be nice to have assurances around it's performance. Thanks @alessandrogario!

Contributor

muffins commented Dec 12, 2017

I'm going to merge this PR, and we can use #3996 to track building out tests for this, as it'd be nice to have assurances around it's performance. Thanks @alessandrogario!

@muffins muffins merged commit e859276 into facebook:master Dec 12, 2017

@alessandrogario alessandrogario deleted the trailofbits:alessandro/feature/windows-event-log branch Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment