Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

archive/tar: reader returns bogus headers #12436

Closed
dvyukov opened this issue Sep 1, 2015 · 9 comments
Closed

archive/tar: reader returns bogus headers #12436

dvyukov opened this issue Sep 1, 2015 · 9 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@dvyukov
Copy link
Member

dvyukov commented Sep 1, 2015

The following program crashes:

package main

import (
    "archive/tar"
    "bytes"
    "io"
)

func main() {
    t := tar.NewReader(bytes.NewReader([]byte(data)))
    var headers []*tar.Header
    for {
        hdr, err := t.Next()
        if err == io.EOF {
            break
        }
        if err != nil {
            return
        }
        hdr1 := *hdr // make a copy to be safe
        headers = append(headers, &hdr1)
    }
    buf := new(bytes.Buffer)
    w := tar.NewWriter(buf)
    for _, hdr := range headers {
        err := w.WriteHeader(hdr)
        if err != nil {
            panic(err)
        }
    }
}

var data =  "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01" +
    "\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x0000000000\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\t" +
    "\xbf\x97KǸ\xd4\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
    "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
panic: archive/tar: header field too long

The problem is that reader produces the following header:

tar.Header{Name:"", Mode:9151314442816847872, Uid:0, Gid:0, Size:0, ModTime:time.Time{sec:62135596800, nsec:0, loc:(*time.Location)(0x5d6f40)}, Typeflag:0x0, Linkname:"", Uname:"", Gname:"", Devmajor:0, Devminor:0, AccessTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}, ChangeTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}, Xattrs:map[string]string(nil)}

Mode is bogus. Then, cString tries to serialize that mode in octal format into 8-byte buffer. It does not fit.
I've inserted panic when this error is generated, and there is output:

b="\x00\x00\x00\x00\x00\x00\x00\x00"
s="774000000000000000000"
panic: archive/tar: header field too long

goroutine 1 [running]:
archive/tar.(*Writer).cString(0xc820090480, 0xc820090517, 0x8, 0x19c, 0xc8200ae0e0, 0x15, 0x505f00, 0x0, 0x0, 0x0)
    src/archive/tar/writer.go:83 +0x333
archive/tar.(*Writer).octal(0xc820090480, 0xc820090517, 0x8, 0x19c, 0x7f00000000000000)
    src/archive/tar/writer.go:102 +0xea
archive/tar.(*Writer).writeHeader(0xc820090480, 0xc8200b8f70, 0x517701, 0x0, 0x0)
    src/archive/tar/writer.go:193 +0x651
archive/tar.(*Writer).WriteHeader(0xc820090480, 0xc8200b8f70, 0x0, 0x0)
    src/archive/tar/writer.go:142 +0x3c
main.main()
    /tmp/tar.go:26 +0x336
exit status 2

go version devel +9c514e1 Tue Sep 1 04:00:12 2015 +0000 linux/amd64
@dvyukov dvyukov mentioned this issue Sep 1, 2015
Closed
@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Sep 1, 2015
@dsnet
Copy link
Member

dsnet commented Sep 1, 2015

Is anyone working on fixing this? I'd be interested in tackling this and the other related tar/archive issues: #11168, #11171, #12434, #12435, #12436.

@bradfitz
Copy link
Contributor

bradfitz commented Sep 1, 2015

All yours, @dsnet.

You can send reviews to @dsymonds

@dsnet
Copy link
Member

dsnet commented Sep 1, 2015

Seems like @dadkins is working on it already.

@dadkins
Copy link

dadkins commented Sep 1, 2015

I've only tackled subsecond atime, ctime, and mtime. Don't let me stop you from working on the rest of those issues.

@dsnet
Copy link
Member

dsnet commented Sep 4, 2015

Copy. I'll tackle this, this weekend.

@dsnet
Copy link
Member

dsnet commented Sep 8, 2015

This should be renamed as: "archive/tar: writer does not support writing base-256 fields"

The issue at hand is not that the reader is giving bogus headers, but that the writer isn't able to encode the headers back since it doesn't support the base-256 format that GNU added.

I recommend closing this as "won't fix". Currently, the tar writer already outputs the Pax format when the header information extends beyond what the original tar header was able to encode. Thus, this prevents the use of any other format type, otherwise the output tar file will neither be Pax nor GNU nor BSD; it will be some weird and incompatible mix of two or more formats. It is fortunate (or unfortunate) that the writer already chose Pax as its default extended output format. Thus, it should not use extensions of any other format.

A peruse of the pax code shows that it does not support reading base256 encoded fields, thus proving that it is not safe to mix some of the exotic GNU/BSD extensions into Pax.

@dsymonds If he agrees.

EDIT: I misread the code and it seems that tar does dynamically choose what format to use based on the fields it needs to encode. As I was afraid, this does cause the writer to potentially write conflicting formats if not careful. Issue #9683 is caused by this phenomenon. This will probably be a good time to carefully look through tar and make sure it doesn't conflict with itself.

As a side note, I should mention that the reader incorrectly reads base256 encoded fields in that it does not handle negative numbers. I'll submit a CL for that. For reference:

The use case of negative numbers allows for timestamps before 1970, and also allows Go's tar implementation to properly reject Tar files with negative file sizes and what not.

@dsnet dsnet mentioned this issue Sep 9, 2015
Closed
@gopherbot
Copy link

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

@dsnet dsnet mentioned this issue Sep 16, 2015
Closed
@rsc
Copy link
Contributor

rsc commented Dec 3, 2015

Still not working. Oh well.

@rsc rsc modified the milestones: Unplanned, Go1.6 Dec 3, 2015
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Dec 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@bradfitz @rsc @dadkins @dvyukov @ianlancetaylor @dsnet @gopherbot