'quic::HeaderProtectionKey::new_mask' behaves differently depending on the version of ring

[MikeRomaniuk]

Hi there!

During the fuzzing of a project I'm working on, I encountered a crash that led me here.

The function quic::HeaderProtectionKey::new_mask is returning an error when it shouldn't. As I understand it, new_mask should work fine when the input data is 16 bytes long. This issue is reproducible starting from version 0.17.1, but everything works fine in version 0.16.20.

Is this intended behavior?

One more note: quic::CHACHA20 works fine regardless of the version. So the problem is in the AES_128 and AES_256.

I've created a sample fuzz target:

#![no_main]  
  
use libfuzzer_sys::fuzz_target;  
use arbitrary::Arbitrary;  
use ring::aead::{quic, MAX_TAG_LEN};  
  
#[derive(Arbitrary, Debug)]  
pub enum Algo {  
    Aes128,  
    Aes256,  
    ChaCha20,  
}  
  
impl Algo {  
    fn to_algorithm(&self) -> &'static quic::Algorithm {  
        match self {  
            Algo::Aes128 => &quic::AES_128,  
            Algo::Aes256 => &quic::AES_256,  
            Algo::ChaCha20 => &quic::CHACHA20,  
        }    
    }
}  
  
#[derive(Arbitrary, Debug)]  
struct Case {  
    data: [u8; MAX_TAG_LEN],  
    algo: Algo,  
}  
  
fuzz_target!(|case: Case| {  
    let algo = case.algo.to_algorithm();  
    let key_len = algo.key_len();  
    let key_data = vec![0u8; key_len];  
  
    let key = quic::HeaderProtectionKey::new(&algo, &key_data).expect("protection key creation failed");  
  
    assert!(key.new_mask(&case.data).is_ok());  
});

Thank you in advance for your time.

[briansmith]

The documentation says:

    /// Generate a new QUIC Header Protection mask.
    ///
    /// `sample` must be exactly `self.algorithm().sample_len()` bytes long.

[EDIT: Removed incorrect comment.]

[MikeRomaniuk]

Thanks for answering.

I still do not understand why I have gotten an error.

In particular, I am not getting why ChaCha20 works and AES256 does not, while their key lengths are identical.

And one more aspect I would like to be clarified is why then quic tests are using 16 as the sample_len, regardless of the algorithm. Because of this test, I thought that algorithms should accept any arbitrary slice of length 16 and should work fine. Am I wrong on that one?

PS: I am not proficient in the crypto algorithms, but would like to read about them if it would be needed to resolve the error/my misunderstanding.

[briansmith]

And one more aspect I would like to be clarified is why then quic tests are using 16 as the sample_len, regardless of the algorithm. Because of this test, I thought that algorithms should accept any arbitrary slice of length 16 and should work fine. Am I wrong on that one?

In all algorithms we currently support, the sample length is 16 bytes.

I have clarified the test in #2446. PTAL.

In any case, I don't think it is useful to compare 0.16.20. Nobody should be using 0.16.20 any longer.

[MikeRomaniuk]

Thanks for clarification.

Nobody should be using 0.16.20 any longer.

Yeah, I agree on that one. I am just getting some code up to speed.

I thought that algorithms should accept any arbitrary slice of length 16 and should work fine

Could you please help me with this one? I just do not understand whether this is my problem and how I can fix/help to fix the issue if there is any.

[briansmith]

It has always been documented, even in 0.16.x, that the argument of HeaderProtectionKey::new_mask must be exactly alg.sample_len() bytes long. In older versions, it wasn't documented what would happen if that prerequisite is violated, and even in newer versions it isn't documented what will happen.

I see that the implementation of HeaderProtectionKey::new changed in the most recent releases, e.g. 0.17.10 as part of our effort to turn places where we might panic into places where we might return an error.

So, it is possible that in earlier versions passing in the wrong length might have different results than you are expecting. I believe the newest version of the code is doing the best thing.

WDYT?

[briansmith]

I added tests for the key length checking in PR #2446 too.

[briansmith]

I see I misread your fuzz test earlier. I updated my earlier comment.

Could you share specific test vectors that were generated, for which the test seems to fail?

[briansmith]

Also, could you share the hardware target you are testing on?

[briansmith]

I figured out the problem. See this test vector:

KEY = e8904ecc2e37a6e4cc02271e319c804b
SAMPLE = ffffffffffffffffffffffffffffffff
MASK = 67387ebf3a

I will update the code and do a release.

[briansmith]

It looks like the problem only occurs when -Coverflow-checks is enabled in release mode or when running in debug mode. This is a good find.

[briansmith]

Thanks or reporting this. I believe it is fixed in PR #2447.

In the future, it would be great if you could use the security vulnerability reporting tool for things like this.

[MikeRomaniuk]

It looks like the problem only occurs when -Coverflow-checks is enabled

Yeah, this is the exact problem, accordingly to logs from the fuzzing.

Let me know, whether you need th logs and the generated slices.

In the future, it would be great if you could use the security vulnerability reporting tool for things like this.

Sure, I will use it in the future. I did not knew this was the security issue.

[MikeRomaniuk]

I have checked the test and the issue is fixed.

Thanks for your time and work!