Re: [lwip-devel] TCP Timestamp Option Incorrectly Parsed on Big-Endian Systems Due to Byte Assembly Order

[Billy Bednar]

Hi Tsachi,

> I believe pcb->ts_recent should always be stored in host byte order,
> as it is an internal lwIP variable.

Yes.

> Therefore, calling lwip_ntohl(tsval) After assembling the value from
> received bytes (which are in network byte order) is required — and
> this applies to both little-endian and big-endian systems.

Although the received bytes are in network byte order, tsval is not,
so using lwip_ntohl is incorrect. The original code assembles tsval
backwards (first / most significant byte is placed in the least
significant position) which means tsval is in the reverse of host
order, not in network order. The correct conversion to host would be
an unconditional swap rather than lwip_ntohl. In the proposed
versions, the assembly order was flipped, so tsval is already in host
order and no conversion is required.

For reference, take a look at the only other option handler with a
multi-byte value, which is about 40 lines up:

> /* An MSS option with the right option length. */
> mss = (u16_t)(tcp_get_next_optbyte() << 8);
> mss |= tcp_get_next_optbyte();
> /* Limit the mss to the configured TCP_MSS and prevent division by zero */
> pcb->mss = ((mss > TCP_MSS) || (mss == 0)) ? TCP_MSS : mss;

It actually isn't quite right either. The value is assembled in the
right order, but the cast should be inside the parentheses. As-is, the
return value of tcp_get_next_optbyte gets promoted to signed int
before the shift. On 16-bit int platforms, that could result in
shifting into the sign bit, which is UB. The timestamp one that we're
discussing has similar problems.

So all together you have:

tsval  = (u32_t)tcp_get_next_optbyte() << 24;
tsval |= (u32_t)tcp_get_next_optbyte() << 16;
tsval |= (u32_t)tcp_get_next_optbyte() << 8;
tsval |= (u32_t)tcp_get_next_optbyte() << 0; // shift and cast optional
// ...
pcb->ts_recent = tsval; // (2 places)
//...

opts[2] = lwip_htonl(pcb->ts_recent);

-Billy