Puppet

The first part of my proposed fix for this issue is now available as a branch in github: http://github.com/daniel-pittman/puppet/commits/feature/5092/

It improves two key areas:

It replaces the hand-written (and insecure) implementation of Tempfile with Tempfile, ensuring among other things that puppet is no longer vulnerable to a TOCTOU write-through-symlink attack if it manages files in any user controlled directory.

(Tempfile uses O_CREAT|O_EXCL which causes that particular attack to fail, where the old code would happily write through a symlink created after it checked that it didn’t exist.)

It also handles file writes nicely in that, ensuring that the data is pushed to disk as safely as possible even if the system uncleanly restarts during a puppet run.

Finally, it removes the window where permissions on the file could be wrong; previously puppet would rename the file into place, then push through the permissions.

Now it sets those correctly before the rename operation, but after the content is written, and ensures that change is persistent. This ensures there is no longer a window where users of that file will see incorrect permissions.

This is not a security vulnerability, per se, but rather a usability issue: the permissions would almost always have been more restrictive rather than less restrictive than they should have been.

The failure mode here would be that an application would be unable to read the configuration file during the race window between property_fix applying the setting and the client application trying to acess the file.

Further work required:

• the property_fix solution is kind of ugly, and could probably do with more cleaning up than it currently has.
• this only fixed the file type/provider, other places in puppet still get this wrong, potentially in very globally risky (and perhaps security critical) ways.

Daniel Pittman wrote:

• this only fixed the file type/provider, other places in puppet still get this wrong, potentially in very globally risky (and perhaps security critical) ways.

Yes, like for instance client of Puppet::Util::FileLocking.writelock (see #4923 for the discussion).

ok, so we didn’t merge the fix for the file type/provider? We haven’t addressed any of it?

Nigel Kersten wrote:

ok, so we didn’t merge the fix for the file type/provider? We haven’t addressed any of it?

No idea, but probably not. I had no great success getting any changes into the product before I started working for the company.