Unsafe unwrap could cause an application to panic about rust-multihash HOT 12 CLOSED

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.

from rust-multihash.

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.

from rust-multihash.

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?

from rust-multihash.

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.

from rust-multihash.

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?

from rust-multihash.

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.

from rust-multihash.

from rust-multihash.

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.

from rust-multihash.

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.

from rust-multihash.

I've created pull requests for

• master: #291
• 0.18: #292

from rust-multihash.

I've released the fix as 0.18.1. It's also fixed on master and will be included in the next minor release.

from rust-multihash.

from rust-multihash.