-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix GPG signature check when decompressing gzipped source (CVE-2021-20319) #655
Merged
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
It's only a convenience method and it makes the API less clear. While we're here, add proper buffering in check_image_and_sig().
The tests currently send GPG verification results to stderr, which will be fixed up in a later commit.
Capture GPG's stderr and forward it to our stderr when checking GPG's exit status. If GpgReader is dropped without checking the exit status, squash the stderr output. This avoids implying that we've validated the signature when we haven't, and also avoids spurious verification errors if we fail a download or install for unrelated reasons. Use eprint!() instead of io::stderr() to forward the output so that the output will be automatically squashed in successful test cases.
Under normal conditions, GzDecoder doesn't read EOF from the underlying source; it stops reading as soon as it reaches the gzip trailer. Since the wrapped GpgReader doesn't see EOF, it doesn't check the exit status of GPG, and a bad signature will not be noticed. XzDecoder is not affected. This allows bypass of signature verification under uncommon circumstances. Notably, installing from a live ISO or PXE image uses either osmet images (Fedora CoreOS or RHEL CoreOS) or a full copy of the install image (RHEL for Edge), and these are not affected because they're trusted (they come from the installation media we've booted from). These flows are affected: 1. Installing with --image-file, --image-url, or coreos.inst.image_url. For example, if a user has a local mirror of installation images, an attacker could replace an image with a gzip-compressed alternative (even if the file extension is .xz). The result: $ coreos-installer install --image-url http://localhost:8080/image.xz /dev/loop0 Downloading image from http://localhost:8080/image.xz Downloading signature from http://localhost:8080/image.xz.sig > Read disk 749.9 MiB/749.9 MiB (100%) gpg: Signature made Mon 20 Sep 2021 02:41:50 PM EDT gpg: using RSA key 8C5BA6990BDB26E19F2A1A801161AE6945719A39 gpg: BAD signature from "Fedora (34) <fedora-34-primary@fedoraproject.org>" [ultimate] Install complete. GPG still complains when its stdin is closed, but coreos-installer doesn't notice. Automation that relies on coreos-installer's exit status will not notice either. 2. `coreos-installer download --decompress --image-url`: $ coreos-installer download --decompress --image-url http://localhost:8080/image.xz > Read disk 749.9 MiB/749.9 MiB (100%) gpg: Signature made Mon 20 Sep 2021 02:41:50 PM EDT gpg: using RSA key 8C5BA6990BDB26E19F2A1A801161AE6945719A39 gpg: BAD signature from "Fedora (34) <fedora-34-primary@fedoraproject.org>" [ultimate] ./image Again, coreos-installer exits 0. 3. Installing with default parameters, when not using live install media with osmet, if the Red Hat-controlled S3 bucket is compromised or the HTTPS connection is successfully MITMed. 4. `coreos-installer download --decompress` if the S3 bucket is compromised or the HTTPS connection is MITMed. Fix this by having callers explicitly check the signature verification result after the fetch pipeline reaches EOF. This is easiest to do within the mutable borrow rules by adding a new VerifyReader which wraps either GpgReader if signatures are being checked, or the underlying source otherwise. To ensure that the explicit check is done consistently, drop the implicit one at EOF, so that it'll be clear from a missing GPG report on stderr that the API was not used correctly.
In cases where we're decompressing from a BufReader, use the bufread versions of the decompressors to prevent them from wrapping the input in another BufReader internally.
Under normal conditions, GzDecoder doesn't read EOF from the underlying source; it stops reading as soon as it reaches the gzip trailer. As a result, trailing garbage will not be noticed. Fix this by trying to read one more byte from the underlying source and failing if we get one. Requires the previous commit. Bug discovered by @raballew; thanks!
When doing the early check to see whether we already have an artifact, report GPG verification success (which is relevant) but not failure (which isn't relevant, since we'll redownload in that case).
lucab
approved these changes
Oct 11, 2021
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.
Under normal conditions,
GzDecoder
doesn't read EOF from the underlying source; it stops reading as soon as it reaches the gzip trailer. Since the wrappedGpgReader
doesn't see EOF, it doesn't check the exit status of GPG, and a bad signature will not be noticed.XzDecoder
is not affected.This allows bypass of signature verification under uncommon circumstances. Notably, installing from a live ISO or PXE image uses either osmet images (Fedora CoreOS or RHEL CoreOS) or a full copy of the install image (RHEL for Edge), and these are not affected because they're trusted (they come from the installation media we've booted from). These flows are affected:
Installing with
--image-file
,--image-url
, orcoreos.inst.image_url
. For example, if a user has a local mirror of installation images, an attacker could replace an image with a gzip-compressed alternative (even if the file extension is.xz
). The result:GPG still complains when its stdin is closed, but coreos-installer doesn't notice. Automation that relies on coreos-installer's exit status will not notice either.
coreos-installer download --decompress --image-url
:Again, coreos-installer exits 0.
Installing with default parameters, when not using live install media with osmet, if the Red Hat-controlled S3 bucket is compromised or the HTTPS connection is successfully MITMed.
coreos-installer download --decompress
if the S3 bucket is compromised or the HTTPS connection is MITMed.Apply several fixes to correct this behavior:
Instead of letting GPG inherit stderr from coreos-installer, capture GPG's stderr output. Forward that output to coreos-installer's stderr only when we actually check the exit status, and not if
GpgReader
is dropped without checking the result. This prevents the affected case, or any similar future one, from producing output that makes it appear that the signature was successfully checked. It also prevents producing spurious verification errors if if a download or install fails for unrelated reasons.Remove the implicit signature verification check at EOF, and instead explicitly check the verification result with a separate method call. Due to the previous fix, any failure to use this API correctly will produce an obvious missing verification report on stderr.
Have
DecompressReader
fail if additional bytes are available from the underlying source afterGzDecoder
reaches EOF. This is not security-critical after the previous fix, but it ensures we correctly fail if a gzip stream contains trailing garbage.Add unit tests for signature verification and for detection of bad signatures in the fetch pipeline.
As a UX improvement, stop producing a spurious verification error when
coreos-installer download
restarts an incomplete download left over from a previous run.Bug discovered by @raballew; thanks!