archive/zip: Reader.Open panics on empty string #48085
Comments
|
Lines 800 to 804 in d384ebd It seems to be a bug, I'll try to fix it. |
|
Change https://golang.org/cl/346349 mentions this issue: |
|
does this qualify for backporting to the current releases? (happy to assist with that effort) |
cherrymui
added
the
NeedsInvestigation
|
Also having the same issue/symptoms, code and zip (actually JAR) file attached. Of note when printing the file names from Detailspackage main
import (
"archive/zip"
"io"
"os"
"path"
)
const destination = "./extracted"
func main() {
reader, err := zip.OpenReader("liquibase-core-4.4.3-sources.zip")
if err != nil {
panic(err)
}
defer reader.Close()
// names := []string{}
// for _, file := range reader.File {
// names = append(names, "'"+file.Name+"'")
// }
// fmt.Println(strings.Join(names, "\n"))
for _, file := range reader.File {
outputPath := path.Join(destination, file.Name)
err := copyZipFileEntry(reader, file, outputPath)
if err != nil {
panic(err)
}
}
}
func copyZipFileEntry(reader *zip.ReadCloser, entry *zip.File, outputPath string) (err error) {
inputFile, err := reader.Open(entry.Name)
if err != nil {
return err
}
defer func() {
err1 := inputFile.Close()
if err == nil {
err = err1
}
}()
if err = os.MkdirAll(path.Dir(outputPath), 0700); err != nil {
return err
}
outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
if err != nil {
return err
}
defer func() {
err1 := outputFile.Close()
if err == nil {
err = err1
}
}()
_, err = io.Copy(outputFile, inputFile)
return err
}liquibase-core-4.4.3-sources.zip
|
|
Our stacktrace is slightly different, but ultimately still hits the panic in |
|
Screenshot showing debugger state when calling |
|
per the spec, what you provided is an invalid zip file:
As such, I think it is totally valid for go to reject archives with input path names that are absolute (either it looks like Info-ZIP helps out during but it refuses to create one: |
I very strongly disagree with this. This was a cause of |
|
I agree that this panic seems accidental, and we could probably return an error on |
I don't believe that this case is comparable, nor that the scenario is particularly "absurd" when real world files exist with this characteristic. There is a precedent from the above linked CVEs (most particularly the second one) that the panic from absolute paths should be addressed, as well as a precedent from other utilities that handle this particular case. I completely understand that the case wouldn't have been thought of while the code was written, I lay no blame on anyone for that, Im simply hoping for the scenario to be handled in such a way that it is still possible for me to consume the zip file and do with the erroneous paths as I wish (in this case, we would skip extracting them). Of additional note, the issue is not triggered when I do Regardless of our thoughts, I was strongly recommended to notify the security team of this issue, which I have 🙂 I appreciate your thoughts nonetheless |
|
Thank you for reporting this to security@golang.org. Invalid input should not cause programs to panic, if the input could be attacker controlled. If this required a call to We'll backport this as a security fix in the PUBLIC track. @gopherbot, please open backport issues. |
|
Backport issue(s) opened: #48251 (for 1.16), #48252 (for 1.17). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
|
I found that the key reason for this problem was the function In fact, attacker can easily construct such names. And the example @mengzhuo submited the CL346349 solved the bug of because when the name is |
|
Ha, good catch, I will abandon my CL. Thanks for the info. |
|
@Jason7602 I believe I had tried that already and it did not fully solve the issue; iterating over reader.Files still surfaces the |
|
Change https://golang.org/cl/349770 mentions this issue: |
Actually, my CL349770 has added check both in |
Just so I know we're on the same page, when iterating over If that's the case, should At the same time I'm not sure if we should be erroring on accessing "legitimate" files. "But the files are there! Why am I getting 'invalid argument' from Go but not from e.g. |
|
When the zip file exist absolute path entry, in function Actually the comment of // Open opens the named file in the ZIP archive,
// using the semantics of fs.FS.Open:
// paths are always slash separated, with no
// leading / or ../ elements.
func (r *Reader) Open(name string) (fs.File, error) {So to better distinguish the two error cases, return the |
|
Ping from the release team -- this is listed as a potential cherrypick but hasn't moved in a while. Any updates? |
|
Actually, this is a very serious security bug of |
|
Note that neither Joe nor Brad work at Google anymore, so while they're the right owners to ping, in terms of security you probably want @golang/security. |
|
I've completely context switched any knowledge of |
|
No worries, we'll take this and provide a fix in advance of the next minor release. |
Actually, I had uploaded a CL349770 that might fix this issue, could you take a look? |
katiehockman
added
Security
NeedsFix
|
@colin-sitehost would you like to be credited in the release announcement for reporting this? If so, how/with what name/affiliation? |
|
yeah, happy for myself and my employer to be credited: Colin Arnott, SiteHost; let me know if odher formatting questions come up |
|
Change https://golang.org/cl/360859 mentions this issue: |
|
Change https://golang.org/cl/360858 mentions this issue: |
Previously, opening a zip with (*Reader).Open could result in a panic if the zip contained a file whose name was exclusively made up of slash characters or ".." path elements. Open could also panic if passed the empty string directly as an argument. Now, any files in the zip whose name could not be made valid for fs.FS.Open will be skipped, and no longer added to the fs.FS file list, although they are still accessible through (*Reader).File. Note that it was already the case that a file could be accessible from (*Reader).Open with a name different from the one in (*Reader).File, as the former is the cleaned name, while the latter is the original one. Finally, made the actual panic site robust as a defense-in-depth measure. Fixes CVE-2021-41772 Fixes #48252 Updates #48085 Co-authored-by: Filippo Valsorda <filippo@golang.org> Change-Id: I6271a3f2892e7746f52e213b8eba9a1bba974678 Reviewed-on: https://go-review.googlesource.com/c/go/+/349770 Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Katie Hockman <katie@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org> Trust: Katie Hockman <katie@golang.org> Trust: Julie Qiu <julie@golang.org> (cherry picked from commit b246873) Reviewed-on: https://go-review.googlesource.com/c/go/+/360859 Trust: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org>
Previously, opening a zip with (*Reader).Open could result in a panic if the zip contained a file whose name was exclusively made up of slash characters or ".." path elements. Open could also panic if passed the empty string directly as an argument. Now, any files in the zip whose name could not be made valid for fs.FS.Open will be skipped, and no longer added to the fs.FS file list, although they are still accessible through (*Reader).File. Note that it was already the case that a file could be accessible from (*Reader).Open with a name different from the one in (*Reader).File, as the former is the cleaned name, while the latter is the original one. Finally, made the actual panic site robust as a defense-in-depth measure. Fixes CVE-2021-41772 Fixes #48251 Updates #48085 Co-authored-by: Filippo Valsorda <filippo@golang.org> Change-Id: I6271a3f2892e7746f52e213b8eba9a1bba974678 Reviewed-on: https://go-review.googlesource.com/c/go/+/349770 Run-TryBot: Filippo Valsorda <filippo@golang.org> Reviewed-by: Katie Hockman <katie@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org> Trust: Katie Hockman <katie@golang.org> Trust: Julie Qiu <julie@golang.org> (cherry picked from commit b246873) Reviewed-on: https://go-review.googlesource.com/c/go/+/360858 Trust: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org>
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
https://play.golang.org/p/7x5dvzJSoCY
What did you expect to see?
What did you see instead?
Anything else?
This was discovered via
io/fs.WalkDir, but the root cause isReader.Open: https://play.golang.org/p/WWHxZRKu2gY:I was not expecting this to work, since most other
fs.FSimplementations (sayembed.FS) return a not found error whenroot == "", but a panic seemed like a bug.The text was updated successfully, but these errors were encountered: