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

Jump to bottom

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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@josharian
Copy link
Contributor

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 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!

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 14, 2020
@gopherbot
Copy link

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

@FiloSottile
Copy link
Contributor

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 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 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 commented Feb 4, 2021

I'll mail something for Go1.16.

@gopherbot
Copy link

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 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.

This was referenced Feb 9, 2021
Closed
Closed
@gopherbot
Copy link

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
Copy link

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

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 9, 2021
@josharian
Copy link
Contributor Author

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 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

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.

@golang golang locked and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
6 participants
@josharian @FiloSottile @dmitshur @odeke-em @dsnet @gopherbot