This concerns part of the function ssh_packet_send2_wrapped() in the file packet.c. The functionality of adding extra padding contains two integer overflows which can be triggered for certain values of extra_pad, block_size and len. Firstly, the computation roundup(state->extra_pad, block_size) can return 0 for certain values of extra_pad and block_size. This causes state->extra_pad = 0 but this variable is used subsequently in a modular operation. Secondly, the assignment padlen += pad might overflow for certain values of extra_pad, padlen and block_size. This can cause the length of the padding appended to the outgoing packet to be strictly less than 4; because no sanity check on the padding length follows the adding of extra padding. The computation roundup(state->extra_pad, block_size) rounds up extra padding to the nearest multiple of the block size. For e.g. block_size = 8, this computation will wrap to 0 when the requested extra padding length is larger or equal to 249. For larger block sizes the wrapping will start at a smaller value. Because the variable state->extra_pad is used subsequently in a modular operation, a floating point exception will be raised when the variable state->extra_pad is set to zero. It is possible that a packet will be finalised with less than 4 bytes of padding, which is a violation of RFC 4253 section 6 that states: "There MUST be at least four bytes of padding". On the receiver end, a server/client will drop a packet (with a SSH_ERR_CONN_CORRUPT error) if the padding is less than 4. For e.g. a block size of 8 the padding appended to the outgoing packet will be less than 4 if 240 < extra_pad < 249 and len = 237, 238, 239, 240, 485, 486, 488, .... I acknowledge the fact that extra padding is (atm) exclusively used for padding user passwords during authentication and used in a way that does not trigger the behaviour described above.
Created attachment 2813 [details] check padding calculations Hi, Thanks for the report. I think we can avoid these by promoting the padding calculation variables' types from u_char to u_int and dropping in a few checks. This patch implements this. It's currently cranking through our regression tests...
Hi Damien, This patch would prevent any integer overflows. But by promoting padlen to u_int you now open up for the possibility of having padlen > 255 (which do happen for the same values that would make the variable wrap in the first case). I'm not sure what consequences it has for the execution of the code following the extra padding computation. It is nonetheless illegal to have more than 255 bytes of padding according to RFC 4253 section 6. Checking if padlen + pad > 255 (instead of just pad > 255) would prevent this problem. Cheers, Torben
Created attachment 2853 [details] revised diff revised diff; check each operation in padding calculation for overflow
I've committed the revised patch, this will be in OpenSSH 7.3 which is due to be released in the next couple of weeks.
Close all resolved bugs after 7.3p1 release