diff --git a/pkg/handlers/apk_content_detection_test.go b/pkg/handlers/apk_content_detection_test.go new file mode 100644 index 000000000..c8da1021b --- /dev/null +++ b/pkg/handlers/apk_content_detection_test.go @@ -0,0 +1,116 @@ +package handlers + +import ( + "archive/zip" + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/trufflesecurity/trufflehog/v3/pkg/context" + "github.com/trufflesecurity/trufflehog/v3/pkg/feature" +) + +// makeAPKZip builds an in-memory zip that satisfies isAPKFile (contains both +// AndroidManifest.xml and classes.dex). A large stored padding entry is written +// first so the manifest/dex local headers fall outside mimetype's detection +// window, keeping mimetype's verdict "application/zip" rather than APK. This +// forces the content-based isAPKFile deep scan (rather than mimetype's own APK +// detection) to be what promotes the file to the APK handler. +func makeAPKZip(t *testing.T, extraFiles map[string][]byte) []byte { + t.Helper() + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + padding, err := zw.CreateHeader(&zip.FileHeader{Name: "padding.txt", Method: zip.Store}) + require.NoError(t, err) + _, err = padding.Write(bytes.Repeat([]byte("A"), 5000)) + require.NoError(t, err) + + for _, name := range []string{"AndroidManifest.xml", "classes.dex"} { + w, err := zw.Create(name) + require.NoError(t, err) + _, err = w.Write([]byte("placeholder")) + require.NoError(t, err) + } + + for name, content := range extraFiles { + w, err := zw.Create(name) + require.NoError(t, err) + _, err = w.Write(content) + require.NoError(t, err) + } + + require.NoError(t, zw.Close()) + return buf.Bytes() +} + +// makeNonAPKZip builds a plain in-memory zip without any APK markers. +func makeNonAPKZip(t *testing.T) []byte { + t.Helper() + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + w, err := zw.Create("hello.txt") + require.NoError(t, err) + _, err = w.Write([]byte("just some content")) + require.NoError(t, err) + require.NoError(t, zw.Close()) + return buf.Bytes() +} + +// TestNewFileReaderAPKContentDetection verifies APK detection is driven by file +// content (the presence of AndroidManifest.xml + classes.dex) and not by any +// filename/extension: the reader is promoted to the APK mime type with no +// extension supplied, while a zip lacking those markers stays a generic zip. +func TestNewFileReaderAPKContentDetection(t *testing.T) { + feature.EnableAPKHandler.Store(true) + t.Cleanup(func() { feature.EnableAPKHandler.Store(false) }) + + ctx := context.Background() + + rdr, err := newFileReader(ctx, bytes.NewReader(makeAPKZip(t, nil))) + require.NoError(t, err) + assert.Equal(t, string(apkMime), rdr.mime.String(), "APK content should route to the APK handler without an extension") + require.NoError(t, rdr.Close()) + + rdrPlain, err := newFileReader(ctx, bytes.NewReader(makeNonAPKZip(t))) + require.NoError(t, err) + assert.Equal(t, string(zipMime), rdrPlain.mime.String(), "a zip without APK markers should remain a generic zip") + require.NoError(t, rdrPlain.Close()) +} + +// TestNewFileReaderAPKDetectionDisabled ensures APK content detection does not +// run when the feature flag is off, leaving the file as a generic zip. +func TestNewFileReaderAPKDetectionDisabled(t *testing.T) { + feature.EnableAPKHandler.Store(false) + + rdr, err := newFileReader(context.Background(), bytes.NewReader(makeAPKZip(t, nil))) + require.NoError(t, err) + assert.Equal(t, string(zipMime), rdr.mime.String()) + require.NoError(t, rdr.Close()) +} + +// TestNewFileReaderAPKCheckErrorFallsThrough ensures that when the APK feature +// flag is on and mimetype identifies a file as a zip, but the content-based APK +// check fails to parse it (e.g. truncated/corrupted zip, or a polyglot that +// mimetype still reports as zip), newFileReader does NOT return a fatal error. +// Instead it must fall through to normal handling so the file is still processed +// rather than skipped entirely. This guards the regression where dropping the +// .apk extension guard made every unparseable zip/jar unprocessable. +func TestNewFileReaderAPKCheckErrorFallsThrough(t *testing.T) { + feature.EnableAPKHandler.Store(true) + t.Cleanup(func() { feature.EnableAPKHandler.Store(false) }) + + // Start from a valid zip (so mimetype detects "application/zip") then truncate + // the tail, removing the end-of-central-directory record so zip.NewReader fails. + valid := makeNonAPKZip(t) + truncated := valid[:len(valid)-10] + + rdr, err := newFileReader(context.Background(), bytes.NewReader(truncated)) + require.NoError(t, err, "APK check failure must not be fatal; file should fall through to normal handling") + assert.Equal(t, string(zipMime), rdr.mime.String(), "file should remain a generic zip, not be skipped") + require.NoError(t, rdr.Close()) +} diff --git a/pkg/handlers/handlers.go b/pkg/handlers/handlers.go index 858feeac6..9c6b0c6a6 100644 --- a/pkg/handlers/handlers.go +++ b/pkg/handlers/handlers.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "path/filepath" "github.com/gabriel-vasile/mimetype" "github.com/mholt/archives" @@ -54,14 +53,6 @@ var ( ErrProcessingWarning = errors.New("error processing file") ) -type readerConfig struct{ fileExtension string } - -type readerOption func(*readerConfig) - -func withFileExtension(ext string) readerOption { - return func(c *readerConfig) { c.fileExtension = ext } -} - // mimeTypeReader wraps an io.Reader with MIME type information. // This type is used to pass content through the processing pipeline // while carrying its detected MIME type, avoiding redundant type detection. @@ -103,12 +94,7 @@ func newMimeTypeReader(r io.Reader) (mimeTypeReader, error) { // newFileReader creates a fileReader from an io.Reader, optionally using BufferedFileWriter for certain formats. // The caller is responsible for closing the reader when it is no longer needed. -func newFileReader(ctx context.Context, r io.Reader, options ...readerOption) (fReader fileReader, err error) { - var cfg readerConfig - - for _, opt := range options { - opt(&cfg) - } +func newFileReader(ctx context.Context, r io.Reader) (fReader fileReader, err error) { // To detect the MIME type of the input data, we need a reader that supports seeking. // This allows us to read the data multiple times if necessary without losing the original position. // We use a BufferedReaderSeeker to wrap the original reader, enabling this functionality. @@ -136,15 +122,26 @@ func newFileReader(ctx context.Context, r io.Reader, options ...readerOption) (f return fReader, fmt.Errorf("error resetting reader after MIME detection: %w", err) } - // Check for APK files - if shouldHandleAsAPK(cfg, fReader) { - isAPK, err := isAPKFile(&fReader) - if err != nil { - return fReader, fmt.Errorf("error checking for APK: %w", err) - } - if isAPK { + // Detection is content-based, so this now runs for every zip/jar rather than only for files with an apk extension. + if shouldHandleAsAPK(fReader) { + // A failure here (e.g. truncated zip, corrupted central directory, or a polyglot that mimetype still + // reports as zip/jar) only means the file isn't a parseable APK. Since this check now runs for every + // zip/jar rather than only files with an .apk extension, we must not treat the error as fatal; otherwise + // any such file would be skipped entirely instead of falling through to the archive or default handler. + isAPK, apkErr := isAPKFile(&fReader) + switch { + case apkErr != nil: + logContext.AddLogger(ctx).Logger().V(3).Info( + "error checking for APK, falling back to normal handling", "error", apkErr) + case isAPK: return handleAPKFile(&fReader) } + + // isAPKFile inspects the archive via zip.NewReader, which leaves the reader positioned mid-stream. + // Reset to the start so archive identification and any downstream handler read from the beginning. + if _, seekErr := fReader.Seek(0, io.SeekStart); seekErr != nil { + return fReader, fmt.Errorf("error resetting reader after APK check: %w", seekErr) + } } // If a MIME type is known to not be an archive type, we might as well return here rather than @@ -224,7 +221,6 @@ const ( rpmHandlerType handlerType = "rpm" apkHandlerType handlerType = "apk" defaultHandlerType handlerType = "default" - apkExt = ".apk" ) type mimeType string @@ -359,8 +355,7 @@ func HandleFile( return errors.New("reader is nil") } - readerOption := withFileExtension(getFileExtension(chunkSkel)) - rdr, err := newFileReader(ctx, reader, readerOption) + rdr, err := newFileReader(ctx, reader) if err != nil { if errors.Is(err, ErrEmptyReader) { ctx.Logger().V(5).Info("empty reader, skipping file") @@ -492,91 +487,14 @@ func isFatal(err error) bool { } } -// getFileExtension extracts the file extension from the chunk's SourceMetadata. -// It considers all sources defined in the MetaData message. -// Note: Probably should add this as a method to the source_metadatapb object. -// then it'd just be chunkSkel.SourceMetadata.GetFileExtension() -func getFileExtension(chunkSkel *sources.Chunk) string { - if chunkSkel == nil || chunkSkel.SourceMetadata == nil { - return "" - } - - var fileName string - - // Inspect the SourceMetadata to determine the source type - switch metadata := chunkSkel.SourceMetadata.Data.(type) { - case *source_metadatapb.MetaData_Artifactory: - fileName = metadata.Artifactory.Path - case *source_metadatapb.MetaData_Azure: - fileName = metadata.Azure.File - case *source_metadatapb.MetaData_AzureRepos: - fileName = metadata.AzureRepos.File - case *source_metadatapb.MetaData_Bitbucket: - fileName = metadata.Bitbucket.File - case *source_metadatapb.MetaData_Buildkite: - fileName = metadata.Buildkite.Link - case *source_metadatapb.MetaData_Circleci: - fileName = metadata.Circleci.Link - case *source_metadatapb.MetaData_Confluence: - fileName = metadata.Confluence.File - case *source_metadatapb.MetaData_Docker: - fileName = metadata.Docker.File - case *source_metadatapb.MetaData_Ecr: - fileName = metadata.Ecr.File - case *source_metadatapb.MetaData_Filesystem: - fileName = metadata.Filesystem.File - case *source_metadatapb.MetaData_Git: - fileName = metadata.Git.File - case *source_metadatapb.MetaData_Github: - fileName = metadata.Github.File - case *source_metadatapb.MetaData_Gitlab: - fileName = metadata.Gitlab.File - case *source_metadatapb.MetaData_Gcs: - fileName = metadata.Gcs.Filename - case *source_metadatapb.MetaData_GoogleDrive: - fileName = metadata.GoogleDrive.File - case *source_metadatapb.MetaData_Huggingface: - fileName = metadata.Huggingface.File - case *source_metadatapb.MetaData_Jira: - fileName = metadata.Jira.Link - case *source_metadatapb.MetaData_Jenkins: - fileName = metadata.Jenkins.Link - case *source_metadatapb.MetaData_Npm: - fileName = metadata.Npm.File - case *source_metadatapb.MetaData_Pypi: - fileName = metadata.Pypi.File - case *source_metadatapb.MetaData_S3: - fileName = metadata.S3.File - case *source_metadatapb.MetaData_Slack: - fileName = metadata.Slack.File - case *source_metadatapb.MetaData_Sharepoint: - fileName = metadata.Sharepoint.Link - case *source_metadatapb.MetaData_Gerrit: - fileName = metadata.Gerrit.File - case *source_metadatapb.MetaData_Test: - fileName = metadata.Test.File - case *source_metadatapb.MetaData_Teams: - fileName = metadata.Teams.File - case *source_metadatapb.MetaData_TravisCI: - fileName = metadata.TravisCI.Link - // Add other sources if they have a file or equivalent field - // Skipping Syslog, Forager, Postman, Vector, Webhook and Elasticsearch - default: - return "" - } - - // Use filepath.Ext to extract the file extension from the file name - ext := filepath.Ext(fileName) - return ext -} - -// shouldHandleAsAPK checks if the file should be handled as an APK based on config and MIME type. -// Note: We can't extend the mimetype package with an APK detection function b/c it would require adjusting settings +// shouldHandleAsAPK reports whether the file should be inspected as a potential APK. +// Detection is content-based (file magic): after confirming the file is a zip/jar, the caller runs isAPKFile, which +// inspects the archive entries for APK markers such as AndroidManifest.xml and classes.dex. +// It deliberately does NOT rely on the file extension so APKs are detected regardless of the source or filename. +// Note: Don't extend the mimetype package with APK detection function. It would require adjusting settings // so that all files are fully read into a byte slice for detection (mimetype.SetLimit(0)), which would bloat memory. -// Instead we call the isAPKFile function in here after ensuring it's a zip/jar file and has an .apk extension. -func shouldHandleAsAPK(cfg readerConfig, fReader fileReader) bool { +func shouldHandleAsAPK(fReader fileReader) bool { return feature.EnableAPKHandler.Load() && - cfg.fileExtension == apkExt && (fReader.mime.String() == string(zipMime) || fReader.mime.String() == string(jarMime)) } diff --git a/pkg/sources/json_enumerator/json_enumerator_test.go b/pkg/sources/json_enumerator/json_enumerator_test.go index 91f777a62..fd215adf2 100644 --- a/pkg/sources/json_enumerator/json_enumerator_test.go +++ b/pkg/sources/json_enumerator/json_enumerator_test.go @@ -1,6 +1,8 @@ package json_enumerator import ( + "archive/zip" + "bytes" "encoding/json" "io" "strings" @@ -12,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/trufflesecurity/trufflehog/v3/pkg/context" + "github.com/trufflesecurity/trufflehog/v3/pkg/feature" "github.com/trufflesecurity/trufflehog/v3/pkg/sources" ) @@ -127,3 +130,87 @@ func TestScanEnumerator(t *testing.T) { }) } } + +// makeZip builds an in-memory zip from the given entries. A large stored padding +// entry is written first so mimetype classifies the bytes as a generic zip, +// leaving APK identification to the content-based deep scan. +func makeZip(t *testing.T, files map[string]string) []byte { + t.Helper() + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + padding, err := zw.CreateHeader(&zip.FileHeader{Name: "padding.txt", Method: zip.Store}) + require.NoError(t, err) + _, err = padding.Write(bytes.Repeat([]byte("A"), 5000)) + require.NoError(t, err) + + for name, content := range files { + w, err := zw.Create(name) + require.NoError(t, err) + _, err = w.Write([]byte(content)) + require.NoError(t, err) + } + + require.NoError(t, zw.Close()) + return buf.Bytes() +} + +// TestScanEnumeratorAPKContentRouting proves json-enumerator input is routed by +// file content, not by any filename/extension. The records carry no filename. +// A zip whose contents mark it as an APK (AndroidManifest.xml + classes.dex) is +// routed to the APK handler; because this synthetic archive lacks resources.arsc +// the APK handler yields no chunks, so the plaintext secret is not surfaced. +// The identical secret in a plain zip (no APK markers) is handled generically +// and surfaces the secret. +func TestScanEnumeratorAPKContentRouting(t *testing.T) { + feature.EnableAPKHandler.Store(true) + t.Cleanup(func() { feature.EnableAPKHandler.Store(false) }) + + secret := secretPart1 + secretPart2 + + apkZip := makeZip(t, map[string]string{ + "AndroidManifest.xml": "placeholder", + "classes.dex": "placeholder", + "assets/secret.txt": secret, + }) + plainZip := makeZip(t, map[string]string{ + "assets/secret.txt": secret, + }) + + run := func(data []byte) (string, error) { + readJSON, writeJSON := io.Pipe() + chunksChan := make(chan *sources.Chunk, 16) + + var workerError error + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + defer close(chunksChan) + ctx := context.WithLogger(t.Context(), logr.Discard()) + source := &Source{} + workerError = source.chunkJSONEnumeratorReader(ctx, readJSON, chunksChan) + }() + + // No filename is set on the record; routing is content-based only. + enc := json.NewEncoder(writeJSON) + require.NoError(t, enc.Encode(&jsonEntry{Metadata: makeRawMessage(t, "{}"), Data: data})) + require.NoError(t, writeJSON.Close()) + + found := "" + for chunk := range chunksChan { + found += string(chunk.Data) + } + wg.Wait() + return found, workerError + } + + foundAPK, err := run(apkZip) + require.NoError(t, err) + assert.NotContains(t, foundAPK, secret, "APK-content archive should be routed to the APK handler") + + foundZip, err := run(plainZip) + require.NoError(t, err) + assert.Contains(t, foundZip, secret, "non-APK zip should be handled generically and surface the secret") +}