From e15264d9c811cbd01e78cd533ea31263738094c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Kreileder?= Date: Wed, 10 Jan 2024 22:26:46 +0100 Subject: [PATCH] :bug: Refactor Dockerfile validation code to handle here-documents (#3774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 https://github.com/ossf/scorecard/issues/3335 Signed-off-by: Jürgen Kreileder * Add test for empty run command case in validateDockerfileInsecureDownloads() Signed-off-by: Jürgen Kreileder * Simplify end line calculation in validateDockerfileInsecureDownloads() Signed-off-by: Jürgen Kreileder * Document why we have a python test case here Signed-off-by: Jürgen Kreileder --------- Signed-off-by: Jürgen Kreileder --- checks/raw/pinned_dependencies.go | 39 ++-- checks/raw/pinned_dependencies_test.go | 175 ++++++++++++++++++ .../raw/testdata/Dockerfile-download-heredoc | 59 ++++++ .../raw/testdata/Dockerfile-empty-run-array | 18 ++ 4 files changed, 277 insertions(+), 14 deletions(-) create mode 100644 checks/raw/testdata/Dockerfile-download-heredoc create mode 100644 checks/raw/testdata/Dockerfile-empty-run-array diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 7a3031e4506..afc352873de 100644 --- 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 index 0eb2ea67c14..f734075005d 100644 --- 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/repo.git@v1.0#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/repo.git@v1.0#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/repo.git@v1.0#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 new file mode 100644 index 00000000000..fb1669f4a15 --- /dev/null +++ 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/repo.git@v1.0#egg=package +EOT + +RUN <