fix: ensure to capture and rethrow errors from image layer parsing

Right now any errors that occur during image layer parsing are not properly caught (because they occur either in an async or EventEmitter context) and result in uncaught exceptions. One of these places is the Go binaries parser, which we now properly try-catch and reject the promise that wraps it. Additionally we need to try-catch the image layer parsing process as a whole, which was missing before.
snyk · Feb 9, 2021 · aca5829 · aca5829
1 parent 4b2b142
commit aca5829
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 68 deletions.
diff --git a/lib/extractor/layer.ts b/lib/extractor/layer.ts
@@ -1,10 +1,13 @@
+import * as Debug from "debug";
 import * as gunzip from "gunzip-maybe";
 import * as path from "path";
 import { Readable } from "stream";
 import { extract, Extract } from "tar-stream";
 import { applyCallbacks, isResultEmpty } from "./callbacks";
 import { ExtractAction, ExtractedLayers } from "./types";
 
+const debug = Debug("snyk");
+
 /**
  * Extract key files from the specified TAR stream.
  * @param layerTarStream image layer as a Readable TAR stream. Note: consumes the stream.
@@ -26,14 +29,23 @@ export async function extractImageLayer(
           action.filePathMatches(absoluteFileName),
         );
         if (matchedActions.length > 0) {
-          const callbackResult = await applyCallbacks(
-            matchedActions,
-            stream,
-            headers.size,
-          );
-
-          if (!isResultEmpty(callbackResult)) {
-            result[absoluteFileName] = callbackResult;
+          try {
+            const callbackResult = await applyCallbacks(
+              matchedActions,
+              stream,
+              headers.size,
+            );
+
+            if (!isResultEmpty(callbackResult)) {
+              result[absoluteFileName] = callbackResult;
+            }
+          } catch (error) {
+            // An ExtractAction has thrown an uncaught exception, likely a bug in the code!
+            debug(
+              "Exception thrown while applying callbacks during image layer extraction",
+              JSON.stringify(error),
+            );
+            reject(error);
           }
         }
       }

diff --git a/lib/go-parser/index.ts b/lib/go-parser/index.ts
@@ -60,58 +60,62 @@ async function findGoBinaries(
     let bytesWritten = 0;
 
     stream.on("end", () => {
-      // Discard
-      if (bytesWritten === 0) {
-        return resolve();
-      }
-
-      const binaryFile = elf.parse(buffer);
-
-      const goBuildInfo = binaryFile.body.sections.find(
-        (section) => section.name === ".go.buildinfo",
-      );
-      // Could be found in file headers
-      const goBuildId = binaryFile.body.sections.find(
-        (section) => section.name === ".note.go.buildid",
-      );
-
-      const interp = binaryFile.body.sections.find(
-        (section) => section.name === ".interp",
-      );
-
-      if (!goBuildInfo && !goBuildId) {
-        return resolve();
-      } else if (interp) {
-        // Compiled using cgo
-        // we wouldn't be able to extract modules
-        // TODO: cgo-compiled binaries are not supported in this iteration
-        return resolve();
-      } else if (goBuildInfo) {
-        const info = goBuildInfo.data
-          .slice(0, buildInfoMagic.length)
-          .toString(encoding);
-
-        if (info === buildInfoMagic) {
-          return resolve(binaryFile);
+      try {
+        // Discard
+        if (bytesWritten === 0) {
+          return resolve();
         }
 
-        return resolve();
-      } else if (goBuildId) {
-        const strings = goBuildId.data
-          .toString()
-          .split(/\0+/g)
-          .filter(Boolean);
-        const go = strings[strings.length - 2];
-        const buildIdParts = strings[strings.length - 1].split("/");
-
-        // Build ID's precise form is actionID/[.../]contentID.
-        // Usually the buildID is simply actionID/contentID, but with exceptions.
-        // https://github.com/golang/go/blob/master/src/cmd/go/internal/work/buildid.go#L23
-        if (go === buildIdMagic && buildIdParts.length >= 2) {
-          return resolve(binaryFile);
+        const binaryFile = elf.parse(buffer);
+
+        const goBuildInfo = binaryFile.body.sections.find(
+          (section) => section.name === ".go.buildinfo",
+        );
+        // Could be found in file headers
+        const goBuildId = binaryFile.body.sections.find(
+          (section) => section.name === ".note.go.buildid",
+        );
+
+        const interp = binaryFile.body.sections.find(
+          (section) => section.name === ".interp",
+        );
+
+        if (!goBuildInfo && !goBuildId) {
+          return resolve();
+        } else if (interp) {
+          // Compiled using cgo
+          // we wouldn't be able to extract modules
+          // TODO: cgo-compiled binaries are not supported in this iteration
+          return resolve();
+        } else if (goBuildInfo) {
+          const info = goBuildInfo.data
+            .slice(0, buildInfoMagic.length)
+            .toString(encoding);
+
+          if (info === buildInfoMagic) {
+            return resolve(binaryFile);
+          }
+
+          return resolve();
+        } else if (goBuildId) {
+          const strings = goBuildId.data
+            .toString()
+            .split(/\0+/g)
+            .filter(Boolean);
+          const go = strings[strings.length - 2];
+          const buildIdParts = strings[strings.length - 1].split("/");
+
+          // Build ID's precise form is actionID/[.../]contentID.
+          // Usually the buildID is simply actionID/contentID, but with exceptions.
+          // https://github.com/golang/go/blob/master/src/cmd/go/internal/work/buildid.go#L23
+          if (go === buildIdMagic && buildIdParts.length >= 2) {
+            return resolve(binaryFile);
+          }
+
+          return resolve();
         }
-
-        return resolve();
+      } catch (error) {
+        reject(error);
       }
     });
 

diff --git a/test/system/application-scans/gomodules.spec.ts b/test/system/application-scans/gomodules.spec.ts
@@ -23,26 +23,20 @@ describe("gomodules binaries scanning", () => {
     expect(pluginResult).toMatchSnapshot();
   });
 
-  it("throws an uncaught exception when a binary cannot be parsed", async () => {
+  it("throws an error when a Go binary cannot be parsed", async () => {
     const elfParseMock = jest.spyOn(elf, "parse").mockImplementation(() => {
       throw new Error("Cannot read property 'type' of undefined");
     });
 
     const fixturePath = getFixture("docker-archives/docker-save/yq.tar");
     const imageNameAndTag = `docker-archive:${fixturePath}`;
 
-    try {
-      const pluginResult = await scan({
+    await expect(() =>
+      scan({
         path: imageNameAndTag,
         "app-vulns": true,
-      });
-      expect(pluginResult).toEqual<PluginResponse>({
-        scanResults: expect.any(Array),
-      });
-    } catch (error) {
-      // This won't be executed!
-      expect(error).toBeDefined();
-    }
+      }),
+    ).rejects.toThrow("Error reading tar archive");
 
     elfParseMock.mockRestore();
   });