common: fix path containment corner cases in passdriver#854
Conversation
|
Packit jobs failed. @containers/packit-build please check. |
With the current implementation of getPath, it's possible to return a path outside the root directory, matching a sibling directory instead through a crafted/specific id. Signed-off-by: Caleb Xu <caxu@redhat.com>
caxu-rh
force-pushed
the
passdriver-path-containment
branch
from
8c11bfb to
709c573
Compare
| gpghomedir := filepath.Join(tmpdir, "gpg") | ||
| require.NoError(t, os.MkdirAll(base, 0o755)) | ||
| require.NoError(t, os.MkdirAll(sibling, 0o755)) | ||
| require.NoError(t, os.MkdirAll(gpghomedir, 0o755)) |
There was a problem hiding this comment.
(It would have been simpler to leave this as t.TempDir().)
There was a problem hiding this comment.
I could move gpghomedir into its own t.TempDir(), but sibling needs to be named after base. It helps to have the base name well-known as well, so that the key can just be ../storex/... rather than needing to dynamically build the key based on the t.TempDir() name. I also wanted to make sure sibling gets cleaned up, hence not putting just manually mkdir'ing sibling after the t.TempDir() creation.
(Then again, I don't think any of the tests actually depend on sibling dir to exist, so maybe we could just skip the creation altogether, but oh well. Happy to simplify once more, if you think its worth it.)
There was a problem hiding this comment.
I’m sorry, I did mean the gpghomedir one only.
Anyway, this is fine as is, let’s wait for a second reviewer.
|
Thanks for the feedback. Yes, I was hoping to also cover the non-absolute/symlink root cases hence the more convoluted fix. Switched to the simpler approach since it sounds like that's not a concern for now. I tried to combine the test setup logic so that the individual cases can be covered in the other tests in that file. Might be a bit clunky, let me know if you'd prefer that to be handled differently. |
2 participants
With the current implementation of getPath, it's possible to return a path outside the root directory, matching a sibling directory instead through a crafted/specific id.
For example: with a root directory of
/tmp/root, an id of../rootx/foocould yield an invalid path of/tmp/rootx/foo.idis random/not user-controlled (as far as I can tell), so I consider this more to be for the benefit of hardening/correctness (arguably, if we trustid, we don't need thestrings.HasPrefixcheck either, since we just constructed it).