-
Notifications
You must be signed in to change notification settings - Fork 734
chore: migrate syft to use mholt/archives instead of anchore fork #4029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| cleanupFn := func() error { | ||
| return os.RemoveAll(tempDir) | ||
| visitor := func(_ context.Context, file archives.FileInfo) error { | ||
| destPath, err := intFile.SafeJoin(tempDir, file.NameInArchive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this to use the same kind of SafeJoin functionality so we don't escape the tmpDir like we do in other parts of syft. If the archives library already handles this internally apologies I just couldn't find it on a quick inspection.
|
@wagoodman I've looked at this one and added a small protection. The visitor now uses a path aware directory join and cannot write outside the temp directory. Also, I don't know what Static Analysis is on about here. I've checked out this branch even on a different machine and it's told me the go.mod and go.sum are tidy locally |
spiffcs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one now looks good to me after a first pass review. I identified a section in the visitor where we might be able to escape and write files to paths outside the temp directory so added a fix for this. I'd like additional 👀 from someone on @anchore/tools to check my work.
one more change is needed here where we also protect against symlink attacks
Signed-off-by: Kudryavcev Nikolay <[email protected]>
Signed-off-by: Kudryavcev Nikolay <[email protected]>
Signed-off-by: Kudryavcev Nikolay <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
923d9a8 to
cfad5bb
Compare
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Angelo Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
kzantow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM the SafeJoin usage I think is good 👍 main things I think we should update:
- don't ever need to log.Error close problems, there's a CloseAndLogError utility to simplify a bunch of those calls anyway
- the tests don't seem to be actually testing the function we want to verify: UnzipToDir
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Angelo Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Description
Rewritten deprecated fork github.com/anchore/archiver to github.com/mholt/archives
Type of change
Checklist: