From 6cf668f749f41426d306d6cfbc87222f89b06d80 Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Tue, 17 Jan 2023 14:06:24 -0500 Subject: [PATCH] fix: nil panic in graalvm cataloger (#1468) * normalize error handling and recover from panics while parsing binaries Signed-off-by: Keith Zantow Signed-off-by: Alex Goodman Co-authored-by: Alex Goodman --- .../java/graalvm_native_image_cataloger.go | 127 +++++++++++------- .../graalvm_native_image_cataloger_test.go | 22 +-- .../java/test-fixtures/java-builds/Makefile | 5 +- .../java-builds/build-example-macho-binary.sh | 7 + 4 files changed, 101 insertions(+), 60 deletions(-) create mode 100755 syft/pkg/cataloger/java/test-fixtures/java-builds/build-example-macho-binary.sh diff --git a/syft/pkg/cataloger/java/graalvm_native_image_cataloger.go b/syft/pkg/cataloger/java/graalvm_native_image_cataloger.go index 4a4955d42..413a8143d 100644 --- a/syft/pkg/cataloger/java/graalvm_native_image_cataloger.go +++ b/syft/pkg/cataloger/java/graalvm_native_image_cataloger.go @@ -115,12 +115,12 @@ func (c *NativeImageCataloger) Name() string { func getPackage(component nativeImageComponent) pkg.Package { var cpes []cpe.CPE for _, property := range component.Properties { - cpe, err := cpe.New(property.Value) + c, err := cpe.New(property.Value) if err != nil { log.Debugf("native-image cataloger: could not parse CPE: %v.", err) continue } - cpes = append(cpes, cpe) + cpes = append(cpes, c) } return pkg.Package{ Name: component.Name, @@ -139,59 +139,59 @@ func getPackage(component nativeImageComponent) pkg.Package { } // decompressSbom returns the packages given within a native image executable's SBOM. -func decompressSbom(databuf []byte, sbomStart uint64, lengthStart uint64) ([]pkg.Package, error) { +func decompressSbom(dataBuf []byte, sbomStart uint64, lengthStart uint64) ([]pkg.Package, error) { var pkgs []pkg.Package lengthEnd := lengthStart + 8 - buflen := len(databuf) - if lengthEnd > uint64(buflen) { - return nil, errors.New("the sbom_length symbol overflows the binary") + bufLen := len(dataBuf) + if lengthEnd > uint64(bufLen) { + return nil, errors.New("the 'sbom_length' symbol overflows the binary") } - length := databuf[lengthStart:lengthEnd] + length := dataBuf[lengthStart:lengthEnd] p := bytes.NewBuffer(length) var storedLength uint64 err := binary.Read(p, binary.LittleEndian, &storedLength) if err != nil { - log.Debugf("native-image-cataloger: could not read from binary file.") - return nil, err + return nil, fmt.Errorf("could not read from binary file: %w", err) } - log.Tracef("native-image cataloger: found SBOM of length %d.", storedLength) + + log.WithFields("len", storedLength).Trace("native-image cataloger: found SBOM") sbomEnd := sbomStart + storedLength - if sbomEnd > uint64(buflen) { + if sbomEnd > uint64(bufLen) { return nil, errors.New("the sbom symbol overflows the binary") } - sbomCompressed := databuf[sbomStart:sbomEnd] + + sbomCompressed := dataBuf[sbomStart:sbomEnd] p = bytes.NewBuffer(sbomCompressed) gzreader, err := gzip.NewReader(p) if err != nil { - log.Debugf("native-image cataloger: could not decompress the SBOM.") - return nil, err + return nil, fmt.Errorf("could not decompress the native-image SBOM: %w", err) } + output, err := io.ReadAll(gzreader) if err != nil { - log.Debugf("native-image cataloger: could not read the decompressed SBOM.") - return nil, err + return nil, fmt.Errorf("could not read the native-image SBOM: %w", err) } + var sbomContent nativeImageCycloneDX err = json.Unmarshal(output, &sbomContent) if err != nil { - log.Debugf("native-image cataloger: could not unmarshal JSON.") - return nil, err + return nil, fmt.Errorf("could not unmarshal the native-image SBOM: %w", err) } for _, component := range sbomContent.Components { p := getPackage(component) pkgs = append(pkgs, p) } + return pkgs, nil } // fileError logs an error message when an executable cannot be read. func fileError(filename string, err error) (nativeImage, error) { // We could not read the file as a binary for the desired platform, but it may still be a native-image executable. - log.Debugf("native-image cataloger: unable to read executable (file=%q): %v.", filename, err) - return nil, err + return nil, fmt.Errorf("unable to read executable (file=%q): %w", filename, err) } // newElf reads a Native Image from an ELF executable. @@ -238,7 +238,7 @@ func newPE(filename string, r io.ReaderAt) (nativeImage, error) { case *pe.OptionalHeader64: exportSymbolsDataDirectory = h.DataDirectory[0] default: - return nil, fmt.Errorf("unable to get exportSymbolsDataDirectory from binary: %s", filename) + return nil, fmt.Errorf("unable to get 'exportSymbolsDataDirectory' from binary: %s", filename) } // If we have no exported symbols it is not a Native Image if exportSymbolsDataDirectory.Size == 0 { @@ -248,8 +248,7 @@ func newPE(filename string, r io.ReaderAt) (nativeImage, error) { exports := make([]byte, exportSymbolsDataDirectory.Size) _, err = r.ReadAt(exports, int64(exportSymbolsOffset)) if err != nil { - log.Debugf("native-image cataloger: could not read the exported symbols data directory: %v.", err) - return fileError(filename, err) + return fileError(filename, fmt.Errorf("could not read the exported symbols data directory: %w", err)) } return nativeImagePE{ file: bi, @@ -273,16 +272,30 @@ func newPE(filename string, r io.ReaderAt) (nativeImage, error) { } // fetchPkgs obtains the packages given in the binary. -func (ni nativeImageElf) fetchPkgs() ([]pkg.Package, error) { +func (ni nativeImageElf) fetchPkgs() (pkgs []pkg.Package, retErr error) { + defer func() { + if r := recover(); r != nil { + // this can happen in cases where a malformed binary is passed in can be initially parsed, but not + // used without error later down the line. + retErr = fmt.Errorf("recovered from panic: %v", r) + } + }() + bi := ni.file + if bi == nil { + log.Debugf("native-image cataloger: file is nil") + return nil, nil + } var sbom elf.Symbol var sbomLength elf.Symbol var svmVersion elf.Symbol si, err := bi.Symbols() if err != nil { - log.Debugf("native-image cataloger: no symbols found.") - return nil, err + return nil, fmt.Errorf("no symbols found in binary: %w", err) + } + if si == nil { + return nil, errors.New(nativeImageMissingSymbolsError) } for _, s := range si { switch s.Name { @@ -295,19 +308,16 @@ func (ni nativeImageElf) fetchPkgs() ([]pkg.Package, error) { } } if sbom.Value == 0 || sbomLength.Value == 0 || svmVersion.Value == 0 { - log.Debugf("native-image cataloger: %v", nativeImageMissingSymbolsError) return nil, errors.New(nativeImageMissingSymbolsError) } dataSection := bi.Section(".data") if dataSection == nil { - log.Debugf("native-image cataloger: .data section missing from ELF file.") - return nil, err + return nil, fmt.Errorf("no .data section found in binary: %w", err) } dataSectionBase := dataSection.SectionHeader.Addr data, err := dataSection.Data() if err != nil { - log.Debugf("native-image cataloger: cannot read the .data section.") - return nil, err + return nil, fmt.Errorf("cannot read the .data section: %w", err) } sbomLocation := sbom.Value - dataSectionBase lengthLocation := sbomLength.Value - dataSectionBase @@ -316,12 +326,27 @@ func (ni nativeImageElf) fetchPkgs() ([]pkg.Package, error) { } // fetchPkgs obtains the packages from a Native Image given as a Mach O file. -func (ni nativeImageMachO) fetchPkgs() ([]pkg.Package, error) { +func (ni nativeImageMachO) fetchPkgs() (pkgs []pkg.Package, retErr error) { + defer func() { + if r := recover(); r != nil { + // this can happen in cases where a malformed binary is passed in can be initially parsed, but not + // used without error later down the line. + retErr = fmt.Errorf("recovered from panic: %v", r) + } + }() + var sbom macho.Symbol var sbomLength macho.Symbol var svmVersion macho.Symbol bi := ni.file + if bi == nil { + log.Debugf("native-image cataloger: file is nil") + return nil, nil + } + if bi.Symtab == nil { + return nil, errors.New(nativeImageMissingSymbolsError) + } for _, s := range bi.Symtab.Syms { switch s.Name { case "_" + nativeImageSbomSymbol: @@ -333,7 +358,6 @@ func (ni nativeImageMachO) fetchPkgs() ([]pkg.Package, error) { } } if sbom.Value == 0 || sbomLength.Value == 0 || svmVersion.Value == 0 { - log.Debugf("native-image cataloger: %v.", nativeImageMissingSymbolsError) return nil, errors.New(nativeImageMissingSymbolsError) } @@ -341,7 +365,7 @@ func (ni nativeImageMachO) fetchPkgs() ([]pkg.Package, error) { if dataSegment == nil { return nil, nil } - databuf, err := dataSegment.Data() + dataBuf, err := dataSegment.Data() if err != nil { log.Debugf("native-image cataloger: cannot obtain buffer from data segment.") return nil, nil @@ -349,7 +373,7 @@ func (ni nativeImageMachO) fetchPkgs() ([]pkg.Package, error) { sbomLocation := sbom.Value - dataSegment.Addr lengthLocation := sbomLength.Value - dataSegment.Addr - return decompressSbom(databuf, sbomLocation, lengthLocation) + return decompressSbom(dataBuf, sbomLocation, lengthLocation) } // fetchExportAttribute obtains an attribute from the exported symbols directory entry. @@ -396,23 +420,19 @@ func (ni nativeImagePE) fetchExportContent() (*exportContentPE, error) { var err error content.numberOfFunctions, err = ni.fetchExportAttribute(0) if err != nil { - log.Debugf("native-image cataloger: could not find the number of exported functions attribute: %v", err) - return nil, err + return nil, fmt.Errorf("could not find the number of exported 'number of functions' attribute: %w", err) } content.numberOfNames, err = ni.fetchExportAttribute(1) if err != nil { - log.Debugf("native-image cataloger: could not find the number of exported names attribute: %v", err) - return nil, err + return nil, fmt.Errorf("could not find the number of exported 'number of names' attribute: %w", err) } content.addressOfFunctions, err = ni.fetchExportAttribute(2) if err != nil { - log.Debugf("native-image cataloger: could not find the exported functions attribute: %v", err) - return nil, err + return nil, fmt.Errorf("could not find the exported 'address of functions' attribute: %w", err) } content.addressOfNames, err = ni.fetchExportAttribute(3) if err != nil { - log.Debugf("native-image cataloger: could not find the exported names attribute: %v", err) - return nil, err + return nil, fmt.Errorf("could not find the exported 'address of names' attribute: %w", err) } return content, nil } @@ -460,7 +480,15 @@ func (ni nativeImagePE) fetchSbomSymbols(content *exportContentPE) { } // fetchPkgs obtains the packages from a Native Image given as a PE file. -func (ni nativeImagePE) fetchPkgs() ([]pkg.Package, error) { +func (ni nativeImagePE) fetchPkgs() (pkgs []pkg.Package, retErr error) { + defer func() { + if r := recover(); r != nil { + // this can happen in cases where a malformed binary is passed in can be initially parsed, but not + // used without error later down the line. + retErr = fmt.Errorf("recovered from panic: %v", r) + } + }() + content, err := ni.fetchExportContent() if err != nil { log.Debugf("native-image cataloger: could not fetch the content of the export directory entry: %v.", err) @@ -468,28 +496,25 @@ func (ni nativeImagePE) fetchPkgs() ([]pkg.Package, error) { } ni.fetchSbomSymbols(content) if content.addressOfSbom == uint32(0) || content.addressOfSbomLength == uint32(0) || content.addressOfSvmVersion == uint32(0) { - log.Debugf("native-image cataloger: %v.", nativeImageMissingSymbolsError) return nil, errors.New(nativeImageMissingSymbolsError) } functionsBase := content.addressOfFunctions - ni.exportSymbols.VirtualAddress sbomOffset := content.addressOfSbom sbomAddress, err := ni.fetchExportFunctionPointer(functionsBase, sbomOffset) if err != nil { - log.Debugf("native-image cataloger: cannot fetch SBOM pointer from exported functions: %v.", err) - return nil, err + return nil, fmt.Errorf("could not fetch SBOM pointer from exported functions: %w", err) } sbomLengthOffset := content.addressOfSbomLength sbomLengthAddress, err := ni.fetchExportFunctionPointer(functionsBase, sbomLengthOffset) if err != nil { - log.Debugf("native-image cataloger: cannot fetch SBOM length pointer from exported functions: %v.", err) - return nil, err + return nil, fmt.Errorf("could not fetch SBOM length pointer from exported functions: %w", err) } bi := ni.file dataSection := bi.Section(".data") if dataSection == nil { return nil, nil } - databuf, err := dataSection.Data() + dataBuf, err := dataSection.Data() if err != nil { log.Debugf("native-image cataloger: cannot obtain buffer from .data section.") return nil, nil @@ -497,7 +522,7 @@ func (ni nativeImagePE) fetchPkgs() ([]pkg.Package, error) { sbomLocation := sbomAddress - dataSection.VirtualAddress lengthLocation := sbomLengthAddress - dataSection.VirtualAddress - return decompressSbom(databuf, uint64(sbomLocation), uint64(lengthLocation)) + return decompressSbom(dataBuf, uint64(sbomLocation), uint64(lengthLocation)) } // fetchPkgs provides the packages available in a UnionReader. diff --git a/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go b/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go index cfe55e72d..e320dc292 100644 --- a/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go +++ b/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go @@ -21,14 +21,20 @@ import ( func TestParseNativeImage(t *testing.T) { tests := []struct { fixture string + newFn func(filename string, r io.ReaderAt) (nativeImage, error) }{ { - fixture: "test-fixtures/java-builds/packages/example-java-app", + fixture: "example-java-app", + newFn: newElf, + }, + { + fixture: "gcc-amd64-darwin-exec-debug", + newFn: newMachO, }, } for _, test := range tests { - t.Run(path.Base(test.fixture), func(t *testing.T) { - f, err := os.Open(test.fixture) + t.Run(test.fixture, func(t *testing.T) { + f, err := os.Open("test-fixtures/java-builds/packages/" + test.fixture) assert.NoError(t, err) readerCloser := io.ReadCloser(ioutil.NopCloser(f)) reader, err := unionreader.GetUnionReader(readerCloser) @@ -36,7 +42,7 @@ func TestParseNativeImage(t *testing.T) { parsed := false readers, err := unionreader.GetReaders(reader) for _, r := range readers { - ni, err := newElf(test.fixture, r) + ni, err := test.newFn(test.fixture, r) assert.NoError(t, err) _, err = ni.fetchPkgs() if err == nil { @@ -108,12 +114,12 @@ func TestParseNativeImageSbom(t *testing.T) { z := gzip.NewWriter(writebytes) _, err = z.Write(sbom) assert.NoError(t, err) - z.Close() - writebytes.Flush() + _ = z.Close() + _ = writebytes.Flush() compressedsbom := b.Bytes() sbomlength := uint64(len(compressedsbom)) - binary.Write(writebytes, binary.LittleEndian, sbomlength) - writebytes.Flush() + _ = binary.Write(writebytes, binary.LittleEndian, sbomlength) + _ = writebytes.Flush() compressedsbom = b.Bytes() actual, err := decompressSbom(compressedsbom, 0, sbomlength) assert.NoError(t, err) diff --git a/syft/pkg/cataloger/java/test-fixtures/java-builds/Makefile b/syft/pkg/cataloger/java/test-fixtures/java-builds/Makefile index fcc61446b..0cac25471 100644 --- a/syft/pkg/cataloger/java/test-fixtures/java-builds/Makefile +++ b/syft/pkg/cataloger/java/test-fixtures/java-builds/Makefile @@ -17,7 +17,7 @@ jars: $(PKGSDIR)/example-java-app-maven-0.1.0.jar $(PKGSDIR)/example-java-app-gr archives: $(PKGSDIR)/example-java-app-maven-0.1.0.zip $(PKGSDIR)/example-java-app-maven-0.1.0.tar $(PKGSDIR)/example-java-app-maven-0.1.0.tar.gz -native-image: $(PKGSDIR)/example-java-app +native-image: $(PKGSDIR)/example-java-app $(PKGSDIR)/gcc-amd64-darwin-exec-debug # jars within archives... @@ -68,6 +68,9 @@ clean-jenkins: $(PKGSDIR)/example-java-app: $(PKGSDIR)/example-java-app-maven-0.1.0.jar ./build-example-java-app-native-image.sh $(PKGSDIR) +$(PKGSDIR)/gcc-amd64-darwin-exec-debug: + ./build-example-macho-binary.sh $(PKGSDIR) + # we need a way to determine if CI should bust the test cache based on the source material $(PKGSDIR).fingerprint: clean-examples mkdir -p $(PKGSDIR) diff --git a/syft/pkg/cataloger/java/test-fixtures/java-builds/build-example-macho-binary.sh b/syft/pkg/cataloger/java/test-fixtures/java-builds/build-example-macho-binary.sh new file mode 100755 index 000000000..f83c92e0b --- /dev/null +++ b/syft/pkg/cataloger/java/test-fixtures/java-builds/build-example-macho-binary.sh @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +set -uxe + +PKGSDIR=$1 + +curl https://raw.githubusercontent.com/blacktop/go-macho/master/internal/testdata/gcc-amd64-darwin-exec-debug.base64 | + base64 -d > $PKGSDIR/gcc-amd64-darwin-exec-debug