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

Backup directories created in world writable mode #2821

Closed
saahn opened this issue Feb 8, 2020 · 3 comments · Fixed by #2827
Closed

Backup directories created in world writable mode #2821

saahn opened this issue Feb 8, 2020 · 3 comments · Fixed by #2827
Assignees

Comments

@saahn
Copy link

saahn commented Feb 8, 2020

Our security scanner is detecting that backup directories created by fluentd are world writable.
I think they are getting created here:

          FileUtils.mkdir_p(backup_dir) unless Dir.exist?(backup_dir)
          File.open(backup_file, 'ab', system_config.file_permission || 0644) { |f|
            chunk.write_to(f)
          }

Is there any reason why those directories need to be world writable, rather than set to 755?

We are using fluentd version 1.7.0

@ganmacs
Copy link
Member

ganmacs commented Feb 12, 2020

FileUtils.mkdir_p create the directory in 755.

require 'fileutils'

path = './test'
p FileUtils.mkdir_p(path)
p File.stat(path).mode.to_s(8) #=> "40755"

BTW, please follow the issue template and https://github.com/fluent/fluentd/blob/master/CONTRIBUTING.md

@ganmacs ganmacs closed this as completed Feb 12, 2020
@saahn
Copy link
Author

saahn commented Feb 15, 2020

I tried to reproduce the above with the same ruby used by td-agent on our hosts, and I'm seeing inconsistencies in the default behavior of FileUtils.mkdir_p from what you posted above:

$ cat test.rb
require 'fileutils'

safe_plugin_id = 'some_plugin_id'
backup_base_dir = '/tmp'
p backup_base_dir
backup_file = File.join(backup_base_dir, 'backup', "foo_id", safe_plugin_id, "bar.log")
p backup_file
backup_dir = File.dirname(backup_file)
p backup_dir

FileUtils.mkdir_p(backup_dir) unless Dir.exist?(backup_dir)
$ rm -rf /tmp/backup/
$ /opt/td-agent/embedded/bin/ruby test.rb
"/tmp"
"/tmp/backup/foo_id/some_plugin_id/bar.log"
"/tmp/backup/foo_id/some_plugin_id"
$ ls -lah /tmp/ |  grep backup
drwx------     3 root      root      4.0K Feb 15 01:59 backup
$ ls -lah /tmp/backup/
total 628K
drwx------     3 root root 4.0K Feb 15 01:59 .
drwxrwxrwt 18770 root root 616K Feb 15 02:00 ..
drwx------     3 root root 4.0K Feb 15 01:59 foo_id
$ ls -lah /tmp/backup/foo_id/
total 12K
drwx------ 3 root root 4.0K Feb 15 01:59 .
drwx------ 3 root root 4.0K Feb 15 01:59 ..
drwx------ 2 root root 4.0K Feb 15 01:59 some_plugin_id

Granted, the 0700 permission is also fine, but the inconsistency in the results point at a potentially deeper issue that may be causing the problematic world-writable directories created by the td-agent on our hosts:

$ ls -lah /tmp/ |  grep fluent
drwxrwxrwx     3 root      root      4.0K Feb 14 09:01 fluent
$ ls -lah /tmp/fluent/
total 628K
drwxrwxrwx     3 root root 4.0K Feb 14 09:01 .
drwxrwxrwt 18770 root root 616K Feb 15 02:06 ..
drwxrwxrwx     3 root root 4.0K Oct  7 23:09 backup
$ ls -lah /tmp/fluent/backup/
total 12K
drwxrwxrwx 3 root root 4.0K Oct  7 23:09 .
drwxrwxrwx 3 root root 4.0K Feb 14 09:01 ..
drwxrwxrwx 3 root root 4.0K Oct  7 23:09 worker0

@ganmacs
Copy link
Member

ganmacs commented Feb 17, 2020

https://apidock.com/ruby/Dir/mkdir/class says that the mode is determined by umask(2) and specified mode like mode & !umask. So I wouldn't say that it has inconsistency but it's fair that it's a little confusing and there is a possibility of creating world-writable directories.
I'll reopen and specify the mode to avoid it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants