feat: block out of bounds symlinks (#9738) #9843
Conversation
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
5974f4f
to
181c492
Compare
|
Hah. It works SO good, it broke another test because it errored out complaining about the symlinks! Gotta figure out how to fix this.. |
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
181c492
to
47da688
Compare
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
6 times, most recently
from
4cbb79d
to
c32e586
Compare
Codecov Report
@@ Coverage Diff @@
## master #9843 +/- ##
==========================================
- Coverage 45.86% 45.78% -0.08%
==========================================
Files 227 227
Lines 26862 27005 +143
==========================================
+ Hits 12320 12364 +44
- Misses 12862 12954 +92
- Partials 1680 1687 +7
Continue to review full report at Codecov.
|
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
4 times, most recently
from
d91a437
to
25ae38b
Compare
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
25ae38b
to
99de448
Compare
|
You gotta quit putting up PRs so fast, I can't review them all! 😆 But for real, thanks for working on this.
lol, I'm not surprised. Hopefully you were able to sort it. A bunch of tests use relative paths, when the code really should only accept absolute paths. |
Sorry! I saw a low-hanging fruit, had a slow day, and I love security. 😆
Seems I've broken an E2E test.. Not entirely sure how since I'm not seeing my error. I have absolutely no idea how to dig into that yet but I'll figure it out. Is it OK if I DM you on the CNCF Slack every once in a while for assistance? |
util/app/path/path.go
Outdated
| return nil | ||
| }) | ||
| if found != "" && err.Error() == outOfBoundsError { | ||
| return true, found, nil |
There was a problem hiding this comment.
Can you add a log.Warn? Out-of-bounds symlinks are suspicious, so it'd be nice to give admins a way to monitor them.
There was a problem hiding this comment.
The log.Warn is in runRepoOperation 😄
Should I move it to the path function?
Sure! Generally I don't have time for support DMs, but I can always make time for contribution DMs. :-) |
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
2 times, most recently
from
87a41c2
to
59c6625
Compare
|
Okay, refactored how the data is returned from the helper by using custom errors that carry the filename. Also added log fields and a repository integration test for out of bounds symlinks (instead of just out-of-bounds values files). Fixed the issue with files that contain |
|
Hm. Is there an e2e test with a symlink up to the base repo dir? |
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
2 times, most recently
from
24ccb83
to
3bf91c5
Compare
|
Added another integration test into |
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
3bf91c5
to
7655fb4
Compare
|
I'm considering whether we should move the goalposts just a little. What if, along with preventing the symlink target from being outside the root dir, we prevented any part of the symlink path from ever leaving the root dir. This would prevent an attacker from being able to brute-force (or otherwise guess) the root dir path. For example, consider this directory structure. I believe the current check would allow this symlink to pass, because its eventual destination does not lie outside the root dir. But the passing check allows the attacker to confirm that they've correctly guessed some external path. Then the attacker could start working on a different path traversal to get to the now-known external directory. Granted, all the sensitive paths should be 128 bits of cryptographically secure entropy. But this would be an extra layer of protection in case of a particularly persistent adversary or a CSPRNG flaw. I think the solution would involve splitting the link path and then reconstructing it, checking for an out-of-bounds error at each step. It would be all string manipulation operations (no filesystem work), so should be reasonably fast. |
|
But if we want to save that work for another time, that's understandable. 😆 |
|
I had to think about this one for a minute, but you're right! Even though the ultimate target can't be outside of the repository, the intermediate directories in the link must exist or it will fail, thus giving a way to determine if a path exists. You're really good at thinking of these edge cases. 😄 I also seem to have introduced a flaky test.. Charts are switching spots. Bah. |
Comes from pondering about 6 CVEs related to path traversal. 😆 I'll take a look at the flaky tests in a bit. Being lazy on the weekend so far and only thinking of ways to make things more difficult rather than more easy. 😉 |
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
7655fb4
to
50da588
Compare
|
I think this'll do the trick! It now traverses each step of the symlink to make sure it never leaves the bounds of the repo. Still gotta figure out that flaky test, though. I added a unit test specifically to check this ( |
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
3 times, most recently
from
8601902
to
48b3c8d
Compare
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
2 times, most recently
from
6b09ef4
to
86caf5d
Compare
|
Okay, flaky test should be fixed. Ran it 100x locally. I think that's everything! |
There was a problem hiding this comment.
I'm going to give this one last pass before smashing merge. But I'm 99.9% sure it's 100% awesome. :-)
Would you kindly add docs/operator-manual/upgrading/2.4-2.5.md and include a note about this change? Ideally the note would explain that we're trying to prevent 1) out-of-bounds symlinks that might be missed by other checks and 2) leaking any information about the presence or absence of files or directories outside the app's repo.
Also feel free to say "naw I'm busy," and I'll add the note later. :-)
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
86caf5d
to
005598f
Compare
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
notfromstatefarm
force-pushed
the
feat/block-out-of-bounds-symlinks
branch
from
005598f
to
ae22f98
Compare
| @@ -318,6 +319,22 @@ func (s *Service) runRepoOperation( | |||
| return err | |||
| } | |||
| defer io.Close(closer) | |||
| if !s.initConstants.AllowOutOfBoundsSymlinks { | |||
| err := argopath.CheckOutOfBoundsSymlinks(chartPath) | |||
There was a problem hiding this comment.
just cosmetic change , i think it would be nice move this block to some dedicate function, so you will not need duplicate it fully in bottom
There was a problem hiding this comment.
Gonna go ahead and merge this, and @notfromstatefarm can follow up with another PR if this can be DRY'ed out.
Signed-off-by: notfromstatefarm 86763948+notfromstatefarm@users.noreply.github.com
Checklist:
Fixes #9738
This PR implements @crenshaw-dev's idea of traversing the repository for symlinks and blocking further processing if any are found to traverse outside of the directory, even if the final target is within it. The check can be disabled via the repo server argument
allow-oob-symlinks.I believe this should cover all our bases since it is implemented inside
runRepoOperation. It will print to the console whenever any are found.I had to change many of the repository tests because they have the root of the mock repo being the root of the Argo repository. This meant that any time it 'fetched' the repository, it was finding the test files I created for the unit tests and thereby failing as it should.
Creating new app:
Refreshing an app that had bad symlinks added to it:
