Docker should use /var/lib/docker/tmp for large temporary files. #6456

rhatdan · 2014-06-16T20:21:44Z

/tmp is often a tmpfs file system and large temporary files could cause
docker commands to fail.

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

tianon · 2014-06-16T20:25:50Z

Oh good call! +1

ewindisch · 2014-06-16T20:46:22Z

I'm okay with this unless anyone has illusions of running Docker on non-Unixy machines, although I'd prefer if we could set this as a runtime flag. The old code allowed setting a TMPDIR environment variable.

timthelion · 2014-06-16T21:16:12Z

Link related issue: #4396

SvenDowideit · 2014-06-16T23:11:29Z

-1 - we have used TMPDIR in boot2docker, and in other setups.

If hardcoding to /var/tmp is considered ok, then it needs to be well documented.

timthelion · 2014-06-17T10:04:35Z

Is it possible to use /var/tmp as default and let TMPDIR over-ride that?

rhatdan · 2014-06-17T12:05:11Z

I think maybe the best fix would be to set TMPDIR=/var/tmp by default iff TMPDIR is not set.

rhatdan · 2014-06-17T12:37:22Z

This patch will basically replace the TempDir patch from OS with one that defaults to /var/tmp.

I am not sure if the is should also be done for the non daemon code path. Should continue to work on non Unix systems.

SvenDowideit · 2014-06-20T13:37:11Z

I wonder if this needs documentation, as i assume that most users will think the default will be /tmp - mmm, and I think I need to look to see if this would change how we setup boot2docker too

rhatdan · 2014-06-23T14:30:34Z

Not sure we document anything about the handling of temp content within docker. At least in the man pages I see no mention of tmp or temp. In a perfect world, I would not want the docker daemon saving this content to /tmp or /var/tmp. It should be saved in a place like /var/lib/docker so that evil users could not attempt to muck around with it.

SvenDowideit · 2014-06-24T00:03:31Z

agreed - thing is, if we are using TMPDIR, we should be consistent with expectations.

I wonder if we can consistently ensure no-one unwittingly uses /tmp or TMPDIR - or ... can we just default to /var/lib/docker/tmp, document that users can over-ride it with TMPDIR and be happy...

rhatdan · 2014-06-24T14:42:16Z

I kind of like defaulting it to /var/lib/docker/tmp and then modifying all of the calls to MKTMP to use it. Not sure how this would effect non unit machines.

unclejack · 2014-06-24T15:24:17Z

@rhatdan What about manually setting TMPDIR for those particular systems which want to use /var/lib/docker/tmp?

It's possible to reconfigure Docker today using TMPDIR to store large files in /var/lib/docker/tmp or /var/tmp.

We can also change the code to default to /var/tmp by setting TMPDIR to that unless it's manually specified.

rhatdan · 2014-06-24T16:38:12Z

Why not set this to /var/lib/docker/tmp and then allow it to be overridden by the user to be in /var/tmp or /tmp. Then make /var/lib/docker/tmp not searchable by anyone but root and docker users

root docker 660

This would guarantee that a non priv user would not be able to attack/steal the image while it is in temporary storage.

timthelion · 2014-06-24T23:02:47Z

I don't really understand why we are discussing the security of /tmp. From my reading of documentation most programming languages provide a secure mktemp function, does golang not do so? Is this broken on weird kernels?

tianon · 2014-06-24T23:10:39Z

