Skip to content
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

Ensure prefix check for path traversal has path separator #223

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

wagoodman
Copy link
Contributor

Reported by @a1loy , it is possible to specify a tar header that matches the random destination path as a prefix, but additionally has more elements as a suffix. In these cases we will still write out to a stereoscope temp directory that is cleaned up (thus this is not a security concern), but could allow for unintentional writes within that temp directory. This addresses the bug by explicitly performing a prefix check with a path separator.

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman requested a review from a team February 16, 2024 14:52
@wagoodman wagoodman self-assigned this Feb 16, 2024
Copy link

Benchmark Test Results

Benchmark results from the latest changes vs base branch
latest: Pulling from library/ubuntu
tar: Option --mtime: Treating date 'UTC 2019-09-16' as 2019-09-16 00:00:00
goos: linux
goarch: amd64
pkg: github.com/anchore/stereoscope/pkg/file
cpu: AMD EPYC 7763 64-Core Processor                
ctr: 
           │ ./.tmp/benchmark-2e4783f.txt │
           │            sec/op            │
TarIndex-2                   35.02µ ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

           │ ./.tmp/benchmark-2e4783f.txt │
           │             B/op             │
TarIndex-2                  5.560Ki ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

           │ ./.tmp/benchmark-2e4783f.txt │
           │          allocs/op           │
TarIndex-2                    93.00 ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

pkg: github.com/anchore/stereoscope/test/integration
                                      │ ./.tmp/benchmark-2e4783f.txt │
                                      │            sec/op            │
SimpleImage_GetImage/docker-archive-2                   1.276m ± ∞ ¹
SimpleImage_GetImage/podman-2                           18.67m ± ∞ ¹
geomean                                                 4.881m
¹ need >= 6 samples for confidence interval at level 0.95

                                      │ ./.tmp/benchmark-2e4783f.txt │
                                      │             B/op             │
SimpleImage_GetImage/docker-archive-2                  329.7Ki ± ∞ ¹
SimpleImage_GetImage/podman-2                          435.1Ki ± ∞ ¹
geomean                                                378.7Ki
¹ need >= 6 samples for confidence interval at level 0.95

                                      │ ./.tmp/benchmark-2e4783f.txt │
                                      │          allocs/op           │
SimpleImage_GetImage/docker-archive-2                   2.789k ± ∞ ¹
SimpleImage_GetImage/podman-2                           2.753k ± ∞ ¹
geomean                                                 2.771k
¹ need >= 6 samples for confidence interval at level 0.95

ctr: failed to dial "/run/containerd/containerd.sock": connection error: desc = "transport: error while dialing: dial unix /run/containerd/containerd.sock: connect: permission denied"
                                                   │ ./.tmp/benchmark-2e4783f.txt │
                                                   │            sec/op            │
SimpleImage_FetchSquashedContents/docker-archive-2                   18.13µ ± ∞ ¹
SimpleImage_FetchSquashedContents/podman-2                           18.17µ ± ∞ ¹
geomean                                                              18.15µ
¹ need >= 6 samples for confidence interval at level 0.95

                                                   │ ./.tmp/benchmark-2e4783f.txt │
                                                   │             B/op             │
SimpleImage_FetchSquashedContents/docker-archive-2                  2.648Ki ± ∞ ¹
SimpleImage_FetchSquashedContents/podman-2                          2.648Ki ± ∞ ¹
geomean                                                             2.648Ki
¹ need >= 6 samples for confidence interval at level 0.95

                                                   │ ./.tmp/benchmark-2e4783f.txt │
                                                   │          allocs/op           │
SimpleImage_FetchSquashedContents/docker-archive-2                    21.00 ± ∞ ¹
SimpleImage_FetchSquashedContents/podman-2                            21.00 ± ∞ ¹
geomean                                                               21.00
¹ need >= 6 samples for confidence interval at level 0.95

@wagoodman wagoodman added the bug Something isn't working label Feb 16, 2024
@wagoodman wagoodman enabled auto-merge (squash) February 16, 2024 16:05
@wagoodman wagoodman merged commit 705198d into main Feb 16, 2024
7 checks passed
@wagoodman wagoodman deleted the check-traversal-with-suffix branch February 16, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants