Repository navigation
fix(media): keep a tool kit's archive entries inside its folder on Windows too - #112
Merged
Merged
Conversation
…ndows too untarGz refused entries that were absolute or climbed out with ../, judged by slashes alone. Windows reads a backslash as a separator too, so an entry named ..\evil passed the check and filepath.Join put it beside the kit's folder. The check is now filepath.IsLocal on the name as this system reads it, which also refuses drive letters and reserved device names on Windows. The kits are pinned by SHA-256, so only a tampered release built into Kando could have done this. CodeQL's go/zipslip alert on toolkit.go pointed at the code, as it does not count the hand-written check as a sanitizer.
github-actions
Bot
added
area: media
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the open code scanning alert, CodeQL
go/zipsliponpackages/media/toolkit.go: "Unsanitized archive entry, which may contain '..', is used in a file system operation."CodeQL flags the code because it doesn't treat the hand-written check as a sanitizer, and on Windows that check really did have a hole:
untarGzrefused entries that were absolute or started with../, but it judged them by slashes only...\evilpassed the check, andfilepath.Jointhen wrote it next to the kit's folder instead of inside it.The check is now
filepath.IsLocalon the name as the current system reads it. On Windows that also refuses drive letters and reserved device names such asNUL.How exposed this was: kits are pinned by SHA-256, so only a tampered archive that matched a pinned checksum could have used this hole.
Test plan
TestInstallKitRejectsUnsafeArchivesadds Windows-only cases:..\evil,bin\..\..\evil,C:\evilandC:evil. On other systems these are ordinary file names...\evilwould be extracted outside the folder, and the test would fail with "ffmpeg is not in the download" instead oferrUnsafeEntry.go test ./packages/media/passes locally, andgo vetpasses for darwin, linux and windows. The Windows cases run in CI's Windows job.Screenshots
None.