tar-split does not read block by block nor validate tar headers

[n4ss]

Problem

https://github.com/moby/moby/blob/master/vendor/github.com/vbatts/tar-split/tar/asm/disassemble.go#L124

This line can read any number of \0s at the end of an archive, potentially taking up all the space in RAM.

We actually read in memory the complete padding sequence.

Reproductible

To reproduce, compress a high amount of 0s and push&pull as an image.

Solution

We should:

• validate the integrity of tar headers
• either refuse large padding sequences or read block by block and write on disk above a certain limit.

/cc @thaJeztah @vdemeester @stevvooe

[fntlnz]

Reproducible with

fallocate -l 2G test.tar
docker import test.tar

In my case the import command completely freezed the host and had to restart

[stevvooe]

@fntlnz That is actually a completely different case than was considered here. I suspect that might be a different path; same result. :(

Looks like this is hitting the same path. Oops.

Thanks for the great report!

[fntlnz]

@steevvoe if we fix the tar thing in this case also all the others should benefit right? IDK if I’m missing something but the point seems to be: “we have to avoid the speculations on malicious images crafted to disturb the docker daemon”

[stevvooe]

@fntlnz Looking back your case, this is actually the same code path. I confused "import" with "load", which properly errors out (we checked!).

[fntlnz]

@stevvooe I was a bit confused 🌈

[runcom]

/cc @vbatts

[cyphar]

@vbatts I was looking at this code, and it's not clear to me whether there's a trivial way of solving it. The core problem is that the encoding for the tar-split.json packer requires having the slice in memory -- which obviously doesn't work if it's 20GB. One solution would be to do it in chunks, but the current Packer API doesn't support that. We could break the previous tar-split code to make it always chunk -- but I'd want to know what @vbatts has to say about that and whether it will break anything.

I'm going to open a bug against tar-split.

[vbatts]

interesting way to OOM. This pad should only ever be 1024, but spec requires reading to the end.
I reckon we could read that remainder to a tempfile, but that seems unfortunate to do for every disassemble.

[vbatts]

oh right. @cyphar is right about the slice. That would be marshaled in memory. Let's see...

[cyphar]

I have proposed a patch for tar-split in vbatts/tar-split#42, and a tracking issue for this bug in vbatts/tar-split#41.

[vbatts]

fixed released in https://github.com/vbatts/tar-split/releases/tag/v0.10.2
@cyphar is working up a PR for moby now.
Thanks for the report!

[thaJeztah]

Thanks both!

[cyphar]

For reference, CVE-2017-14992 was assigned for this bug.