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

Configuration files must not be writeable by other users #3544

Merged
merged 2 commits into from
Feb 7, 2017

Conversation

andrewkroh
Copy link
Member

This PR adds enforcement of ownership and file permissions on configuration files. Any configuration file must be owned by the same user that the Beat is running as and the file must not be writable by anyone other than the owner.

This strict permission checking is limited to platforms with POSIX file permissions. The DACLs used by Windows are not checked at this time.

The check can be disabled on the CLI with -strict.perms=false.

This PR adds enforcement of ownership and file permissions on configuration files. Any configuration file must be owned by the same user that the Beat is running as and the file must not be writable by anyone other than the owner.

This strict permission checking is limited to platforms with POSIX file permissions. The DACLs used by Windows are not checked at this time.

The check can be disabled on the CLI with `-strict.perms=false` or by setting env var `BEAT_STRICT_PERMS=false`.
@andrewkroh
Copy link
Member Author

jenkins, test it

// IsStrictPerms returns true if strict permission checking on config files is
// enabled.
func IsStrictPerms() bool {
if !*flagStrictPerms || os.Getenv("BEAT_STRICT_PERMS") == "false" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably document that there is also an environment variable available for this setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we will need to update the documentation to explain the ownership and permission requirements. This info can go in there.

}

euid := os.Geteuid()
fileUID, _ := info.UID()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ignoring the error here ok? I'm mainly interested in the behaviour on windows. If I understand correctly, fileUID will be -1 in that case, right? Is euid also -1 on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should never get this far because of the first check to see if this is running on Windows.

On Windows FileInfo.UID() will always return an error. And syscall.Geteuid() always returns -1.

@tsg tsg merged commit 32d4285 into elastic:master Feb 7, 2017
@SlavaValAl
Copy link

Guys, do you have plans to make this option configurable via configuration file?

@andrewkroh
Copy link
Member Author

@SlavaValAl No. What are you doing that you need this?

@SlavaValAl
Copy link

@andrewkroh Hi, I'm working on application tool what generates prospectors configurations. I'm using this #3362 feature to get it reloaded automatically. But, my application and filebeat are acting under different users. So, I'm using feature from current pull request, and, I'm prefer to configure this option in filebeat.yml instead of environment variable or CLI option. It's not so critical, but, it would be nice to have it.

@andrewkroh
Copy link
Member Author

Well we can't put the option to disable the check in the config file because that would be self-defeating. A bad actor could drop a config file in disabling the check and do bad things.

I think for your case I would disable the check and lock down the config permissions. The check is there to prevent people from accidentally making themselves vulnerable. I think I would make the conf.d dir writable only by some group that is common to both applications. That should prevent other users from being able to drop an arbitrary config file in and have it be executed.

@andrewkroh andrewkroh deleted the feature/strict-perms branch March 31, 2017 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants