Making `tar-split/archive/tar` private breaks users

[mtrmac]

The just-released 0.12.0 , in #79, made tar-split/archive/tar private.

The callback to asm.IterateHeaders requires the caller to refer to tar-split/archive/tar.Header. (The callback we use, I think, doesn’t need any special feature of tar-split/archive/tar, but the implementation of asm.IterateHeaders does depend on .ExpectedPadding; so IterateHeaders would need to tediously convert a tar-split/archive/tar.Header into a standard-library archive/tar.Header if it wanted to remove the former from the API.

More directly, both containers/storage and containerd/stargz-snapshotter call tar-split/archive/tar.Reader.RawBytes.

[vbatts]

oh shit. Thanks for the quick report. Either we: - revert and release - this would be transparent and likely the way to go. This also means I should go through and cherry-pick more from upstream archive/tar into our carried version. - or expose some variable of this in a different variable/path? That would still be a change required for all downstreams and would take some investigation. What's your recommendation, Mirek?

[mtrmac]

This also means I should go through and cherry-pick more from upstream archive/tar into our carried version.

I think that should, ideally, happen ~either way — whether or not the the code can be called from outside tar-split, the parser is run on untrusted data and hypothetical vulnerabilities are reachable. Making it private would only significantly help if some of the features could be outright removed.

---

or expose some variable of this in a different variable/path? That
would still be a change required for all downstreams and would take
some investigation.

TLDR: I think it makes most sense to only revert and keep tar-split/archive/tar public.

---

The mechanics of it: Moving the API in a single release, still breaking compatibility, would be a bit of a hassle to coordinate across the 2 (more?) consumers that are both included in Podman, but manageable. With a transition period where both is available for a few months, sure, why not; c/storage could migrate ~immediately and then we wait for others.

---

OTOH the content: For the “seekable layer” producers like zstd:chunked, we need:

• NewReader(input)
• reader.NextTarHeader() -> (parsed? , raw non-data bytes)
• reader.FileContentReader()

And the existing archive/tar subpackage does not really provide much else (sure, there is ExpectedPadding) — so I don’t see that redefining the reader API can make things any simpler.

On the write path — does that have anything special on top of the standard library? I can see that estargz calls tar-split/archive/tar.NewWriter; I guess the primary attraction is that it can get a tar.Header out of tar-split/archive/tar.Reader and submit it into a tar-split/archive/tar.Writer without having to do any kind of copying/conversion. That’s … not nothing. Also compare the asm.IterateHeader concern — it might be interesting to provide a tar-split/archive.tar.Header.ToStandardLibraryHeader, then Writer could be dropped, and IterateHeaders could provide the standard-library type.

Or maybe there are other ways to carve this, e.g. to only use the standard-library tar.Header struct for all purposes, and copy/duplicate only tar-split/archive/tar.Reader. I didn’t check whether that is possible with the current implementation.

---

Ultimately, cost/benefit:

The net effect of all of the above: Not too much, but some work, and the hypothetical benefit would be that security fixes for the tar.Writer code would not need to be reviewed/duplicated in this repo. Is that enough of a benefit? I think it’s unlikely, but that’s a guess; I didn’t carefully analyze past vulnerabilities of archive/tar.

[vbatts]

fixed in #82

[vbatts]

@mtrmac thanks for that walk through. I'll revert and patch the known CVE portion of the archive/tar.Writer as well (#83).
Also, this will be a chore to attempt to stay on top of such CVE's that upstream uncovers. Ideal state would be to get the diff of the "Accounting" changes and propose upstream, so that the carried version goes away, and tag a v1.0.

[mtrmac]

@vbatts Thanks for dealing with this so quickly.

Ideally, various vendors should also be looking for other copies of vulnerable code. Looking at my notes, I did start the process for this one back then — I’m afraid I didn’t follow up to ensure it got dealt with.