From 2f32d351874ec4945e6b7d9db9451e1dede3746f Mon Sep 17 00:00:00 2001 From: John Kjell Date: Wed, 10 Jan 2024 10:15:04 -0500 Subject: [PATCH 1/2] Fix "uncontrolled data use" from not verifying input to archivista get/store request parameters Signed-off-by: John Kjell --- internal/objectstorage/filestore/file.go | 12 ++- internal/objectstorage/filestore/file_test.go | 79 +++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 internal/objectstorage/filestore/file_test.go diff --git a/internal/objectstorage/filestore/file.go b/internal/objectstorage/filestore/file.go index 47be12dc..f46d75bb 100644 --- a/internal/objectstorage/filestore/file.go +++ b/internal/objectstorage/filestore/file.go @@ -44,9 +44,17 @@ func New(ctx context.Context, directory string, address string) (*Store, <-chan } func (s *Store) Get(ctx context.Context, gitoid string) (io.ReadCloser, error) { - return os.Open(filepath.Join(s.prefix, gitoid+".json")) + if filepath.IsLocal(gitoid) { + return os.Open(filepath.Join(s.prefix, gitoid+".json")) + } else { + return nil, filepath.ErrBadPattern + } } func (s *Store) Store(ctx context.Context, gitoid string, payload []byte) error { - return os.WriteFile(filepath.Join(s.prefix, gitoid+".json"), payload, 0644) + if filepath.IsLocal(gitoid) { + return os.WriteFile(filepath.Join(s.prefix, gitoid+".json"), payload, 0644) + } else { + return filepath.ErrBadPattern + } } diff --git a/internal/objectstorage/filestore/file_test.go b/internal/objectstorage/filestore/file_test.go new file mode 100644 index 00000000..5ded1fd9 --- /dev/null +++ b/internal/objectstorage/filestore/file_test.go @@ -0,0 +1,79 @@ +// Copyright 2022 The Archivista Contributors +// +// 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. +package filestore_test + +import ( + "context" + "io" + "os" + "path/filepath" + "testing" + + filestore "github.com/in-toto/archivista/internal/objectstorage/filestore" +) + +func TestStore_Get(t *testing.T) { + // Create a temporary directory for testing + tempDir, err := os.MkdirTemp("", "filestore_test") + if err != nil { + t.Fatalf("Failed to create temporary directory: %v", err) + } + defer os.RemoveAll(tempDir) + + // Create a new file store + store, _, err := filestore.New(context.Background(), tempDir, "") + if err != nil { + t.Fatalf("Failed to create file store: %v", err) + } + + // Define a test payload + payload := []byte("test payload") + + // Store the payload + err = store.Store(context.Background(), "test_gitoid", payload) + if err != nil { + t.Fatalf("Failed to store payload: %v", err) + } + + // Attempt storing at malicious payload location + err = store.Store(context.Background(), "../../test_gitoid", payload) + if err != nil && err != filepath.ErrBadPattern { + t.Errorf("Failed to detect bad path: %v", err) + } + + // Retrieve the payload + reader, err := store.Get(context.Background(), "test_gitoid") + if err != nil { + t.Errorf("Failed to retrieve payload: %v", err) + } + defer reader.Close() + + // Read the payload from the reader + retrievedPayload, err := io.ReadAll(reader) + if err != nil { + t.Fatalf("Failed to read payload: %v", err) + } + + // Compare the retrieved payload with the original payload + if string(retrievedPayload) != string(payload) { + t.Errorf("Retrieved payload does not match original payload") + } + + // Attempt to retrieve non-local payload + _, err = store.Get(context.Background(), "/etc/passwd") + if err != nil && err != filepath.ErrBadPattern { + t.Errorf("Failed to detect bad path: %v", err) + } + +} From 9da5ca30486502647fb6571b8f2059d1732b4f93 Mon Sep 17 00:00:00 2001 From: Kairo Araujo Date: Thu, 11 Jan 2024 15:13:07 +0100 Subject: [PATCH 2/2] Migrate UT to testify test suite (#154) Signed-off-by: Kairo de Araujo --- internal/objectstorage/filestore/file_test.go | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/internal/objectstorage/filestore/file_test.go b/internal/objectstorage/filestore/file_test.go index 5ded1fd9..7521edb5 100644 --- a/internal/objectstorage/filestore/file_test.go +++ b/internal/objectstorage/filestore/file_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 The Archivista Contributors +// Copyright 2024 The Archivista Contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -20,21 +20,39 @@ import ( "path/filepath" "testing" - filestore "github.com/in-toto/archivista/internal/objectstorage/filestore" + "github.com/in-toto/archivista/internal/objectstorage/filestore" + "github.com/stretchr/testify/suite" ) -func TestStore_Get(t *testing.T) { +// Test Suite: UT FileStoreSuite +type UTFileStoreSuite struct { + suite.Suite + tempDir string + payload []byte +} + +func TestUTFileStoreSuite(t *testing.T) { + suite.Run(t, new(UTFileStoreSuite)) +} + +func (ut *UTFileStoreSuite) SetupTest() { // Create a temporary directory for testing tempDir, err := os.MkdirTemp("", "filestore_test") if err != nil { - t.Fatalf("Failed to create temporary directory: %v", err) + ut.FailNow(err.Error()) } - defer os.RemoveAll(tempDir) + ut.tempDir = tempDir + ut.payload = []byte("test payload") +} + +func (ut *UTFileStoreSuite) TearDownTest() { + os.RemoveAll(ut.tempDir) +} +func (ut *UTFileStoreSuite) Test_Get() { - // Create a new file store - store, _, err := filestore.New(context.Background(), tempDir, "") + store, _, err := filestore.New(context.Background(), ut.tempDir, "") if err != nil { - t.Fatalf("Failed to create file store: %v", err) + ut.FailNow(err.Error()) } // Define a test payload @@ -43,37 +61,35 @@ func TestStore_Get(t *testing.T) { // Store the payload err = store.Store(context.Background(), "test_gitoid", payload) if err != nil { - t.Fatalf("Failed to store payload: %v", err) + ut.FailNow(err.Error()) } // Attempt storing at malicious payload location err = store.Store(context.Background(), "../../test_gitoid", payload) if err != nil && err != filepath.ErrBadPattern { - t.Errorf("Failed to detect bad path: %v", err) + ut.FailNowf("Failed to detect bad path: %v", err.Error()) } // Retrieve the payload reader, err := store.Get(context.Background(), "test_gitoid") if err != nil { - t.Errorf("Failed to retrieve payload: %v", err) + ut.FailNowf("Failed to retrieve payload: %v", err.Error()) } defer reader.Close() // Read the payload from the reader retrievedPayload, err := io.ReadAll(reader) if err != nil { - t.Fatalf("Failed to read payload: %v", err) + ut.FailNowf("Failed to read payload: %v", err.Error()) } // Compare the retrieved payload with the original payload - if string(retrievedPayload) != string(payload) { - t.Errorf("Retrieved payload does not match original payload") - } + ut.Equal(string(retrievedPayload), string(payload)) // Attempt to retrieve non-local payload _, err = store.Get(context.Background(), "/etc/passwd") if err != nil && err != filepath.ErrBadPattern { - t.Errorf("Failed to detect bad path: %v", err) + ut.FailNowf("Failed to detect bad path: %v", err.Error()) } }