Nobody here is discussing the security of /tmp. We're discussing the appropriateness of /tmp for large files, which it's ordinarily not well-suited for (especially since it's often tmpfs and thus in RAM).

timthelion · 2014-06-24T23:13:25Z

@tianon um, to quote @rhatdan "It should be saved in a place like /var/lib/docker so that evil users could not attempt to muck around with it."

rhatdan · 2014-06-25T15:01:04Z

Well I actually believe this is a potential security issue as well as a space issue.

I wrote this blog in 2007 about this.

https://danwalsh.livejournal.com/11467.html

Having a privileged process write to /tmp or /var/tmp is potentially dangerous, mkstemp and friends are attempts of mitigating this danger. They probably will prevent well known guess attempts at the name, but the image could contain content that you might not want to leak to a unpriv user. and for a period of time this content could be stored in a temporary file. Do we make sure of the permissions on this temp content? Such that the user could not read it? I don't know? Are we sure in the future that no programmer will make a mistake on the protection flags. If we make /var/lib/docker/tmp as 660, and then default all tmp functions in docker to use it, there is a much smaller attack surface.

The secondary issue is that these images can be large and we have already told people the images are stored in /var/lib/docker, so I would assume that the temporary content for the images would also fit there. Relying on /tmp or /var/tmp which would probably be on a different partition could be problematic.

timthelion · 2014-06-25T15:20:25Z

@rhatdan You say "If we make /var/lib/docker/tmp as 660, and then default all tmp functions in docker to use it, there is a much smaller attack surface." Does it actually reduce attack surface? It's only advantage is that it reduces the risk that a programmer would forget to set the file mode. If the file mode is set correctly and the mktemp function is implemented correctly(and really, a correctly implemented mktemp function should set the file mode correctly when you are root ;) ) there is no security risk.

However, I agree that large files don't belong in /tmp especially now that everyone with an SSD has to make /tmp into a ramdisk or end up screwing their drive :(

rhatdan · 2014-06-25T16:40:45Z

Yes if Programmers do not create bugs we have nothing to worry about. :^(

SvenDowideit · 2014-06-25T23:42:29Z

I'm +1 to using /var/lib/docker/tmp (really I mean -g-path/tmp) by default, as this sidesteps all sorts of /tmp pain - and our graph dir is already storing all the container's underwear.

/var/lib/docker is already 700 as all access to it is via the docker daemon - i (i think) we don't need to make /var/lib/docker/tmp accessible to group users?

rhatdan · 2014-06-26T13:12:44Z

Updated the patch to use /var/lib/docker/tmp. Also added to utils utils.RootDir to save the setting of the -g command.

SvenDowideit · 2014-06-27T01:32:46Z

@rhatdan can you please push it :)

rhatdan · 2014-06-27T09:43:41Z

@SvenDowideit oops.

Additionally, this can be overridden by setting the TMPDIR variable, like this was already the case for the generic `mkimage.sh` script. As explained in moby#6456, the rationale to use `/var/tmp` instead of `/tmp` is that `/tmp` is often a small tmpfs filesystem with more restricted rights. Docker-DCO-1.1-Signed-off-by: Vincent Bernat <vincent@bernat.im> (github: vincentbernat)

SvenDowideit · 2014-06-30T03:04:43Z

utils/daemon.go

+	RootDir = "/var/lib/docker"
+)
+
+func SetRootDir(tmpdir string) {


What has this got to do with tmpdir? I presume dir string would be more what you mean?

SvenDowideit · 2014-06-30T03:06:47Z

I feel it would be worth mentioning that the tmp dir defaults to /tmp under the --graph dir, and that it can be over-ridden using the TMPDIR env var in the daemon docs in cli.md.

otherwise its one of those hidden secrets only the code readers know.

but on the other hand - maybe they are the only ones that have a good reason?

rhatdan · 2014-07-09T17:12:25Z

I updated to patch to use DOCKER_TMPDIR, is this what you meant?

tianon · 2014-07-09T17:22:34Z

So, I meant something more like:

var tmpDir string
if tmpDir = os.Getenv("DOCKER_TMPDIR"); tmpDir == "" {
    tmpDir = filepath.Join(RootDir, "tmp")
}
os.MkdirAll(...)
return tmpDir

tiborvass · 2014-07-09T19:16:25Z

utils/tmpdir_linux.go

+		rootdir = RootDir
+	}
+	dir := fmt.Sprintf("%s/tmp", rootdir)
+	if _, err := os.Stat(dir); os.IsNotExist(err) {


From godoc: If path is already a directory, MkdirAll does nothing and returns nil.
So no need for the if.

rhatdan · 2014-07-11T13:28:15Z

Renamed to tmpdir_unix.go

tiborvass · 2014-07-11T13:48:58Z

docker/docker.go

@@ -48,7 +48,7 @@ func main() {
 		bridgeName           = flag.String([]string{"b", "-bridge"}, "", "Attach containers to a pre-existing network bridge\nuse 'none' to disable container networking")
 		bridgeIp             = flag.String([]string{"#bip", "-bip"}, "", "Use this CIDR notation address for the network bridge's IP, not compatible with -b")
 		pidfile              = flag.String([]string{"p", "-pidfile"}, "/var/run/docker.pid", "Path to use for daemon PID file")
-		flRoot               = flag.String([]string{"g", "-graph"}, "/var/lib/docker", "Path to use as the root of the Docker runtime")
+		flRoot               = flag.String([]string{"g", "-graph"}, utils.RootDir, "Path to use as the root of the Docker runtime")


I also think it makes more sense to store the RootDir variable not in utils, but in the var section of this file, just like dockerConfDir, unexported so rootDir. Then you could have utils.TempDir take the rootDir as a parameter. It just feels wrong to have the RootDir variable in the utils package. Let me know if you disagree.

ping @rhatdan

rhatdan · 2014-07-16T17:42:06Z

Switched to use @tiborvass method.

unclejack · 2014-07-17T16:06:55Z

LGTM

ping @crosbymichael @tiborvass @vieux

tiborvass · 2014-07-18T23:52:13Z

TestBuldForbiddenContextPath fails: docker_cli_build_test.go:1300: Wrong error, must be about forbidden ../../ path

tiborvass · 2014-07-19T00:13:33Z

#assignee=tiborvass

rhatdan · 2014-07-19T10:02:57Z

@tiborvass What does this mean?

I have rebased.

tiborvass · 2014-07-20T16:02:49Z

@rhatdan rebasing was without conflict so it was mainly about fixing that test case.
We're sometimes carrying PRs to cut the back and forth between reviewers and contributors to save everyone's time. It helps merging faster. So no worries :)

rhatdan · 2014-07-28T18:35:46Z

@tiborvass How does it look now?

tiborvass · 2014-07-28T18:44:01Z

LGTM

tianon · 2014-07-28T19:04:24Z

utils/tmpdir_unix.go

+	if tmpDir = os.Getenv("DOCKER_TMPDIR"); tmpDir == "" {
+		tmpDir = filepath.Join(rootdir, "tmp")
+	}
+	os.MkdirAll(tmpDir, 0700)


Any particular reason we're ignoring any errors from MkdirAll? Would it be prudent to either not do that, or add a comment as to why we're doing that?

SvenDowideit · 2014-07-29T03:47:14Z

Docs LGTM - @jamtur01 @fredlf @ostezer

ghost · 2014-07-29T11:54:15Z

LGTM

jamtur01 · 2014-07-30T17:57:08Z

LGTM

fredlf · 2014-07-31T18:45:06Z

LGTM (that's a docs quorum). ping @vieux @crosbymichael

/tmp is often a tmpfs file system and large temporary files could cause docker commands to fail. Also using /tmp potentially allows users on the system to get access to content, or even attack the content. Moving the tmpdir to /var/lib/container/tmp will protect the data. Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan) Conflicts: docker/docker.go

rhatdan · 2014-08-05T20:46:02Z

Rebased to current docker. Can we get this pull request upstream.

rhatdan · 2014-08-06T16:47:58Z

ping @vieux @crosbymichael

unclejack · 2014-08-06T16:59:54Z

@crosbymichael This PR is good to go. Can you review it, please?

Docker should use /var/lib/docker/tmp for large temporary files.

erikh added the Runtime label Jun 26, 2014

vincentbernat mentioned this pull request Jun 27, 2014

mkimage: use /var/tmp by default instead of /tmp #6729

Merged

rhatdan changed the title ~~Docker should use /var/tmp for large temporary files.~~ Docker should use /var/lib/docker/tmp for large temporary files. Jun 27, 2014

SvenDowideit reviewed Jun 30, 2014
View reviewed changes

tiborvass reviewed Jul 9, 2014
View reviewed changes

rhatdan added the /project/doc label Jul 11, 2014

tiborvass reviewed Jul 11, 2014
View reviewed changes

rhatdan assigned tiborvass Jul 19, 2014

tiborvass mentioned this pull request Jul 19, 2014

Use DOCKER_TMPDIR=/var/lib/docker/tmp for large temp files (rebase/fix of 6456) #7113

Closed

rhatdan unassigned tiborvass Jul 24, 2014

tianon reviewed Jul 28, 2014
View reviewed changes

unclejack added a commit that referenced this pull request Aug 6, 2014

Merge pull request #6456 from rhatdan/tmpdir

66c8f87

Docker should use /var/lib/docker/tmp for large temporary files.

unclejack merged commit 66c8f87 into moby:master Aug 6, 2014

tiborvass mentioned this pull request Aug 6, 2014

Fix tmpdir integration tests #7448

Merged

Docker should use /var/lib/docker/tmp for large temporary files. #6456

Docker should use /var/lib/docker/tmp for large temporary files. #6456

Uh oh!

Conversation

rhatdan commented Jun 16, 2014

Uh oh!

tianon commented Jun 16, 2014

Uh oh!

ewindisch commented Jun 16, 2014

Uh oh!

timthelion commented Jun 16, 2014

Uh oh!

SvenDowideit commented Jun 16, 2014

Uh oh!

timthelion commented Jun 17, 2014

Uh oh!

rhatdan commented Jun 17, 2014

Uh oh!

rhatdan commented Jun 17, 2014

Uh oh!

SvenDowideit commented Jun 20, 2014

Uh oh!

rhatdan commented Jun 23, 2014

Uh oh!

SvenDowideit commented Jun 24, 2014

Uh oh!

rhatdan commented Jun 24, 2014

Uh oh!

unclejack commented Jun 24, 2014

Uh oh!

rhatdan commented Jun 24, 2014

Uh oh!

timthelion commented Jun 24, 2014

Uh oh!

tianon commented Jun 24, 2014

Uh oh!

timthelion commented Jun 24, 2014

Uh oh!

rhatdan commented Jun 25, 2014

Uh oh!

timthelion commented Jun 25, 2014

Uh oh!

rhatdan commented Jun 25, 2014

Uh oh!

SvenDowideit commented Jun 25, 2014

Uh oh!

rhatdan commented Jun 26, 2014

Uh oh!

SvenDowideit commented Jun 27, 2014

Uh oh!

rhatdan commented Jun 27, 2014

Uh oh!

SvenDowideit Jun 30, 2014

Choose a reason for hiding this comment

Uh oh!

rhatdan Jul 9, 2014

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Jun 30, 2014

Uh oh!

rhatdan commented Jul 9, 2014

Uh oh!

tianon commented Jul 9, 2014

Uh oh!

tiborvass Jul 9, 2014

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Jul 11, 2014

Uh oh!

tiborvass Jul 11, 2014

Choose a reason for hiding this comment

Uh oh!

tiborvass Jul 16, 2014

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Jul 16, 2014

Uh oh!

unclejack commented Jul 17, 2014

Uh oh!

tiborvass commented Jul 18, 2014