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 insecure write to PID file #2174

Merged

Conversation

@juergenhoetzel
Copy link
Contributor

@juergenhoetzel juergenhoetzel commented May 2, 2016

Ensure PID file doesn't exist and don't follow symlinks.

See: https://en.wikipedia.org/wiki/Symlink_race

Ensure PID file doesn't exist and don't follow symlinks.
@joschi
Copy link
Contributor

@joschi joschi commented May 3, 2016

@juergenhoetzel Thank you for your contribution to Graylog!

Before we can accept any code from you, we need you to sign our Contributor License Agreement. Please send us a signed copy to hello@graylog.com so we can merge your pull request.

This is only required once, no matter how many contributions you make in the future. Thank you for helping us with this unfortunate administrative requirement!

@@ -157,7 +159,7 @@ protected void savePidFile(final String pidFile) {
throw new Exception("Could not determine PID.");
}

Files.write(pidFilePath, pid.getBytes(StandardCharsets.UTF_8));
Files.write(pidFilePath, pid.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW, LinkOption.NOFOLLOW_LINKS);

This comment has been minimized.

@joschi

joschi May 3, 2016
Contributor

Wouldn't it make sense to use CREATE and TRUNCATE_EXISTING instead of CREATE_NEW in this place? If the file already exists and CREATE_NEW is being used, the call would fail instead of overwriting the specified file.

This comment has been minimized.

@juergenhoetzel

juergenhoetzel May 3, 2016
Author Contributor

Wouldn't it make sense to use CREATE and TRUNCATE_EXISTING instead of CREATE_NEW in this place? If the file already exists and CREATE_NEW is being used, the call would fail instead of overwriting the specified file.

This is exactly the purpose for this change.

An attacker will place a symlink like

ln -s /PATH/TO/OVERWRITE_USING_GRAYLOG_USER  /tmp/graylog.pid

in the public /tmp folder.

In this case the startup should fail.

This comment has been minimized.

@joschi

joschi May 4, 2016
Contributor

Makes sense. Thanks!

@joschi joschi removed the needs-input label May 4, 2016
@joschi joschi added this to the 2.0.1 milestone May 4, 2016
@joschi
Copy link
Contributor

@joschi joschi commented May 4, 2016

LGTM. 👍

@joschi joschi merged commit 74ef2aa into Graylog2:master May 4, 2016
3 checks passed
3 checks passed
@garybot2
ci-server-integration Jenkins build graylog2-server-integration-pr 878 has succeeded
Details
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 366 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
joschi pushed a commit that referenced this pull request May 4, 2016
Ensure PID file doesn't exist and don't follow symlinks.
(cherry picked from commit 74ef2aa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants