archive/tar: needs hardening and refactoring

[dsnet]

I'm filing this issue as the "capture all" ticket for all currently known archive/tar issues. I've discovered quite a few issues and it's getting unwieldy filing an issue for them.

Over time, the tar library has become increasingly buggy, especially with the inclusion of support for PAX, GNU, and sparse files. There are many edge conditions where these various formats interact in unexpected ways leading to bugs. The fact that the fuzzer has detected a large of bugs in archive/tar goes to show its fragility. Also, most of the "fixes" done to resolve each issue were bandaids on a larger problem.

I have gone through the Reader implementation several times now and have read parts of the GNU, BSD, and STAR codebases to get a good understanding of how all the different utilities handle all the oddities when it comes to the TAR format.

Some of the major issues with the library are:

• archive/tar: package understanding of GNU format is wrong #12594: the package's understanding of GNU format is wrong.
• Example. Malicious tar files can cause arbitrary memory allocation due to blind trust of user provided sizes.
• Example. Malicious tar files can cause arbitrary infinite loops. This occurs because the current data fragment is only popped when data has been read. However, if the reader experiences an io.EOF before the claimed end of the data fragment, then the EOF will be masked as nil and it will make no progress and readers will loop forever.
• Example. Malicious tar files can cause arbitrary panics. The position and offset and never verified to be non-negative. Thus, bytesLeft can be computed to be negative.

Currently open tickets are:

• archive/tar: package understanding of GNU format is wrong #12594: package understanding of GNU format is wrong
• archive/tar: call to Next does not detect that stream is truncated #12557: call to Next does not detect that stream is truncated
• archive/tar: reader returns bogus headers #12436: reader returns bogus headers
• archive/tar: invalid memory address or nil pointer dereference (2) #12435: invalid memory address or nil pointer dereference
• archive/tar: output is nondeterministic #12358: output is nondeterministic
• archive/tar: Writer does not support sub-second time resolution #11171: Writer incorrectly encodes header data (Writer does not support sub-second resolution timestamps)
• archive/tar: do not use ustar long filename encoding and binary number encoding together #9683: do not use ustar long filename encoding and binary number encoding together (fix by avoid GNU format altogether)
• archive/tar: Writer permits "/" FileHeader rejected by Reader #9647: Writer permits "/" FileHeader rejected by Reader

The plan is to refactor the Reader code first to be as reliable as possible, and then to tackle various out-standing Writer issues.

I'll be filing no more archive/tar related issues after this. I promise :]

[gopherbot]

CL https://golang.org/cl/14624 mentions this issue.

[dsnet]

@dsymonds, what is the point of the Writer.Flush method?

At first, I thought that the function was used to ignore the rest of the current file and automatically fill it in with zeros. In fact, the for loop on L57 seems to indicate that this was the original purpose. However, the check on L51 prevents use of the method unless all file contents have been written already. Is the purpose of this method really just to flush out the final padding? If that's the case, why can't Writer.Write just do that when nb hits zero?

[dsymonds]

You're looking at some pretty ancient code. I wrote the original archive/tar in mid 2009, before we even really had a good grasp of what good Go code looked like. It's accreted changes ever since as people have had need. It probably needs a good tidy up.

I managed to find an email from the code review where Writer.Flush got added. @rsc said:

Line 51: func (tw *Writer) writePadding() {
rename to Flush and return os.Error.
clients may want to call this explicitly
to finish one file even if they're not
ready to call Close or WriteHeader.

In its original form it did indeed write out zeros to the end of the current file. It seems a little bogus now, but we can't drop it.

The neutering on line 51 does indeed look silly. It was added in d75abb7 (early 2012), and @rsc noticed that it was silly after it was submitted (https://codereview.appspot.com/5777064). I can't remember why we didn't revise it back then.

With the benefit of hindsight, I can't see the use case for writing the zeros automatically. The user of tar.Writer needs to know the size in advance (to put it in the header), and can easily write zeros if they want. The 2012 change made Flush only write the trailing padding anyway, so we could simplify it now, but it's also somewhat of a historical artifact at this point.

[gopherbot]

CL https://golang.org/cl/14723 mentions this issue.

[gopherbot]

CL https://golang.org/cl/14823 mentions this issue.

[dsymonds]

Sorry, I don't have time to review all these changes. I'll finish off the one I started, but you'll need to track down a different reviewer.

[dsnet]

Is there anyone you recommend for doing the reviews?

[dsymonds]

@davecheney might be interested, or you can ask around on golang-dev.

[dsnet]

@davecheney, would you be willing to do this? Otherwise, I will ask around.

[rsc]

Please file separate, focused issues for bugs you find. One giant issue does not help anyone. It doesn't give a crisp statement of something to do, and it doesn't help when you see it in a CL description.