openssh bugs - Nov 2019 - [Bug 3098] New: remote channel ID seems to be checked with a wrong number

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

            Bug ID: 3098
           Summary: remote channel ID seems to be checked with a wrong
                    number
           Product: Portable OpenSSH
           Version: 8.0p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: t-hashida at amiya.co.jp

https://github.com/openssh/openssh-portable/commit/7ec5cb4d15ed2f2c5c9f5d00e6b361d136fc1e2d#diff-68e5826568dd6f49d090ff4387c220d6R684

At this commit, remote channel ID upper bound is checked with INT_MAX.
However, it seems that the remote channel ID is uint_32.

https://tools.ietf.org/html/rfc4254#section-5.1

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

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

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 Takashi Hashida from comment #0)
> https://github.com/openssh/openssh-portable/commit/
> 7ec5cb4d15ed2f2c5c9f5d00e6b361d136fc1e2d#diff-
> 68e5826568dd6f49d090ff4387c220d6R684
> 
> At this commit, remote channel ID upper bound is checked with
> INT_MAX.
> However, it seems that the remote channel ID is uint_32.
> https://tools.ietf.org/html/rfc4254#section-5.1
In the protocol it is, however in the code until this commit it in the
channels struct was an int:
https://github.com/openssh/openssh-portable/commit/9f53229c2

I think what you found is a remnant of some tangled history: the rework
to switch to the new API took a long time and was out-of-tree, so I
suspect the tests you found date back to before the int->uint32_t
change.  The other clue that this is the case is that after the INT_MAX
check rchan is cast to int, which would make sense if remote_id was
still an int:

    c->remote_id = (int)rchan;

As it is I think the test would disallow valid channel ids, can't be
changed into anything sensible (since all 2**32 ids expressible by a
uint32_t are equally valid) and should be deleted.

That still leaves the question of why it's a uint32_t in the header,
but u_int in the function arguments that handle channel ids.

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3079


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3079
[Bug 3079] Tracking bug for 8.2 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=3098

--- Comment #2 from Darren Tucker <dtucker at dtucker.net> ---
I actually changed this, but then when looking at follow up changes
realised that the channels code still largely uses ints.  I think this
check is for that reason (and there are a couple of other examples in
channels.c), so ended up reverting the change.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
This has been re-applied and will be in openssh-8.2

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
close bugs that were resolved in OpenSSH 8.5 release cycle

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