Unsafe unwrap could cause an application to panic

[kevin-valerio]

rust-multihash/src/multihash.rs

Line 368 in 8e14b19

return Ok(decode::u64(&b[..=i]).unwrap().0);

The unwrap() here might cause some crash using the library if an error occurred. For chains using Substrate, this could cause critical severity issues if an attacker is able to craft a malicious payload in order to trigger the unwrap.

You can find an image of a malicious payload that will cause a panic below.

Below is an example of a fix that could be used :

   if decode::is_last(b[i]) {
      match decode::u64(&b[..=i]) {
        Ok(val) => return Ok(val.0),
        Err(_) => return Err(Error::VarIntDecodeError),
      }
    }

[vmx]

Your example malicious payload has 11 elements, but the we decode only up to 10 elements, due to the buffer having a length of 10. Do you have an example that leads to a panic?

Though, even if a panic cannot be triggered, it might still make sense to change the code for clarity.

[kevin-valerio]

Your example malicious payload has 11 elements, but the we decode only up to 10 elements, due to the buffer having a length of 10. Do you have an example that leads to a panic?

Sure, let me give you another example, all the details should be visible in the screenshots.

[vmx]

I've tried:

    #[test]
    fn decode_overflow() {
        let data = [203, 155, 0, 0, 0, 5, 67];
        let result = read_u64(&data[..]);
        println!("result: {:?}", result);
    }

Just as your error message, it returns an error that it's not minimal, but it does not panic. What am I missing?

[kevin-valerio]

Just as your error message, it returns an error that it's not minimal, but it does not panic. What am I missing?

Put in your Cargo.toml:

[dependencies]
multihash = { version = "0.18.0", git="https://github.com/multiformats/rust-multihash", default-features = false, features = [ "multihash-impl", "sha2"] }

Because this function will have a different behaviour if you have std in the features (because of #[cfg(not(feature = "std"))]), so ensure std is not on the features.

[mattheworris]

We have also run into panics on this line with a NotMinimal encoding error. We are also using a Substrate, no-std environment.
Any chance this fix will get merged in the near future?

[vmx]

We have also run into panics on this line with a NotMinimal encoding error.

That is an error and not a panic, isn't it?

I cannot reproduce the panic. If you can reproduce the panic, please provide a unit test for it.

[mattheworris]

The NotMinimal error triggers a panic when it hits unwrap(). I tried to create a unit test, but I cannot get the unit tests to run “no-std”. If features=std, then it doesn’t panic. However, we cannot use “std” for Substrate. Our unit tests will fail every time in a no-std environment.

…

On Wed, Apr 12, 2023 at 07:32 Volker Mische ***@***.***> wrote: We have also run into panics on this line with a NotMinimal encoding error. That is an error and not a panic, isn't it? I cannot reproduce the panic. If you can reproduce the panic, please provide a unit test for it. — Reply to this email directly, view it on GitHub <#282 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALGDXGL3UDAEB2WGIXFSFLXA2VH3ANCNFSM6AAAAAAWI6PBXM> . You are receiving this because you commented.Message ID: ***@***.***>

[vmx]

The NotMinimal error triggers a panic when it hits unwrap().

Oh sorry, it took me a long time to understand all that. So the problem is that you have a non-minimal encoded varint. That varint is then decoded and returns a NotMinimal error. As we call unwrap(), it leads to a panic.

Let me try if I create a unit test triggering that, now that I've understood the issue.

[vmx]

Alright, I now know why I wasn't able to reproduce it. The tests have std enabled (even with --no-default-features), hence it wasn't triggering the code path that contains the issue. On current master --no-default-features works as expected.