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

Make use of the O_CLOEXEC flag when dealing with file descriptors #145

Merged
merged 4 commits into from
Apr 13, 2018

Conversation

kbabioch
Copy link
Contributor

This series makes use of the O_CLOEXEC flag whenever opening file descriptor, which makes sure that those file descriptors won't be leaked into child processes, even if a final fclose() is forgotten (as was the case with #136).

@kbabioch kbabioch force-pushed the feat/fdcloexec branch 3 times, most recently from 418dfef to e0b4cf6 Compare April 10, 2018 09:26
This opens any file descriptors with the O_CLOEXEC flag, which will make sure
that file descriptors won't be leaked into any child process. This was
previously an issue due to a forgotten fclose() (Yubico#136).
This adds the `e` flag to fopen() calls, making sure the `O_CLOEXEC` flag is
used. This makes sure that the file descriptor is being closed and not leaked
into child processes. This was an issues previously due to a missing fclose()
(Yubico#136).
This uses mkostemp() instead of mkstemp(), passing along the `O_CLOEXEC` flag,
which makes sure that the file descriptor is closed and won't be leaked into
any child process, which was previously an issue due to a missing fclose()
(Yubico#136).
@klali
Copy link
Member

klali commented Apr 10, 2018

An interesting thing here is that macos silently swallows the 'e' flag but doesn't seem to support it. O_CLOEXEC seems to be supported from 10.12

I'm unsure if it's better to switch to open()+fdopen() because of this to get the same protections for all files and all platforms.. thoughts?

@kbabioch
Copy link
Contributor Author

In my view this is definitely a feature that should be used as often as possible. Given that we already had an issue like this and we have a lot of goto statements, we might always forget to close some resources properly.

Unfortunately I'm not a MacOS guy, so I don't have too much experience in this regard. Since there were no build errors from Travis, I assumed that everything is OK, and didn't give it a second thought.

O_CLOEXEC is definitely supported (apparently since 10.7), but finding definite answers whether and since when fopen() has support for it, is difficult.

I don't see another option, we probably need to work around these issue with open() and fdopen() as proposed by you. I'm happy to re-submit, but I'm cannot test on MacOS and have to rely on Travis, so if you want to tackle this for your own, feel free to do so ;).

By the way: What are the minimum requirements on the MacOS side? Which is the oldest supported versions? Apparently other projects like Python and D-Bus had some issues with this flag, as it is not always available and had to put some conditionals around it. Is this something we need to worry about?

@klali
Copy link
Member

klali commented Apr 10, 2018

I'll answer in reverse order here.

For mac the support has to be latest + 1 (so 10.12 and 10.13).

I agree that it makes sense to have the same behaviour on all platforms, using flags that are unsupported/ignored is a bad practice.
I'd be happy if you add this to the pull request, I'll review and look over the mac parts.

kbabioch added a commit to kbabioch/yubico-pam that referenced this pull request Apr 11, 2018
…g fopen()

A previous commit (d51124e) added the `e` flag to the `fopen()` calls. However
this flag is not supported on all platforms (MacOS) and will be silently
dropped (see Yubico#145). This patch works around those issues by manually opening
the file descriptor using `open()` with the `O_CLOEXEC` flag, and invoking
`fd_open()` on the resulting file descriptor to open an appropriate `FILE`
stream.

This makes sure that all files used by pam_yubico will be opened with the
`O_CLOEXEC` flag on all supported platforms to mitigate issues with missing
`fclose()` invocation (see Yubico#136).
@kbabioch
Copy link
Contributor Author

kbabioch commented Apr 11, 2018

Ok, I've added another commit (e5bd2ef) which should address the function. It is pretty straight forward, but let me know if there are any issues with it.

…g fopen()

A previous commit (d51124e) added the `e` flag to the `fopen()` calls. However
this flag is not supported on all platforms (MacOS) and will be silently
dropped (see Yubico#145). This patch works around those issues by manually opening
the file descriptor using `open()` with the `O_CLOEXEC` flag, and invoking
`fd_open()` on the resulting file descriptor to open an appropriate `FILE`
stream.

This makes sure that all files used by pam_yubico will be opened with the
`O_CLOEXEC` flag on all supported platforms to mitigate issues with missing
`fclose()` invocation (see Yubico#136).
@kbabioch
Copy link
Contributor Author

Unfortunately the commit breaks the coveralls check. I'm not familiar with the test suite and it looks kind of hackery. @klali would you be willing to fix somehow increase the code coverage to make the check happy? Is this even necessary here?

@klali
Copy link
Member

klali commented Apr 13, 2018

This looks good to me.

I see the coverage check as more informative than something blocking us, the project needs more tests (especially for the challenge-response mode) but that isn't something that needs to block this.

merging.

@klali klali merged commit e5bd2ef into Yubico:master Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants