Conditional compilation flag allows planting backdoor in ink! contracts

[R9295]

Summary

When generating a contract's metadata, the ink! compiler parses the contract's code and takes into account conditional compilation flags. This allows malicious contract authors to hide messages, events and constructors from the metadata but place them in the WASM blob.

Issue details

ink! generates the code for metadata and embeds it inside the contract file, which, when compiled, outputs the metadata.
Due to the inclusion of conditional compilation flags such as #[cfg(target_family="wasm")] in metadata generation and the metadata not being compiled with wasm, there can be a mismatch between the metadata and the compiled wasm blob.

Example Scenario

Suppose a malicious contract author adds a utility function test_set_value to their contract for use in tests, marked with #[cfg(any(test, target_family="wasm")]. This message would allow any user to set the contract's value to any integer, which they should not be able to do in a normal scenario.

From the point of view of a contract's user, they would see this function used only in tests, but not be included in the metadata, even when they build it themselves on their machine.

A user would have to manually inspect the WASM blob to see the function included there. We argue that even advanced users are unlikely to manually inspect the WASM blob of the contract.

What we have in this situation is a backdoor planted by a malicious code author, hidden in the code through the seemingly test-only nature of the function, completely hidden from the metadata.

This malicious code author could at some point trigger this contract's message by manually calling its selector via the contract-pallet.

#![cfg_attr(not(feature = "std"), no_std, no_main)]

#[ink::contract]
pub mod my_cool_contract {

    /// My cool contract, containing a value that can only go up by one
    #[ink(storage)]
    pub struct MyCoolContract {
        value: u32,
    }

    impl MyCoolContract {
        // Value is set to 0 at construction
        #[ink(constructor)]
        pub fn new() -> Self {
            Self { value: 0 }
        }

        // The main functionality of this contract: Make the value go up by one!
        #[ink(message)]
        pub fn increment(&mut self) {
            self.value = self.value.saturating_add(1);
        }

        // Test message used to set the value to a certain number.
        // Not available in the built contract.
        #[cfg(any(test, target_family = "wasm"))]
        #[ink(message)]
        pub fn test_set_value(&mut self, value: u32) {
            self.value = value;
        }
    }

    #[cfg(test)]
    mod tests {
        use super::*;

        /// Complete test
        #[ink::test]
        fn complete_test() {
            let mut contract = MyCoolContract::new();

            assert_eq!(contract.value, 0);

            contract.increment();

            assert_eq!(contract.value, 1);

            contract.test_set_value(41);
            contract.increment();

            assert_eq!(contract.value, 42);
        }
    }
}

Notice how the generated metadata does not list the message test_set_value. The function will nevertheless be compiled into the wasm blob and available through its selector like any other message.

{
  "source": {
    "hash": "0xa770b1afd99b6ad4e451d31b2bbb5c9f345b459b12f759caabf9619e4d441ec6",
    "language": "ink! 5.0.0",
    "compiler": "rustc 1.81.0",
    "build_info": {
      "build_mode": "Release",
      "cargo_contract_version": "4.1.1",
      "rust_toolchain": "stable-x86_64-unknown-linux-gnu",
      "wasm_opt_settings": {
        "keep_debug_symbols": false,
        "optimization_passes": "Z"
      }
    }
  },
  "contract": {
    "name": "my-cool-contract",
    "version": "5.0.0",
    "authors": [
      "Parity Technologies <[email protected]>"
    ]
  },
  "image": null,
  "spec": {
    "constructors": [
      {
        "args": [],
        "default": false,
        "docs": [],
        "label": "new",
        "payable": false,
        "returnType": {
          "displayName": [
            "ink_primitives",
            "ConstructorResult"
          ],
          "type": 2
        },
        "selector": "0x9bae9d5e"
      }
    ],
    ...
    "messages": [
      {
        "args": [],
        "default": false,
        "docs": [],
        "label": "increment",
        "mutates": true,
        "payable": false,
        "returnType": {
          "displayName": [
            "ink",
            "MessageResult"
          ],
          "type": 2
        },
        "selector": "0x12bd51d3"
      }
    ]
  },
  ...
  "version": 5
}

Risk

The discrepancy between a contract's WASM blob and the associated metadata allows an attacker to hide malicious functionality from a contract's users and exploit the contract after some desired outcome has been reached (e.g. some tokens have been deposited on the contract by victims).

Mitigation

The ink! toolchain should Error on all conditional compilation flags in a contract's source code.

[cmichi]

Thanks for reporting this issue!

The ink! toolchain should Error on all conditional compilation flags in a contract's source code.

It would be very unfortunate if we disallow conditional compilation flags altogether. What do you think about this mitigation instead:

• The issue happens when code is only included when compiling for wasm, but not when generating the metadata.
• We disallow only conditional flags that concern the wasm architecture/target. So any conditional cfg flag that makes use of the wasm target.

This way users could still use conditional flags for e.g. only adding contract methods in a test environment.

[louismerlin]

Other conditional compilation flags can be used with the same effect, with potentially more being added in the future to Rust.

One example: target_pointer_width set to "32".

How about having an allow-list of compilation flags? Including test and feature (the two keys used in the ink-examples) and any others that you think are necessary but that do not allow differentiating wasm vs native.

[cmichi]

I've created use-ink/ink#2313 to fix the issue. Could you take a look? The failing CI there is because of an unrelated error, I'll fix that one in a separate PR.

Two things:

• I realized that while we want to allow feature, it can be used to differentiate native vs. wasm via #[cfg(not(feature = "std"))]. I've hence allowed feature, except for feature = "std".
• It's a hefty breaking change to userland and I would ideally only release it with the next major version of ink! (v6).

[louismerlin]

Nice, the PR looks good to me, and the breaking change rationale too.