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

[release-0.10] io: check for EOF when decoding a gzip stream (CVE-2021-20319) #659

Merged
merged 4 commits into from Oct 11, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Prev
io: check for EOF when decoding a gzip stream (CVE-2021-20319)
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 reading one more byte from the source after GzDecoder returns
EOF.  If that also returns EOF, great, and we've given the source an
opportunity to do its own EOF processing.  If not, fail, because the stream
contains trailing garbage.

Also add unit tests for signature verification and for detection of bad
signatures in the fetch pipeline.

Requires the previous commit.

Bug discovered by @raballew; thanks!
  • Loading branch information
bgilbert committed Oct 11, 2021
commit ad243c6f0eff2835b2da56ca5f7f33af76253c89
Binary file added fixtures/verify/1M.gz
Binary file not shown.
Binary file added fixtures/verify/1M.gz.sig
Binary file not shown.
5 changes: 5 additions & 0 deletions src/download.rs
Expand Up @@ -477,6 +477,11 @@ mod tests {
&include_bytes!("../fixtures/verify/1M.sig")[..],
&[0; 1 << 20][..],
);
test_one_signed_file(
&include_bytes!("../fixtures/verify/1M.gz")[..],
&include_bytes!("../fixtures/verify/1M.gz.sig")[..],
&[0; 1 << 20][..],
);
test_one_signed_file(
&include_bytes!("../fixtures/verify/1M.xz")[..],
&include_bytes!("../fixtures/verify/1M.xz.sig")[..],
Expand Down
46 changes: 44 additions & 2 deletions src/io.rs
Expand Up @@ -196,7 +196,23 @@ impl<R: BufRead> Read for DecompressReader<R> {
use CompressDecoder::*;
match &mut self.decoder {
Uncompressed(d) => d.read(buf),
Gzip(d) => d.read(buf),
Gzip(d) => {
let count = d.read(buf)?;
if count == 0 {
// GzDecoder stops reading as soon as it encounters the
// gzip trailer, so it doesn't notice trailing data,
// which indicates something wrong with the input. Try
// reading one more byte, and fail if there is one.
let mut buf = [0; 1];
if d.get_mut().read(&mut buf)? > 0 {
return Err(io::Error::new(
ErrorKind::InvalidData,
"found trailing data after compressed gzip stream",
));
}
}
Ok(count)
}
Xz(d) => d.read(buf),
}
}
Expand Down Expand Up @@ -249,7 +265,7 @@ impl<R: Read> Read for LimitReader<R> {
#[cfg(test)]
mod tests {
use super::*;
use std::io::Cursor;
use std::io::{BufReader, Cursor};

#[test]
fn test_ignition_hash_cli_parse() {
Expand Down Expand Up @@ -419,4 +435,30 @@ mod tests {
"collision with foo at offset 50"
);
}

/// Test that DecompressReader fails if data is appended to the
/// compressed stream.
#[test]
fn test_decompress_reader_trailing_data() {
test_decompress_reader_trailing_data_one(&include_bytes!("../fixtures/verify/1M.gz")[..]);
test_decompress_reader_trailing_data_one(&include_bytes!("../fixtures/verify/1M.xz")[..]);
}

fn test_decompress_reader_trailing_data_one(input: &[u8]) {
let mut input = input.to_vec();
let mut output = Vec::new();

// successful run
DecompressReader::new(BufReader::new(&*input))
.unwrap()
.read_to_end(&mut output)
.unwrap();

// add trailing garbage, make sure we notice
input.push(0);
DecompressReader::new(BufReader::new(&*input))
.unwrap()
.read_to_end(&mut output)
.unwrap_err();
}
}