Bug 2478 - Escape sequences (starting with ~) don't work when remote shell is BusyBox ash
Summary: Escape sequences (starting with ~) don't work when remote shell is BusyBox ash
Status: NEW
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 7.1p1
Hardware: ix86 Linux
: P5 minor
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-13 07:11 AEDT by Alex Dowad
Modified: 2016-12-09 12:41 AEDT (History)
2 users (show)

See Also:


Attachments
Fix (2.80 KB, patch)
2015-10-13 07:11 AEDT, Alex Dowad
dtucker: ok-
Details | Diff
Patch v2 (3.25 KB, patch)
2015-10-13 17:22 AEDT, Alex Dowad
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Dowad 2015-10-13 07:11:17 AEDT
Created attachment 2725 [details]
Fix

This was originally send directly to the mailing list, but I am also posting it here as requested by Damien Miller.

This patch was prompted by the observation that SSH escapes (using ~) don't work at all when the remote shell is BusyBox ash (used by Alpine Linux and some other minimalist and/or embedded Linux distributions).

This is because the BusyBox shell *always* sends a "query cursor position" terminal control escape sequence each time you press the <ENTER> key. Quick as a flash, as your pinky finger is descending towards the tilda key, the terminal sends back a "report cursor position" sequence. The result is that the SSH client *never* sees your tilda as immediately following the newline.

Those ANSI terminal control sequences always begin with "<ESC>[" and end
with an alphabetic character. Hence the call to isalpha() in my code.

Can someone confirm that isalpha() *always* treats a-z and A-Z as alphabetic, no matter what the locale is?
Comment 1 Darren Tucker 2015-10-13 08:18:42 AEDT
Comment on attachment 2725 [details]
Fix

>+		if (ch == 0x1B && buf[i+1] == '[') {

If the escape char is the last byte in the buffer then the control-sequence detection won't work and you'll get an out-of-bounds read.

Admittedly this is unlikely in regular usage, but certainly possible for a malicious server and maybe possible by traffic shaping a legit connection.
Comment 2 Damien Miller 2015-10-13 12:12:48 AEDT
Yeah, I think it should skip over the escape char if it is the last character. escape_pending won't be reset and the client will try to go read some more tty data and call process_escapes again (I think).

I'm not sure the pushing back of escape sequences to the "bin" buffer is correct though - it might end up messing up the order of things. Something a bit more tricky is probably needed.
Comment 3 Alex Dowad 2015-10-13 14:48:51 AEDT
dtucker: Thanks for pointing that out. I realized the same when falling asleep yesterday. Will post an updated patch.

djm: "I'm not sure the pushing back of escape sequences to the bin buffer is correct" -- this is exactly what happens to them in the existing code, without this patch. That aspect is unchanged. The only thing which changes, is that <CR><escape><escaped char> sequences are processed by the client *even if* an ANSI escape appears in between the <CR> and <escape> or between the <escape> and <escaped char>.

There's no reason why OpenSSH should intercept and change or remove ANSI escape sequences. That would make a remote shell behave differently from a local shell. By writing them into 'bin', just as they appear in the input, SSH can be 'transparent' as far as the interaction between terminal emulator and remote shell goes.
Comment 4 Alex Dowad 2015-10-13 17:22:59 AEDT
Created attachment 2726 [details]
Patch v2

Please have a look at the v2. It treats ANSI terminal escapes consistently regardless of whether they are 'cut off' at the end of 'buf' (and straddle 2 separate calls to process_escapes) or not.
Comment 5 Alex Dowad 2015-10-13 20:46:30 AEDT
I've just looked at the possibility of adding some tests for ssh escape sequences to your regression tests.

One difficulty is that the ~C command prompt works by reading/writing /dev/tty directly, rather than using stdin/stdout. This makes it hard to capture what the user sees and compare it to expected output.
Comment 6 Alex Dowad 2015-10-31 01:31:36 AEDT
Ping...
Comment 7 Damien Miller 2016-07-22 14:10:50 AEST
retarget unfinished bugs to next release
Comment 8 Damien Miller 2016-07-22 14:14:53 AEST
retarget unfinished bugs to next release
Comment 9 Damien Miller 2016-07-22 14:15:42 AEST
retarget unfinished bugs to next release
Comment 10 Damien Miller 2016-07-22 14:17:15 AEST
retarget unfinished bugs to next release
Comment 11 Damien Miller 2016-12-09 12:41:15 AEDT
I think this patch needs more work - at the very least it needs to conditionalise its special-casing of ANSI sequences to be enabled only for ANSI-like $TERM (-inals).

BusyBox's behaviour does seem kind of dumb though...