Remove assumptions about temp file locations #509

doublerebel · 2016-01-12T22:01:03Z

Many configuration files live in a parent directory that is only writeable by root or another user. Despite this, current code assumes the parent directory is writable by the consul-template process.

In addition, the assumption is made that the temporary file is created on the same filesystem.

We can still retain an atomic copy+delete using the existing copyFile(...) and deferred os.Remove(...).

This patch allows for more granular security and more variation in the host filesystem. In my case, this patch allows consul-template to run as a dedicated user, with explicit permission to writeable locations.

sethvargo · 2016-01-12T22:08:15Z

Hi @doublerebel

The reason we write to the parent directory is because Consul Template may be writing secret data. Writing secrets into a world-readable directory like /tmp is generally a bad practice.

copyFile is not a public API on the os function.

doublerebel · 2016-01-12T22:20:27Z

@sethvargo I'd be glad to change it if necessary, though from what I can tell consul-template isn't guaranteed to have any writable directories.

ioutil.TempFile automatically creates files as 0600, and does check for existence of the file. Is there another attack vector for that method?

sethvargo · 2016-02-10T11:52:02Z

Hi @doublerebel

Sorry for the delayed reply here. The implication here is actually the same reason the build is failing. In order for the file write to be atomic, all file permissions must be properly set before the file is moved into place. As you can see from the build failure, your solution creates the file in TempDir, but then it is chmoded on 843 to match the value of the existing file.

Let's say the existing template is 644, but it's in a directory that is 700. The result is that the file is momentarily stored in /tmp with permissions of 644 before being moved into the more secure directory.

So the obvious logical followup is "why don't we chmod the file after we move it". This is problematic because then the write operation is no longer atomic and there exists a race condition where an application could attempt to read the file before the permissions have properly changed, or (even worse), the permissions fail to update and now you're left in a bad state.

This is the reason we write into the same directory as the original file, because it avoids all the issues.

Does that make sense?

doublerebel · 2016-02-12T01:06:52Z

Hi @sethvargo

Thanks, you are right to point out that the rendered file is vulnerable between when perms are set and the file is moved from the tempDir. To solve this issue, I have wrapped the temporary file in a temporary subdir. The subdir is only readable by consul-template, and the file perms can be safely set before copying. I looked for a precedent for this technique and found it recommended by Apple's security document under "Create Temporary Files Correctly".

As for the failed tests, I discovered the cause was tempFile perms being set to 0000. The mock template configs are not passed through the parseConfig funcs and therefore never have default permissions set.

Now all tests are passing, but please let me know if I have missed anything.

sethvargo · 2016-02-12T03:24:20Z

runner.go

@@ -822,11 +822,16 @@ func atomicWrite(path string, contents []byte, perms os.FileMode, backup bool) e
 		}
 	}

-	f, err := ioutil.TempFile(parent, "")
+	d, err := ioutil.TempDir(os.TempDir(), "")


I think it's more idiomatic to use "" here instead of os.TempDir, which will default to tmpdir anyway

I agree, I almost did that -- but I grepped the codebase and found that os.TempDir is used with the instances of ioutil.TempFile. Easy change either way.

Fair enough 😄

sethvargo · 2016-02-12T03:25:11Z

Hi @doublerebel

Thanks for clearing that up. I'd like to get @jefferai and @slackpad to weigh in and make sure we aren't opening a vuln here.

slackpad · 2016-02-13T01:23:26Z

runner.go

@@ -854,7 +859,7 @@ func atomicWrite(path string, contents []byte, perms os.FileMode, backup bool) e
 		}
 	}

