More secure file permissions in case of system-wide C/R configuration

[kbabioch]

This issue is about file permissions in case of a system-wide challenge-response configuration (as opposed to a system-wide mapping file as discussed in #147). This is described, for instance, in Authentication_Using_Challenge-Response.adoc. The documentation suggests to do the following for setting up this directory:

yubico-pam/doc/Authentication_Using_Challenge-Response.adoc

Lines 71 to 83 in 0d61b26

	From security perspective, it is generally a good idea to move the challenge file in a system-wide path that is only read- and writable by root. To do this do as follow:
	----
	$ sudo mkdir /var/yubico
	$ sudo chown root.root /var/yubico
	$ sudo chmod 700 /var/yubico
	$ ykpamcfg -2 -v
	...
	Stored initial challenge and expected response in '$HOME/.yubico/challenge-123456'.
	$ sudo mv ~/.yubico/challenge-123456 /var/yubico/alice-123456
	$ sudo chown root.root /var/yubico/alice-123456
	$ sudo chmod 600 /var/yubico/alice-123456
	----

However it is up to the system administrator to follow these steps and making sure the configuration is correct. It is never checked for and/or enforced in the code.

The code is only doing some very basic sanity checking in regard to this file:

yubico-pam/pam_yubico.c

Lines 544 to 554 in e5bd2ef

	if (fstat(fd, &st) < 0) {
	DBG ("Cannot stat file: %s (%s)", userfile, strerror(errno));
	close(fd);
	goto restpriv_out;
	}
	if (!S_ISREG(st.st_mode)) {
	DBG ("%s is not a regular file", userfile);
	close(fd);
	goto restpriv_out;
	}

There are no file ownership and/or permission checks in place, so this file could be group and/or world-writable, leading to insecure setups.

Later on, when a new file is created to store the new challenge, the previous ownership and permissions are applied:

yubico-pam/pam_yubico.c

Lines 657 to 670 in e5bd2ef

	fd = mkostemp(tmpfile, O_CLOEXEC);
	if (fd < 0) {
	DBG ("Cannot open file: %s (%s)", tmpfile, strerror(errno));
	goto restpriv_out;
	}
	if (fchmod (fd, st.st_mode) != 0) {
	DBG ("could not set correct file permissions");
	goto restpriv_out;
	}
	if (fchown (fd, st.st_uid, st.st_gid) != 0) {
	DBG ("could not set correct file ownership");
	goto restpriv_out;
	}

This makes sure that any insecure permissions will be propagated and the system administrator is never made aware of the insecure setup.

At the very least, there should be some sort of warning, when the ownership and permissions are wrong. This could be implemented as warning in debug mode or, as suggested by the PAM module writer's guide, using syslog(3) with LOG_AUTHPRIV as facility and LOG_ERR as level might be appropriate. Another possibility might be to inform the user about this using the functionality provided by pam_conv(3), although this might make an user aware of the security issue in the first place.

Personally, I would prefer to simply refuse to work in case of wrong permissions and ownership, just like OpenSSH does when the authorized_keys file is setup wrong, since it is inherently dangerous when the file permissions are set up wrongly.

I'm happy to provide appropriate pull requests, but wanted to give this some discussion, since I don't know how aggressive we want to be about this and which approach we should take.

[klali]

Hey,

The level of strictness to apply with this is a tricky question.

One way to see it is to start with a warning that this is going to become more strict and if you rely on this behaviour you should open an issue ASAP.

I think it makes sense to write this to both debug log and syslog, for syslog I'd like us to use the pam_syslog()/openpam_log() API, should be possible to fill in the correct one with a macro depending on what we detect with autoconf.

[klali]

Having said that about syslog() above I see we have current calls to normal syslog(), guess that should be fixed but that doesn't have to be a part of this effort.