🐛 Refactor Dockerfile validation code to handle here-documents (#3774)

* Refactor Dockerfile validation code to handle here-documents Refactors the `validateDockerfileInsecureDownloads` function to handle Dockerfiles that contain here-documents. This implementation handles the basic use-case, namely shell commands. It does not manage other interpreters that are specified through a she-bang, such as python. Fixes #3335 Signed-off-by: Jürgen Kreileder <[email protected]> * Add test for empty run command case in validateDockerfileInsecureDownloads() Signed-off-by: Jürgen Kreileder <[email protected]> * Simplify end line calculation in validateDockerfileInsecureDownloads() Signed-off-by: Jürgen Kreileder <[email protected]> * Document why we have a python test case here Signed-off-by: Jürgen Kreileder <[email protected]> --------- Signed-off-by: Jürgen Kreileder <[email protected]>
ossf · Jan 10, 2024 · e15264d · e15264d
1 parent 6c345f1
commit e15264d
Show file tree

Hide file tree

Showing 4 changed files with 277 additions and 14 deletions.
diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go
@@ -162,7 +162,6 @@ var validateDockerfileInsecureDownloads fileparser.DoWhileTrueOnFileContent = fu
 	// Walk the Dockerfile's AST.
 	taintedFiles := make(map[string]bool)
 	for i := range res.AST.Children {
-		var bytes []byte
 
 		child := res.AST.Children[i]
 		cmdType := child.Value
@@ -172,21 +171,33 @@ var validateDockerfileInsecureDownloads fileparser.DoWhileTrueOnFileContent = fu
 			continue
 		}
 
-		var valueList []string
-		for n := child.Next; n != nil; n = n.Next {
-			valueList = append(valueList, n.Value)
-		}
+		if len(child.Heredocs) > 0 {
+			startOffset := 1
+			for _, heredoc := range child.Heredocs {
+				cmd := heredoc.Content
+				lineCount := startOffset + strings.Count(cmd, "\n")
+				if err := validateShellFile(pathfn, uint(child.StartLine+startOffset)-1, uint(child.StartLine+lineCount)-2,
+					[]byte(cmd), taintedFiles, pdata); err != nil {
+					return false, err
+				}
+				startOffset += lineCount
+			}
+		} else {
+			var valueList []string
+			for n := child.Next; n != nil; n = n.Next {
+				valueList = append(valueList, n.Value)
+			}
 
-		if len(valueList) == 0 {
-			return false, sce.WithMessage(sce.ErrScorecardInternal, errInternalInvalidDockerFile.Error())
-		}
+			if len(valueList) == 0 {
+				return false, sce.WithMessage(sce.ErrScorecardInternal, errInternalInvalidDockerFile.Error())
+			}
 
-		// Build a file content.
-		cmd := strings.Join(valueList, " ")
-		bytes = append(bytes, cmd...)
-		if err := validateShellFile(pathfn, uint(child.StartLine)-1, uint(child.EndLine)-1,
-			bytes, taintedFiles, pdata); err != nil {
-			return false, err
+			// Build a file content.
+			cmd := strings.Join(valueList, " ")
+			if err := validateShellFile(pathfn, uint(child.StartLine)-1, uint(child.EndLine)-1,
+				[]byte(cmd), taintedFiles, pdata); err != nil {
+				return false, err
+			}
 		}
 	}
 

diff --git a/checks/raw/pinned_dependencies_test.go b/checks/raw/pinned_dependencies_test.go
@@ -631,6 +631,38 @@ func TestDockerfileInvalidFiles(t *testing.T) {
 	}
 }
 
+func TestDockerfileInsecureDownloadsBrokenCommands(t *testing.T) {
+	t.Parallel()
+	//nolint:govet
+	tests := []struct {
+		name     string
+		filename string
+		err      error
+	}{
+		{
+			name:     "dockerfile downloads",
+			filename: "./testdata/Dockerfile-empty-run-array",
+			err:      errInternalInvalidDockerFile,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt // Re-initializing variable so it is not changed while executing the closure below
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			content, err := os.ReadFile(tt.filename)
+			if err != nil {
+				t.Errorf("cannot read file: %v", err)
+			}
+
+			var r checker.PinningDependenciesData
+			_, err = validateDockerfileInsecureDownloads(tt.filename, content, &r)
+			if !strings.Contains(err.Error(), tt.err.Error()) {
+				t.Errorf(cmp.Diff(err, tt.err, cmpopts.EquateErrors()))
+			}
+		})
+	}
+}
+
 func TestDockerfileInsecureDownloadsLineNumber(t *testing.T) {
 	t.Parallel()
 	//nolint:govet
@@ -843,6 +875,149 @@ func TestDockerfileInsecureDownloadsLineNumber(t *testing.T) {
 	}
 }
 
