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.

Sign up for GitHub

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

Jump to bottom

Eliminate removal of nonblocking flag for "special" files #7530

Merged
merged 2 commits into from
Apr 29, 2022
Merged

Eliminate removal of nonblocking flag for "special" files #7530

merged 2 commits into from
Apr 29, 2022

Conversation

josephsweeney
Copy link
Contributor

Currently, when opening or reading a named pipe (or fifo), osquery will block. This is a potential avenue for a denial of service attack.

After some investigation, I believe the comment here is incorrect.

// A special file cannot be read in non-blocking mode, reopen in blocking
// mode

According to The Linux Programming Interface Section 5.9

Nonblocking mode can be used with devices (e.g., terminals and pseudoterminals), pipes, FIFOs, and sockets. (Because file descriptors for pipes and sockets are not obtained using open(), we must enable this flag using the fcntl() F_SETFL operation described in Section 5.3.)
O_NONBLOCK is generally ignored for regular files, because the kernel buffer cache ensures that I/O on regular files does not block, as described in Section 13.1. How- ever, O_NONBLOCK does have an effect for regular files when mandatory file locking is employed (Section 55.4).

Given this, the best solution seemed to be to get rid of the check for fd->isSpecialFile() and the non-blocking mode will handle not blocking and for fifo's it will just return an error that we can handle.

A test is also added to make sure this change behaves properly on Windows as well.

@josephsweeney josephsweeney requested review from a team as code owners March 24, 2022 14:30
@mike-myers-tob mike-myers-tob added bug ready for review Pull requests that are ready to be reviewed by a maintainer performance core and removed ready for review Pull requests that are ready to be reviewed by a maintainer labels Mar 24, 2022
@directionless
Copy link
Member

According to The Linux Programming Interface Section 5.9

Do you think that's true for the other platforms?

@mike-myers-tob mike-myers-tob added this to the 5.3.0 milestone Apr 14, 2022
@Smjert
Copy link
Member

Smjert commented Apr 14, 2022 •
edited

Something I just noticed is that the non blocking branch in the readFile function does not check for read_max while reading, to stop reading if the API user has not specified an amount of bytes (size, which happens in most of the readFile use cases) to be read and the file has size 0 (as special files do).

We should add that check.

@josephsweeney
Copy link
Contributor Author

The status of this PR is that even after rebasing to master, the tests are still failing on Debug linux builds. I am currently looking into this, but it is confusing as it does build in release. Does anyone have an idea what is different in debug linux builds?

@directionless Based on testing on each platform and reading the documentation for each platform, I believe the same holds. I am still investigating the debug linux build though so maybe I am wrong.

@Smjert I agree, and I will address these issues in this PR.

Not sure this will be done for 5.3 but I will continue with updates here as I get a better sense.

@Smjert
Copy link
Member

Smjert commented Apr 14, 2022

The status of this PR is that even after rebasing to master, the tests are still failing on Debug linux builds. I am currently looking into this, but it is confusing as it does build in release. Does anyone have an idea what is different in debug linux builds?

The build itself is working, it's the tests that runs in the Debug build that are not.
There could be many reasons:

  1. Debug builds enable asserts on third party libraries
  2. Debug builds executables might be slower, so anything that is time sensitive could be affected
  3. Optimizations are off in Debug builds; this can sometimes surface issues that were hidden in Release mode due to a different memory layout, or could actually hide them. Also various undefined behaviors might behave differently (and be hidden in Release mode, or vice versa).

But in general, there isn't nothing particular in osquery debug builds.

@josephsweeney
Eliminate removal of nonblocking flag for "special" files
328fcfd 
On Linux (also macos and Windows) special files *can* be read in non-blocking
mode. A test is added to ensure that opening and reading a special file behaves
as expected.
@josephsweeney
Copy link
Contributor Author

After much toying around, the simplest fix seemed to be to just have special files use the blocking_io path when reading. This ensures the existing code that reads from special files doesn't have any behavior change.

Even after working with this code, I am unsure why the separate paths for blocking and non_blocking exist for reading files? I even tried on linux, and just deleting the if/else and always using the blocking_io path has all the tests failing. What am I missing here?

@mike-myers-tob mike-myers-tob added the ready for review Pull requests that are ready to be reviewed by a maintainer label Apr 20, 2022
@josephsweeney josephsweeney mentioned this pull request Apr 22, 2022
Open
@mike-myers-tob mike-myers-tob merged commit 176faea into osquery:master Apr 29, 2022
@mike-myers-tob mike-myers-tob deleted the joesweeney/nonblocking-open branch April 29, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Smjert Smjert Smjert approved these changes

Assignees
No one assigned
Labels
bug core performance ready for review Pull requests that are ready to be reviewed by a maintainer
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@josephsweeney @directionless @Smjert @mike-myers-tob