Log permissions are hard-coded to 0600

[jarppiko]

Summary

crowdsec-firewall-bouncer does not provide admins options to configure log files access permissions, but log file permissions are hard-coded to 0600.

crowdsec-firewall-bouncer does not respect process umask either which prevents admins to use systemd's UMask= option. The underlying lumberjack log roller library actually respects existing log file permissions, but crowdsec-firewall bouncer disables this functionality. The current behavior resets the log file permissions to 0600 in every restart.

While hard-coded 0600 log file permission is secure, it also prevents admins to change the log file permissions if needed. If other tools (e.g. promtail) requiring access to the log files run in rootless mode (a good security practice), the tools cannot read Crowdsec bouncer log files.

Technical details

It seems this "feature" was created as a response to upstream issue natefinch/lumberjack#82, but this issue has been fixed more than 4 years ago.

The code responsible for setting the access rights resides in LoggerForFile() in pkg/cfg/logging.go. It calls logtools.fileperms.SetLogFilePermissions() to hard-reset log file permissions to 0600. Otherwise SetLogFilePermissions() seems a copy-paste from lumberjack.

See pkg/logtools/fileperms.go in crowdsecurity/go-cs-lib:

// SetLogFilePermissions sets the permissions of the log file to 0600.
// If the file does not exist, it will be created.
// lumberjack will then respect our permissions.
// https://github.com/natefinch/lumberjack/issues/82
func SetLogFilePermissions(logDir string, logFile string) (string, error) {
	err := os.MkdirAll(logDir, 0755)
	if err != nil {
		return "", fmt.Errorf("failed to create log directory: %w", err)
	}

	logPath := filepath.Join(logDir, logFile)

	st, err := os.Stat(logPath)
	if err != nil {
		if !os.IsNotExist(err) {
			return "", fmt.Errorf("failed to check file existence: %w", err)
		}
		file, err := os.OpenFile(logPath, os.O_CREATE|os.O_WRONLY, 0600)
		if err != nil {
			return "", fmt.Errorf("failed to create file: %w", err)
		}
		file.Close()
		return logPath, nil
	}

	if st.IsDir() {
		return "", fmt.Errorf("expected a file, found a directory: %s", logPath)
	}

	err = os.Chmod(logPath, 0600)                        // <<<< Hard-reseting file perms to 0600
	if err != nil {
		return "", fmt.Errorf("failed to change file permissions: %w", err)
	}

	return logPath, nil
}

Proposal

Proposal is to remove disabling of lumberjack's built-in behavior and thus to give admins option to change log file permissions if needed. I will submit a PR shortly.

As an added-bonus 🏅, the whole logtools/fileperms.go in crowdsecurity/go-cs-lib could be removed if the change was implemented to the three other bouncers using the same function 😃

[mmetc]

Thanks @jarppiko for the analysis. Here I may have missed something because I tried to generalize a behavior that we implemented on some, but not all packages.

Are you saying this: https://github.com/crowdsecurity/crowdsec/blob/master/pkg/types/utils.go#L43

is not required anymore?
I am asking because you said the issue has been fixed 4 years ago, and CrowdSec did not exist back then.

If you mean we can set umask in the systemd service, I'm not sure we want to do it for every file that is created.
Or we can always add a configuration setting.

what do you think @buixor

[jarppiko]

Hi @mmetc ,

How lumberjack works is that it creates a new file (and the required directory tree) by itself during the first Logger.write() call. So there is no need to create the log file by an application, just to give filename. This can be seen see in lumberjack.go lines 146-150:

func (l *Logger) Write(p []byte) (n int, err error) {
	l.mu.Lock()
	defer l.mu.Unlock()

	writeLen := int64(len(p))
	if writeLen > l.max() {
		return 0, fmt.Errorf(
			"write length %d exceeds maximum file size %d", writeLen, l.max(),
		)
	}

       // ----------------------- beef is here @jarppiko -------------
	if l.file == nil {
		if err = l.openExistingOrNew(len(p)); err != nil {
			return 0, err
		}
	}
        // -------------------------------------------------------------

	if l.size+writeLen > l.max() {
		if err := l.rotate(); err != nil {
			return 0, err
		}
	}

	n, err = l.file.Write(p)
	l.size += int64(n)

	return n, err
}

By default, lumberjack sets file permissions to 0600 :

1. Logger.write() calls openExistingOrNew()
2. openExistingOrNew() calls openNew() if the file does not exists
3. openNew() creates dir tree if needed (0755 ❗) and the file (0600)

[jarppiko]

Hello @mmetc,

My comments:

Are you saying this: https://github.com/crowdsecurity/crowdsec/blob/master/pkg/types/utils.go#L43

is not required anymore?

The file you referred to relates to crowdsec, not cs-firewall-bouncer. But the same comment applies - I do not think the file creation needed there (lines 49-52) since lumberjack will create the file in the first write if it does not exist. However, otherwise the function mentioned just sets the default parameters for (crowdsec) log file and thus is required still.

If you mean we can set umask in the systemd service, I'm not sure we want to do it for every file that is created.
Or we can always add a configuration setting.

I was more thinking aloud options how to provide configurability of log file permissions to admins. These are the options, IMHO:

1. Crowdsec config files In this case crowdsec modules (crowdsec, bouncers) need to create the log file first with set permissions and lumberjack will retain those.
2. Allow lumberjack to preserve existing file permissions (PR Remove hard-coded 0600 log file perms #294). This option is simpler and does not require a config file option. It just should be documented somewhere (other than source code) 😉
3. Rely on process umask. Allow the underlying Unix OS to apply to process-specific umask. This however, would require tweaking lumberjack since it applies to hard-coded 0600 permissions to a new file created. In log file rotation it preserves the old file's permissions.

Out of these only options 1 and 2 make sense, IMO.

[mmetc]

Thanks, closing as fixed

I was confused at first because I was sure I had replicated the issue, but it was before updating the lumberjack dependency