-	if err := os.Rename(f.Name(), path); err != nil {


This seems like it may introduce some non-atomicity when swapping the file since this does an open, and then an io.Copy from Go, whereas a rename is usually an atomic thing. If the service was watching for changes to this file it seems like it may see some intermediate state. I'm not sure how we'd fix this in a general way unless consul-template gets a scratch dir to work in under the parent, but that's ugly.

The only places default perms are set is in config.ParseConfig(...) and config.ParseConfigTemplate. These code paths are never reached in the runner tests, and if perms are not defined, they are interpreted to be 0000. This causes os.Open(...) to fail after perms are set.

doublerebel · 2016-02-16T00:36:03Z

@slackpad thanks for working through this with me, I see where you're coming from and I've learned a lot today.

While not all OS targets' syscalls default to an atomic rename, golang now basically guarantees it and with go 1.6 provides atomic rename on Windows.

We want to retain an atomic rename, so that there can be no race condition or intermediate state between versions of a rendered template.
In order to enable an atomic rename, we need to create a temporary file on the same filesystem as the target file.
Due to mounts, consul-template can't guarantee a temporary file on the same filesystem as the target file. The current best guess is to use the parent directory.
The user can provide a temporary file that is guaranteed to be on the same filesystem as the parent directory.
Therefore temporary files should be created in a predictable location for the user.
To avoid race conditions, locking, errors, and for security, temporary filenames should be randomized.
By creating a working directory in the parent directory of the file, we can satisfy all the above conditions.

This has the side effect of keeping temporary files clean and understandable to the user. I noticed during testing that failures in consul-template lead to orphaned temporary files in the template target directories. Since the filenames were completely random I had to examine the contents to discover at which file the failure occurred. By adding the filename prefix in addition to the working directory, errors and problems are more descriptive and easier to debug.

This solution allows me to create a .ctmpl working directory that is writable by consul-template in any directory to where I need templates written.

slackpad · 2016-02-16T17:31:57Z

@doublerebel your analysis makes sense. I think with this scheme you'd still have to have the parent directory writable in order to create the temporary directory in there, right?

doublerebel · 2016-02-16T17:46:08Z

@slackpad Yes, this defaults to the existing behavior of requiring a writable parent directory. But to solve my case, the .ctmpl directory can be created manually with the correct permissions and consul-template will just adopt that .ctmpl without needing a writable parent.

slackpad · 2016-03-01T20:35:43Z

Hi @doublerebel I think this is looking good. Just had a couple thoughts:

I think the behavior should be "if we see .ctmpl use that, otherwise do what we did before". This way we won't drop a new .ctmpl directory wherever we run consul-template.
We should add a note to the README about this feature.
Can you add a unit test that puts this directory in place and makes sure things work as expected?

Any thoughts on that first one?

sethvargo · 2016-03-27T19:29:44Z

Ping @doublerebel

doublerebel · 2016-03-27T21:17:44Z

Thanks @sethvargo . @slackpad I can support 1) only using .ctmpl if it exists. I'll work on 2) and 3) and update this PR.

sethvargo · 2016-05-03T14:54:53Z

Ping @doublerebel

sethvargo · 2016-07-13T01:35:21Z

Hi @doublerebel

Thank you for opening this PR, but we haven't heard back from you in some time. I'm going to close this out. If you'd still like to see this, please open a new PR. Thanks and sorry! 😄

doublerebel force-pushed the patch-1 branch from 4b371a7 to 6523b07 Compare January 12, 2016 22:08

doublerebel force-pushed the patch-1 branch from 6523b07 to ab9edb0 Compare February 12, 2016 00:57

sethvargo reviewed Feb 12, 2016
View reviewed changes

slackpad reviewed Feb 13, 2016
View reviewed changes

doublerebel added 2 commits February 13, 2016 12:49

manage temp files: add prefix and use working directory

4a4940d

doublerebel force-pushed the patch-1 branch from ab9edb0 to 4a4940d Compare February 16, 2016 00:14

sethvargo closed this Jul 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove assumptions about temp file locations #509

Remove assumptions about temp file locations #509

doublerebel commented Jan 12, 2016

sethvargo commented Jan 12, 2016

doublerebel commented Jan 12, 2016

sethvargo commented Feb 10, 2016

doublerebel commented Feb 12, 2016

sethvargo Feb 12, 2016

doublerebel Feb 12, 2016

sethvargo Feb 12, 2016

sethvargo commented Feb 12, 2016

slackpad Feb 13, 2016

doublerebel commented Feb 16, 2016

slackpad commented Feb 16, 2016

doublerebel commented Feb 16, 2016

slackpad commented Mar 1, 2016

sethvargo commented Mar 27, 2016

doublerebel commented Mar 27, 2016

sethvargo commented May 3, 2016

sethvargo commented Jul 13, 2016

Remove assumptions about temp file locations #509

Remove assumptions about temp file locations #509

Conversation

doublerebel commented Jan 12, 2016

sethvargo commented Jan 12, 2016

doublerebel commented Jan 12, 2016

sethvargo commented Feb 10, 2016

doublerebel commented Feb 12, 2016

sethvargo Feb 12, 2016

Choose a reason for hiding this comment

doublerebel Feb 12, 2016

Choose a reason for hiding this comment

sethvargo Feb 12, 2016

Choose a reason for hiding this comment

sethvargo commented Feb 12, 2016

slackpad Feb 13, 2016

Choose a reason for hiding this comment

doublerebel commented Feb 16, 2016

slackpad commented Feb 16, 2016

doublerebel commented Feb 16, 2016

slackpad commented Mar 1, 2016

sethvargo commented Mar 27, 2016

doublerebel commented Mar 27, 2016

sethvargo commented May 3, 2016

sethvargo commented Jul 13, 2016