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.

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

archive/tar: malformed input causes panic in parsePAXRecord #40196

Closed
josharian opened this issue Jul 13, 2020 · 14 comments
Closed

archive/tar: malformed input causes panic in parsePAXRecord #40196

josharian opened this issue Jul 13, 2020 · 14 comments
Labels
Milestone

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Jul 13, 2020

Discovered by oss-fuzz, at https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24029. This is a slightly smaller reproducer.

cc @dsnet @dvyukov @FiloSottile

Reproduce:

package main

import (
	"archive/tar"
	"bytes"
)

func main() {
	r := bytes.NewReader(data)
	t := tar.NewReader(r)
	for {
		_, err := t.Next()
		if err != nil {
			return
		}
	}
}

var data = []byte("./PaxHeaders.20745/aaa\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x000000644\x000000000\x000000000\x0000000000132\x0012531145371\x00011420\x00 x\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00ustar\x0000\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x000000000000000000000000000000000030 mtime=1432668921.098285006\n30 ctime=2147483649.151633198\n30 ctime=1432668921.151633198\n\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00")

Result:

$ go run fuzztar.go
panic: runtime error: slice bounds out of range [35:29]

goroutine 1 [running]:
archive/tar.parsePAXRecord(0xc0000200c0, 0x5a, 0x5a, 0x200, 0xc0000200c0, 0x5a, 0x0, 0x0, 0x0, 0x0)
	/Users/josh/go/1.14/src/archive/tar/strconv.go:269 +0x3e6
archive/tar.parsePAX(0x1106a40, 0xc000014480, 0xc0000144a8, 0x0, 0x0)
	/Users/josh/go/1.14/src/archive/tar/reader.go:310 +0x118
archive/tar.(*Reader).next(0xc000014480, 0x10d8500, 0xc00007a101, 0xc000014480)
	/Users/josh/go/1.14/src/archive/tar/reader.go:90 +0x1e2
archive/tar.(*Reader).Next(0xc000014480, 0xc000014480, 0xc000051f78, 0x1006ebf)
	/Users/josh/go/1.14/src/archive/tar/reader.go:52 +0x42
main.main()
	/Users/josh/Desktop/tardelme/fuzztar.go:12 +0x13c
exit status 2

Reproduces with tip, 1.14, and 1.13. I didn't try earlier.

@josharian josharian added this to the Go1.16 milestone Jul 13, 2020
@dsnet
Copy link
Member

@dsnet dsnet commented Jul 13, 2020

Code hasn't been touched in years, so probably been around for a while. Although, I was probably the one who introduced this bug. Ooops!

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 16, 2020

Change https://golang.org/cl/243038 mentions this issue: archive,image: document security expectations

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jul 16, 2020

This was also reported by Chijin Zhou of the Tencent Blade Team to security@golang.org a few hours before OSS-Fuzz found it. We've not been considering panics in archive/... and image/... packages to be security issues, because they can cleanly be recovered by applications that process untrusted inputs, and I've sent a CL to document that.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 4, 2021

We didn't get to work on this for Go1.16, but I'll triage it for Go1.17 as early-in-cycle and if ready, we'll backport it.

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Feb 4, 2021
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 4, 2021

This PAX record is the offensive one "0000000000000000000000000000000030 mtime=1432668921.098285006\n30 ctime=2147483649.15163319"

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 4, 2021

I'll mail something for Go1.16.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 4, 2021

Change https://golang.org/cl/289629 mentions this issue: archive/tar: strip leading 0s in decimal length for a PAX record

@odeke-em odeke-em modified the milestones: Go1.17, Go1.16 Feb 4, 2021
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 9, 2021

@gopherbot please backport this issue as it is a security problem and has existed since for the past 8 years as per https://codereview.appspot.com/6700047.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2021

Backport issue(s) opened: #44182 (for 1.14), #44183 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot closed this in e0ac989 Feb 9, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2021

Change https://golang.org/cl/290649 mentions this issue: [release-branch.go1.14] archive/tar: detect out of bounds accesses in PAX records resulting from padded lengths

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2021

Change https://golang.org/cl/290650 mentions this issue: [release-branch.go1.15] archive/tar: detect out of bounds accesses in PAX records resulting from padded lengths

@josharian
Copy link
Contributor Author

@josharian josharian commented Feb 9, 2021

I'm not sure that this should be backported, per @FiloSottile's comment above ("We've not been considering panics in archive/... and image/... packages to be security issues").

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 9, 2021

I did read his comment, but:

  • this fix fits within the backport policy of things broken in the past 2 releases, and no way for users to migrate, except by getting the future Go1.16
  • the CL updating the security policy stalled
    and that’s why I requested the backports
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 10, 2021

We did not handle this as a security issue, so it should not be backported as a security patch.

The decision on whether to backport it as a critical fix is for the @golang/release team, but if it was introduced 8 years ago it's not a regression, and if no one noticed it doesn't sound like a critical issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants