ton-blockchain / ton

Main TON monorepo
Other
2.6k stars 715 forks source link

Account random seed calculation has UB #978

Open ProgramCrafter opened 2 months ago

ProgramCrafter commented 2 months ago

Code track

Proof

Consider the following FunC code.

int prefix_of(slice, slice) asm "SDPFX";
int claimed_seed(slice block_seed, builder my_addr) asm "TWO HASHEXT_SHA256" "s0 DUMP";
int actual_seed() asm "RANDSEED" "s0 DUMP";

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

() main() {
  throw(65535);
}

;; extern "wallet-v5"
cell process_external(int my_id, cell my_storage, slice in_msg_body) method_id(89430) {
  ;; #_ block_seed:uint256 bounty:MsgAddressInt = InMsgBody;
  throw_if(100, my_storage.begin_parse().prefix_of(in_msg_body));

  slice block_seed = in_msg_body~load_bits(256);
  slice me = my_address(); me~skip_bits(11);
  ;; VVV  bug demonstration  VVV
  builder hashed = begin_cell().store_slice(me.preload_bits(32)).store_slice(me.preload_bits(224));
  throw_if(101, claimed_seed(block_seed, hashed) ^ actual_seed());

  slice dest = in_msg_body~load_msg_addr();

  accept_message();

  send_raw_message(begin_cell()
    .store_uint(0x18, 6)
    .store_slice(dest)
    .store_coins(40000000)   ;; 0.040 TON per seed delivered
    .store_uint(0, 107)
    .end_cell(), 2);

  return begin_cell()
    .store_slice(block_seed)
    .end_cell();
}

It succeeds when emulated with certain block seed and the same block seed provided in external message. Thus, random seed of account is calculated from block seed, first 32 and first 224 bits of address.

Expected behavior

The random seed of account is hash of block seed and whole address. In particular, there are no out-of-bounds reads when creating a transaction.

ProgramCrafter commented 2 months ago

If you deem this worthy of a bug bounty, my address is UQCyoez1VF4HbNNq5Rbqfr3zKuoAjKorhK-YZr7LIIiVrX0-.

EmelyanenkoK commented 1 month ago

Hi, thank you! This peculiarity was first time noted by Alex Gapak in February. We have https://github.com/ton-blockchain/ton/compare/testnet...SpyCheese:ton:trans-rand-seed?expand=1 this patch but decided to put it off for now.

ProgramCrafter commented 1 month ago

I believe it's possible to retain current behavior but do so explicitly? Because now it creates a technical debt - requirement that fields addr_rewrite and addr remain in this order.