CVE-2021-0561 in stream_encoder.c

[diabonas]

According to latest Pixel Update Bulletin—June 2021 there is an information disclosure security issue CVE-2021-0561 in stream_encoder.c:

In append_to_verify_fifointerleaved of stream_encoder.c, there is a possible out of bounds write due to a missing bounds check. This could lead to local information disclosure with no additional execution privileges needed. User interaction is not needed for exploitation. [...]

Should the following downstream patch in Android applied upstream as well?

diff --git a/libFLAC/stream_encoder.c b/libFLAC/stream_encoder.c
index 037b8cb..b0b2650 100644
--- a/libFLAC/stream_encoder.c
+++ b/libFLAC/stream_encoder.c

@@ -2578,7 +2578,9 @@
            encoder->private_->verify.needs_magic_hack = true;
        }
        else {
-           if(!FLAC__stream_decoder_process_single(encoder->private_->verify.decoder)) {
+           if(!FLAC__stream_decoder_process_single(encoder->private_->verify.decoder)
+               || (!is_last_block
+                   && (FLAC__stream_encoder_get_verify_decoder_state(encoder) == FLAC__STREAM_DECODER_END_OF_STREAM))) {
                FLAC__bitwriter_release_buffer(encoder->private_->frame);
                FLAC__bitwriter_clear(encoder->private_->frame);
                if(encoder->protected_->state != FLAC__STREAM_ENCODER_VERIFY_MISMATCH_IN_AUDIO_DATA)

[rillian]

Thanks for pointing this out!

[NotTsunami]

@NeelkamalSemwal Do you think https://android.googlesource.com/platform/external/flac/+/368eb3f5bec249a197c95a95583ff8153aa6a87f and/or https://android.googlesource.com/platform/external/flac/+/027e439d519a341d233a337fde415245ea4538b0 should be cherry-picked to upstream?

[NeelkamalSemwal]

Yes, since they are already merged in the AOSP tree, we can get them cherry-picked to upstream. Creating pull requests for the same.

[half-duplex]

(#259 and #260)

[rillian]

This should be resolved by #259.

[mlichvar]

Is there more information about this issue, like does it impact the flac program, or only 3rd party applications of the libFLAC library?

The patch adds a new condition for the verifying decoder (checking data that was just encoded) to consider some error that was previously ignored. But there normally shouldn't be any errors unless there is a HW problem or a bug in the encoder. Is this related to some other issue that was fixed recently?

[mlichvar]

FWIW, I can reproduce the crash with modified src/flac/encode.c or examples/c/encode/file/main.c to call FLAC__stream_encoder_set_total_samples_estimate() with a smaller number of samples than what will be provided, so this could impact (buggy?) 3rd party applications, but I'm still not sure if unmodified flac is impacted.

[ktmf01]

Is there more information about this issue, like does it impact the flac program, or only 3rd party applications of the libFLAC library?

I couldn't find any more information. Digging through the upstream (Android) source, it seems viewing bug reports (still) requires elevated privileges.

[NeelkamalSemwal]

According to the bug report: " In append_to_verify_fifointerleaved of stream_encoder.c, there is a possible out of bounds write due to a missing bounds check. This could lead to local escalation of privilege with no additional execution privileges needed. User interaction is not needed for exploitation."

[mlichvar]

That doesn't explain how it happens.

The case I found triggers the end-of-stream in the verifying decoder when the number of decoded samples reaches the encoder's estimate set by FLAC__stream_encoder_set_total_samples_estimate(), but there is actually more input provided to the encoder. This might be a bug in the Android application, but not in the flac program.

Another question is whether the decoder shouldn't ignore the encoder's estimate. If the estimate was low, we would still want to encode the whole input, right?

[ktmf01]

The case I found triggers the end-of-stream in the verifying decoder when the number of decoded samples reaches the encoder's estimate set by FLAC__stream_encoder_set_total_samples_estimate(), but there is actually more input provided to the encoder. This might be a bug in the Android application, but not in the flac program.

Well, setting a wrong estimate should not trigger problematic behaviour, right? The problem here (I think, didn't check) is that the estimate is written to streaminfo, which is taken up by the verifying decoder and then no longer considered an estimate by the decoder.

Another question is whether the decoder shouldn't ignore the encoder's estimate. If the estimate was low, we would still want to encode the whole input, right?

Good call, I noticed that bit before but didn't think of the verifying process.

So, presumably removing this bit of code also fixes CVE-2021-0561?

https://github.com/xiph/flac/blob/2610594d04e6deb5c9bc0c8c7f620769c0c1149a/src/libFLAC/stream_decoder.c#L1966-L1973

[ktmf01]

Okay, so I just checked this

On encoding, the estimate is written to streaminfo https://github.com/xiph/flac/blob/2610594d04e6deb5c9bc0c8c7f620769c0c1149a/src/libFLAC/stream_encoder.c#L1208

The updated amount never reaches the verifying decoder, and because of the code mentioned in my previous post, the decoder stops before reaching the end of the file.

[mlichvar]

Well, setting a wrong estimate should not trigger problematic behaviour, right? The problem here (I think, didn't check) is that the estimate is written to streaminfo, which is taken up by the verifying decoder and then no longer considered an estimate by the decoder.

Right.

So, presumably removing this bit of code also fixes CVE-2021-0561?

It would, assuming there is no other way to trigger the end-of-stream, but that doesn't look like a good fix. I think the encoder should have a way to tell the decoder to ignore the value.

[ktmf01]

but that doesn't look like a good fix

I don't see why this isn't a good fix. Removing this bit of code is actually on my PR-todo-list for another reason.

Assume we get a file where the total number of samples in the streaminfo metadata block differs from the actual total number of samples in the file. Such a file is invalid, but they inevitably exist, either from setting a wrong estimate when writing to a pipe (total samples cannot be updated in that case), a bug in some software, bit flip etc. In that case, I'd rather have the decoder take the actual number of samples in the file and not the samples specified in the streaminfo block. The latter is more prone to corruption/bit flip, as it has no checksumming and no redundancy.

edit: in other words, I'd consider the streaminfo block informational (something for a program to print somewhere, or calculate the length of the file in seconds with), not something to decide when to stop decoding.

[mlichvar]

My concern was "wasting time trying to sync on an ID3V1 tag" from the comment above that code, but you make a point. It makes sense to make it symmetric. If the encoder can write unreliable data, the decoder should be able to deal with it.

[ktmf01]

As using a ID3V1 tag has been always been strongly discouraged and such a tag is only 128 bytes (355 bytes extended) long, I think the performance loss here is negligible.

Anyway, thanks for digging into this. I didn't give this fix much thought but your analysis that it actually masks a problem elsewhere is on point, as is the finding that the wrong number-of-samples estimate is the root cause.