trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.33k stars 649 forks source link

Revise utils.consteq() #2466

Open andrewkozlik opened 2 years ago

andrewkozlik commented 2 years ago

In utils.consteq(sec, pub) the caller needs to guarantee that len(sec) >= len(pub). This is important in case len(sec) is itself secret information. This is a very dangerous function, since it can access memory behind valid length of sec.

https://github.com/trezor/trezor-firmware/blob/0b4ccf45fc5a7de9ccff48dd33c8458bff1d20c9/core/embed/extmod/modtrezorutils/modtrezorutils.c#L50-L75

We could make it better by:

  1. Adding assert(secbuf.len >= pubbuf.len); at the beginning, so that we at least have a better chance of catching programming bugs in the debug build.
  2. Iterating only up to the minimum length, which can be computed in constant time:
    // Compute minlen = min(secbuf.len, pubbuf.len) in constant time.
    uint32_t mask = -(secbuf.len < pubbuf.len);
    uint32_t minlen = (secbuf.len & mask) | (pubbuf.len & ~mask);

    However, if secbuf.len < pubbuf.len, then the timing of the loop reveals the length of secbuf. This is a problem if the caller actually knows that they can read past the buffer's length, because more bytes are allocated, e.g. in case of bytearray. BTW, in that case we could check mp_obj_array_t::free, but not with mp_buffer_info_t directly. The question is whether there actually is any place in our code that uses this feature.

  3. Accessing s modulo secbuf.len, i.e.
    diff |= s[i % secbuf.len] - p[i];

    Not sure how to deal with secbuf.len==0 though. Also not sure whether the % operation is constant-time.

_Originally posted by @andrewkozlik in https://github.com/trezor/trezor-firmware/pull/2289#discussion_r949899240_

prusnak commented 2 years ago

Can we afford to just bail out when secbuf.len != pubbuf.len? Most constant-time comparison implementations do that.