Metadata structure verification (e.g. path traverse) cleanup/strengthening #1412
Conversation
This file contains 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
I was reading about a recent security issue with both EMC and VMWare: https://arstechnica.com/information-technology/2018/01/emc-vmware-security-bugs-throw-gasoline-on-cloud-security-fire/ It's a classic path traversal problem, and that made me think more about our handling of this in libostree. Fortunately of course, not being new to this rodeo, long ago I *did* consider path traversal. Inside the pull code, we call `ot_util_filename_validate()`. Also, `fsck` does this too. I have further followups here, but let's add some test cases for this. I crafted a repository with a `../` in a dirtree object by patching libostree to inject it, and that's included as a tarball. This patch covers the two cases where we do already have checks; pulling via HTTP, and in `fsck`.
While we do protect against path traversal during pull, let's also validate during checkout; it's a cheap operation and provides good last-mile protection.
Previously we were doing e.g. `ot_util_filename_validate()` specifically inline in dirtree objects, but only *after* writing them into the staging directory (by default). In (non-default) cases such as not using a transaction, such an object could be written directly into the repo. A notable gap here is that `pull-local --untrusted` was *not* doing this verification, just checksums. We harden that (and also the static delta writing path, really *everything* that calls `ostree_repo_write_metadata()` to also do "structure" validation which includes path traversal checks. Basically, let's try hard to avoid having badly structured objects even in the repo. One thing that sucks in this patch is that we need to allocate a "bounce buffer" for metadata in the static delta path, because GVariant imposes alignment requirements, which I screwed up and didn't fulfill when designing deltas. It actually didn't matter before because we weren't parsing them, but now we are. In theory we could check alignment but ...eh, not worth it, at least not until we change the delta compiler to emit aligned metadata which actually may be quite tricky. (Big picture I doubt this really matters much right now but I'm not going to pull out a profiler yet for this) The pull test was extended to check we didn't even write a dirtree with path traversal into the staging directory. There's a bit of code motion in extracting `_ostree_validate_structureof_metadata()` from `fsck_metadata_object()`. Then `_ostree_verify_metadata_object()` builds on that to do checksum verification too.
|
This one should be good to go. Just a Travis flake downloading debs. |
|
Yup, this look good to me! |
|
⚡ Test exempted: pull fully rebased and already tested. |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jan 12, 2018
While we do protect against path traversal during pull, let's also validate during checkout; it's a cheap operation and provides good last-mile protection. Closes: #1412 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jan 12, 2018
Previously we were doing e.g. `ot_util_filename_validate()` specifically inline in dirtree objects, but only *after* writing them into the staging directory (by default). In (non-default) cases such as not using a transaction, such an object could be written directly into the repo. A notable gap here is that `pull-local --untrusted` was *not* doing this verification, just checksums. We harden that (and also the static delta writing path, really *everything* that calls `ostree_repo_write_metadata()` to also do "structure" validation which includes path traversal checks. Basically, let's try hard to avoid having badly structured objects even in the repo. One thing that sucks in this patch is that we need to allocate a "bounce buffer" for metadata in the static delta path, because GVariant imposes alignment requirements, which I screwed up and didn't fulfill when designing deltas. It actually didn't matter before because we weren't parsing them, but now we are. In theory we could check alignment but ...eh, not worth it, at least not until we change the delta compiler to emit aligned metadata which actually may be quite tricky. (Big picture I doubt this really matters much right now but I'm not going to pull out a profiler yet for this) The pull test was extended to check we didn't even write a dirtree with path traversal into the staging directory. There's a bit of code motion in extracting `_ostree_validate_structureof_metadata()` from `fsck_metadata_object()`. Then `_ostree_verify_metadata_object()` builds on that to do checksum verification too. Closes: #1412 Approved by: jlebon
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.
I was reading about a recent security issue with both EMC and VMWare:
https://arstechnica.com/information-technology/2018/01/emc-vmware-security-bugs-throw-gasoline-on-cloud-security-fire/
It's a classic path traversal problem, and that made me think more about our
handling of this in libostree. Fortunately of course, not being new to this
rodeo, long ago I did consider path traversal. Inside the HTTP pull code, we call
ot_util_filename_validate(). Also,fsckdoes this too.The first patch just adds tests for those.
So...IMO, we squeak by here and I have no plans to request a CVE; even if there were path traversal practically speaking it mostly only matters for flatpak (maybe buildstream?), or people downloading untrusted ostree content not for the bootable OS. (Someone able to provide you malicious OS content can do whatever they want on the next boot)
That said though even for "OS ostrees" there are use cases for e.g. checking out subtrees as non-root outside of the deployment.
So I went through the code and added a lot of hardening for this; specifically
pull-local --untrustedandcheckout.