openssh bugs - Nov 2018 - [Bug 2929] New: OpenSSH server should not send the SSH_MSG_EXT_INFO message after rekeying

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

            Bug ID: 2929
           Summary: OpenSSH server should not send the SSH_MSG_EXT_INFO
                    message after rekeying
           Product: Portable OpenSSH
           Version: 7.7p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

The RFC 8308 specifies, that the SSH_MSG_EXT_INFO message should be
sent after the *first* SSH_MSG_NEWKEYS message, while the OpenSSH
server sends it also after the rekey:
>    o  As the next packet following the server's first SSH_MSG_NEWKEYS.
Side note:
The draft-ssh-ext-info-04  from [1] is already RFC [2], so the page
could make use of an update. Also the draft-rsa-dsa-sha2-256-03 is
already RFC [3].

[1] http://www.openssh.com/specs.html
[2] https://tools.ietf.org/html/rfc8308
[3] https://tools.ietf.org/html/rfc8332

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

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at dtucker.net

--- Comment #1 from Darren Tucker <dtucker at dtucker.net> ---
(In reply to Jakub Jelen from comment #0)
> The RFC 8308 specifies, that the SSH_MSG_EXT_INFO message should be
> sent after the *first* SSH_MSG_NEWKEYS message, while the OpenSSH
> server sends it also after the rekey:
> 
> >    o  As the next packet following the server's first
SSH_MSG_NEWKEYS.
Under what conditions does the server send SSH_MSG_EXT_INFO a second
time?  The client removes it from the proposal once sent:

sshconnect2.c:().
        /* remove ext-info from the KEX proposals for rekeying */
        myproposal[PROPOSAL_KEX_ALGS]            
compat_kex_proposal(options.kex_algorithms);

and kex.c sets the handler to return an error after the first instance:

kex.c:kex_input_ext_info():
        debug("SSH2_MSG_EXT_INFO received");
        ssh_dispatch_set(ssh, SSH2_MSG_EXT_INFO, &kex_protocol_error);

even removing that dispatch_set on the client side I can only see a
single SSH2_MSG_EXT_INFO received on the client side.
> Side note:
> The draft-ssh-ext-info-04  from [1] is already RFC [2], so the page
> could make use of an update. Also the draft-rsa-dsa-sha2-256-03 is
> already RFC [3].
> 
> [1] http://www.openssh.com/specs.html
> [2] https://tools.ietf.org/html/rfc8308
> [3] https://tools.ietf.org/html/rfc8332
Fixed those, thanks.  I periodically check them but don't always catch
status changes.

-- 
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=2929

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
I noticed this with different client than OpenSSH. This indeed happens
when the client sends the ext-info-c also with the rekeying
SSH_MGS_KEXINIT, which looks like wrong thing to do (and which I
probably missed in the first reads of the rfc):
>   Applications implementing this mechanism MUST add one of the
>   following indicator names to the field kex_algorithms in the
>   SSH_MSG_KEXINIT message sent by the application in the first key
>   exchange:
In that case, I will make sure this is fixed in libssh does not append
the ext-info-c to the rekeying requests.

But even though the client did not do the correct thing, I think server
should not be manipulated to do the bad thing either.

The message sending is triggered directly by setting the ext_info from
the current key exchange, but I think, there should be also a condition
to skip the call to kex_send_ext_info() if we are in state of rekeying.

https://github.com/openssh/openssh-portable/blob/120a1ec7/kex.c#L421

-- 
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=2929

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
   Attachment #3205|                            |ok?
              Flags|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 3205
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3205&action=edit
only consider ext-info-c during initial KEX

-- 
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=2929

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2915


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2915
[Bug 2915] Tracking bug for 8.0 release
-- 
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=2929

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3205|ok?                         |ok+
              Flags|                            |

-- 
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=2929

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
applied - this will be in OpenSSH 8.0

-- 
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=2929

Pawel Jakub Dawidek <openssh at dawidek.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED
                 CC|                            |openssh at dawidek.net

--- Comment #5 from Pawel Jakub Dawidek <openssh at dawidek.net> ---
Hi Damian,

I think the fix is incomplete. It probably only works with the OpenSSH
server when sandboxing is enabled, but it doesn't work with ssh_api.c.

When using API, the kex structure is allocated only once and during the
first KEX the ext_info_c field is set to 1. It is then never set to 0,
so during next rekeying, even though KEX_INITIAL is no longer set, the
SSH_MSG_EXT_INFO will be send again as ext_info_c remains 1.

To fix that it would be enough to add:

kex->ext_info_c = 0;

right after:

kex->flags &= ~KEX_INITIAL;

in the kex_input_newkeys() function.

Thank you.

-- 
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=2929

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3316|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
Created attachment 3316
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3316&action=edit
check KEX_INITIAL before sending ext-info

IMO it's better to check KEX_INITIAL. Add some debug() to make it clear
whether/when the ext-info is sent.

Note that disabling privsep is not supported (there is no option) and
the API is still very much a work in progress.

-- 
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=2929

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3316|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

--- Comment #7 from Darren Tucker <dtucker at dtucker.net> ---
Comment on attachment 3316
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3316
check KEX_INITIAL before sending ext-info
> (ssh->kex->flags & KEX_INITIAL) != 0
given that it's being used as a boolean you could omit the != 0.

-- 
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=2929

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
This was fixed in openssh-8.1 last year

-- 
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=2929

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 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=2929

Ahmed Sayeed <ahmedsayeed1982 at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ahmedsayeed1982 at yahoo.com

--- Comment #10 from Ahmed Sayeed <ahmedsayeed1982 at yahoo.com> ---
<1><e1e>: Abbrev Number: 46 (DW_TAG_array_type)
http://www.compilatori.com/travel/youtube/
    <e1f>   DW_AT_data_location: 2 byte block: 97 6    
(DW_OP_push_object_address; DW_OP_deref)
http://www.acpirateradio.co.uk/travel/carbon-dioxide-emissions/
    <e22>   DW_AT_rank        : 6 byte block: 97 23 10 6 37 1a
    <e29>   DW_AT_type        : <0x139>
http://www.logoarts.co.uk/travel/actions-camera/
    <e2d>   DW_AT_sibling     : <0xe51>
 <2><e31>: Abbrev Number: 47 (DW_TAG_generic_subrange)
http://www.slipstone.co.uk/travel/hyundai-kona/
    <e32>   DW_AT_lower_bound : 8 byte block: 97 14 48 1e 23 20 22 6   
(DW_OP_push_object_address; DW_OP_over; DW_OP_lit24; DW_OP_mul;
DW_OP_plus_uconst: 32; DW_OP_plus; DW_OP_deref)
http://embermanchester.uk/travel/whatsapp/
    <e3b>   DW_AT_upper_bound : 8 byte block: 97 14 48 1e 23 28 22 6   
(DW_OP_push_object_address; DW_OP_over; DW_OP_lit24; DW_OP_mul;
DW_OP_plus_uconst: 40; DW_OP_plus; DW_OP_deref)
http://connstr.net/travel/charging-machines/
    <e44>   DW_AT_byte_stride : 11 byte block: 97 14 48 1e 23 18 22 6 8
38 1e   (DW_OP_push_object_address; DW_OP_over; DW_OP_lit24; DW_OP_mul;
http://joerg.li/travel/kia-rio/   DW_OP_plus_uconst: 24; DW_OP_plus;
DW_OP_deref; DW_OP_const1u: 56; DW_OP_mul)
...
 http://www.jopspeech.com/travel/london/
We start out in read_array_type with:
...
  type = element_type;
... http://www.wearelondonmade.com/travel/renault/
and then iterate over range_types to build up the type further.

But there are no DW_TAG_subrange_type children (only one
DW_TAG_generic_subrange), so range_types is empty, and type is kept
unmodified. https://waytowhatsnext.com/services/netflix-services/

Consequently, in set_die_type we apply the DW_AT_data_location to the
element_type (the one at 0x139) instead of to the newly build array
type.
http://www.iu-bloomington.com/services/xiaomi-services/
Then we try to print c_nd:
...
 <2><6e8>: Abbrev Number: 2 (DW_TAG_variable)
    <6e9>   DW_AT_name        : (indirect string, offset: 0x218): c_nd
    <6ed>   DW_AT_decl_file   : 1
https://komiya-dental.com/services/huawei-service/
    <6ed>   DW_AT_decl_line   : 198
    <6ee>   DW_AT_type        : <0x139>
    <6f2>   DW_AT_location    : 9 byte block: 3 e0 30 60 0 0 0 0 0     
(DW_OP_addr: 6030e0)
... http://www-look-4.com/travel/new-cars/
and find that the type has a data_location property, which when used
gives incorrect results.
...
  type = element_type;
...
and then iterate over range_types to build up the type further.
 https://www.webb-dev.co.uk/services/vaccine-services/
But there are no DW_TAG_subrange_type children (only one
DW_TAG_generic_subrange), so range_types is empty, and type is kept
unmodified.

Consequently, in set_die_type we apply the DW_AT_data_location to the
element_type (the one at 0x139) instead of to the newly build array
type.

-- 
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.