[spl-token-swap] Unsound implementation in instruction::unpack

[shinmao]

The source of unsoundness

solana-program-library/token-swap/program/src/instruction.rs

Lines 605 to 611 in 282c716

	pub fn unpack<T>(input: &[u8]) -> Result<&T, ProgramError> {
	if input.len() < size_of::<u8>() + size_of::<T>() {
	return Err(ProgramError::InvalidAccountData);
	}
	#[allow(clippy::cast_ptr_alignment)]
	let val: &T = unsafe { &*(&input[1] as *const u8 as *const T) };
	Ok(val)

Hi, we consider that instruction::unpack function unsound because it can cast u8 type with any bit patterns to arbitrary types. This could break the validity invariants which should be hold anywhere in rust program. Additionally, it can also break alignment requirements.

To reproduce the bug

use spl_token_swap::instruction::unpack;

fn main() {
    let a: [u8; 3] = [3; 3];
    let up = unpack::<bool>(&a).unwrap();
    println!("{}", up);
}

then run with miri,

error: Undefined Behavior: constructing invalid value: encountered 0x03, but expected a boolean
    --> /${HOME}/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2307:25
     |
2307 |         Display::fmt(if *self { "true" } else { "false" }, f)
     |                         ^^^^^ constructing invalid value: encountered 0x03, but expected a boolean
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior

To break alignment requirements,

use spl_token_swap::instruction::unpack;

fn main() {
    let a: [u8; 3] = [3; 3];
    let up = unpack::<u16>(&a).unwrap();
    println!("{}", up);
}

just cargo run and get panic,

thread 'main' panicked at 'misaligned pointer dereference: address must be a multiple of 0x2 but is 0x7fff2f83e6bd', /${HOME}/.cargo/registry/src/index.crates.io-6f17d22bba15001f/spl-token-swap-3.0.0/src/instruction.rs:562:28
stack backtrace:
   0: begin_panic_handler
             at /rustc/a97c36dd2e6f711949fc9b790476e93bd9e6d1f4/library/std/src/panicking.rs:593:5
   1: panic_fmt
             at /rustc/a97c36dd2e6f711949fc9b790476e93bd9e6d1f4/library/core/src/panicking.rs:67:14
   2: panic_misaligned_pointer_dereference
             at /rustc/a97c36dd2e6f711949fc9b790476e93bd9e6d1f4/library/core/src/panicking.rs:174:5
   3: spl_token_swap::instruction::unpack
             at /${HOME}/.cargo/registry/src/index.crates.io-6f17d22bba15001f/spl-token-swap-3.0.0/src/instruction.rs:562:28
   4: rand::main
             at ./src/main.rs:5:14

[Shnatsel]

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on. Since this is an API soundness issue, it will be surfaced as a warning rather than a hard error.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

[joncinque]

Hi @Shnatsel , thanks for flagging this. We're moving the SPL programs into separate repos, and spl-token-swap won't be coming, since it's been unmaintained for a few years (outside of dependency upgrades). I can file an issue at the advisory database repo once this repo is archived.