Bug 3098

Summary:	remote channel ID seems to be checked with a wrong number
Product:	Portable OpenSSH	Reporter:	Takashi Hashida <t-hashida>
Component:	sshd	Assignee:	Assigned to nobody <unassigned-bugs>
Status:	CLOSED FIXED
Severity:	normal	CC:	djm, dtucker
Priority:	P5
Version:	8.0p1
Hardware:	Other
OS:	Linux
Bug Depends on:
Bug Blocks:	3079

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