openssh bugs - May 2016 - [Bug 2566] New: 2 integer overflows in function ssh_packet_send2_wrapped()

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

            Bug ID: 2566
           Summary: 2 integer overflows in function
                    ssh_packet_send2_wrapped()
           Product: Portable OpenSSH
           Version: 7.2p1
          Hardware: 68k
                OS: Mac OS X
            Status: NEW
          Severity: minor
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: Torben.Hansen.2015 at rhul.ac.uk

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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 2813
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2813&action=edit
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...

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

--- Comment #2 from Torben <Torben.Hansen.2015 at rhul.ac.uk> ---
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

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2543


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2813|0                           |1
        is obsolete|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 2853
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2853&action=edit
revised diff

revised diff; check each operation in padding calculation for overflow

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2566

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.