Bug 3098 - remote channel ID seems to be checked with a wrong number
Summary: remote channel ID seems to be checked with a wrong number
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 8.0p1
Hardware: Other Linux
: P5 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_8_2
  Show dependency treegraph
 
Reported: 2019-11-26 16:51 AEDT by Takashi Hashida
Modified: 2021-03-04 09:51 AEDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Hashida 2019-11-26 16:51:44 AEDT
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
Comment 1 Darren Tucker 2019-11-26 23:12:42 AEDT
(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.
Comment 2 Darren Tucker 2019-11-27 17:29:15 AEDT
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.
Comment 3 Damien Miller 2020-01-25 15:51:44 AEDT
This has been re-applied and will be in openssh-8.2
Comment 4 Damien Miller 2021-03-04 09:51:41 AEDT
close bugs that were resolved in OpenSSH 8.5 release cycle