+func TestDockerfileWithHeredocsInsecureDownloadsLineNumber(t *testing.T) {
+	t.Parallel()
+	//nolint:govet
+	tests := []struct {
+		name             string
+		filename         string
+		processingErrors int
+		expected         []struct {
+			snippet   string
+			startLine uint
+			endLine   uint
+			pinned    bool
+			t         checker.DependencyUseType
+		}
+	}{
+		{
+			name:             "dockerfile heredoc downloads",
+			filename:         "./testdata/Dockerfile-download-heredoc",
+			processingErrors: 1,
+			expected: []struct {
+				snippet   string
+				startLine uint
+				endLine   uint
+				pinned    bool
+				t         checker.DependencyUseType
+			}{
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/[email protected]#egg=package",
+					startLine: 20,
+					endLine:   20,
+					pinned:    false,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567",
+					startLine: 24,
+					endLine:   24,
+					pinned:    true,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "curl bla | bash",
+					startLine: 28,
+					endLine:   28,
+					pinned:    false,
+					t:         checker.DependencyUseTypeDownloadThenRun,
+				},
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567",
+					startLine: 32,
+					endLine:   32,
+					pinned:    true,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/[email protected]#egg=package",
+					startLine: 36,
+					endLine:   36,
+					pinned:    false,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "curl bla | bash",
+					startLine: 38,
+					endLine:   38,
+					pinned:    false,
+					t:         checker.DependencyUseTypeDownloadThenRun,
+				},
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567",
+					startLine: 42,
+					endLine:   43,
+					pinned:    true,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/[email protected]#egg=package",
+					startLine: 43,
+					endLine:   44,
+					pinned:    false,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "curl bla | bash",
+					startLine: 45,
+					endLine:   45,
+					pinned:    false,
+					t:         checker.DependencyUseTypeDownloadThenRun,
+				},
+				{
+					snippet:   "pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567",
+					startLine: 50,
+					endLine:   52,
+					pinned:    true,
+					t:         checker.DependencyUseTypePipCommand,
+				},
+				{
+					snippet:   "curl bla | bash",
+					startLine: 51,
+					endLine:   53,
+					pinned:    false,
+					t:         checker.DependencyUseTypeDownloadThenRun,
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt // Re-initializing variable so it is not changed while executing the closure below
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			content, err := os.ReadFile(tt.filename)
+			if err != nil {
+				t.Errorf("cannot read file: %v", err)
+			}
+
+			var r checker.PinningDependenciesData
+			_, err = validateDockerfileInsecureDownloads(tt.filename, content, &r)
+			if err != nil {
+				t.Errorf("error during validateDockerfileInsecureDownloads: %v", err)
+			}
+
+			for _, expectedDep := range tt.expected {
+				isExpectedDep := func(dep checker.Dependency) bool {
+					return dep.Location.Offset == expectedDep.startLine &&
+						dep.Location.EndOffset == expectedDep.endLine &&
+						dep.Location.Path == tt.filename &&
+						dep.Location.Snippet == expectedDep.snippet &&
+						*dep.Pinned == expectedDep.pinned &&
+						dep.Type == expectedDep.t
+				}
+
+				if !scut.ValidatePinningDependencies(isExpectedDep, &r) {
+					t.Errorf("test failed: dependency not present: %+v", tt.expected)
+				}
+			}
+
+			if tt.processingErrors != len(r.ProcessingErrors) {
+				t.Errorf("expected %v processing errors. Got %v", tt.processingErrors, len(r.ProcessingErrors))
+			}
+		})
+	}
+}
+
 func TestShellscriptInsecureDownloadsLineNumber(t *testing.T) {
 	t.Parallel()
 	//nolint:govet

diff --git a/checks/raw/testdata/Dockerfile-download-heredoc b/checks/raw/testdata/Dockerfile-download-heredoc
@@ -0,0 +1,59 @@
+
+# Copyright 2024 OpenSSF Scorecard Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Add tab
+    FROM python:3.7
+
+RUN <<-EOT
+	pip install --no-deps -e git+https://github.com/username/[email protected]#egg=package
+EOT
+
+RUN <<EOT
+pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567
+EOT
+
+RUN <<"EOT"
+	curl bla | bash
+EOT
+
+RUN <<EOT1 bash
+	pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567
+EOT1
+
+RUN <<EOT1 <<EOT2
+	pip install --no-deps -e git+https://github.com/username/[email protected]#egg=package
+EOT1
+	curl bla | bash
+EOT2
+
+RUN <<EOT1 <<EOT2
+	pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567
+	pip install --no-deps -e git+https://github.com/username/[email protected]#egg=package
+EOT1
+	curl bla | bash
+EOT2
+
+RUN <<EOT
+#!/bin/sh
+pip install --no-deps -e git+https://github.com/username/repo.git@0123456789abcdef0123456789abcdef01234567
+curl bla | bash
+EOT
+
+# Here-documents can specify interpreters with shebang headers. This shouldn't cause a fatal error.
+# Example from https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md
+RUN <<EOT
+#!/usr/bin/env python
+print("hello world")
+EOT
diff --git a/checks/raw/testdata/Dockerfile-empty-run-array b/checks/raw/testdata/Dockerfile-empty-run-array
@@ -0,0 +1,18 @@
+# Copyright 2024 OpenSSF Scorecard Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# Note: taken from https://github.com/pushiqiang/utils/blob/master/docker/Dockerfile_template
+
+FROM scratch
+
+RUN []