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 permissions on opening pipes for reading in pipes table #7810

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

ExceptionalHandler
Copy link
Contributor

This is in response to the #7809

Fixing the sharing mode parameter of CreateFileW API in genPipes() to open pipes in sharing mode, so that other processes can have read/write/delete access to the pipes when osquery's handles to those pipes are still open.

@mike-myers-tob mike-myers-tob changed the title bugfix/genPipes : Open pipe handles with sared privileges Fix permissions on opening pipes for reading in pipes table Nov 9, 2022
@mike-myers-tob
Copy link
Member

Hi, I've reviewed this and agree with the issue, and thank you for your PR. It is definitely important for osquery not to cause other processes to fail while trying to access pipes just because osquery is enumerating them at that instant.

I don't have approval rights, but I would say it LGTM.

Just one comment, if we are allowing other processes to delete the pipe while we are looking at it (FILE_SHARE_DELETE), this may cause rows in the table where one or more fields get set to -1. The alternative is to prevent pipe deletion by not allowing FILE_SHARE_DELETE but this could hypothetically interfere with another process expecting pipe deletion to succeed.

@Smjert
Copy link
Member

Smjert commented Nov 15, 2022

Closing and reopening for the CI to pick up the fix in master.

@Smjert Smjert closed this Nov 15, 2022
@Smjert Smjert reopened this Nov 15, 2022
@Smjert
Copy link
Member

Smjert commented Nov 15, 2022

Just one comment, if we are allowing other processes to delete the pipe while we are looking at it (FILE_SHARE_DELETE), this may cause rows in the table where one or more fields get set to -1. The alternative is to prevent pipe deletion by not allowing FILE_SHARE_DELETE but this could hypothetically interfere with another process expecting pipe deletion to succeed.

Yeah I would say that if osquery can in any way make an API call fail due to its inspection, that's a no go.

@mike-myers-tob mike-myers-tob merged commit 106bf5f into osquery:master Nov 22, 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.

3 participants