SSH client doesn't handle properly non-ASCII chars

[Marco Trevisan (Treviño)]

[ Impact ]

Non-ascii visible chars (including back-slashes, new lines and so) are not properly rendered by clients, showing their octal visualization.

Such as:

  Hello SSHD \\ We love \360\237\215\225!

Instead of:

  Hello SSHD \ We love 🍕!

This is particularly an issue when a server has configured keyboard interactive authentication and a PAM module wants to show non-ASCII characters such as a QR code for web authentication:

When using an ubuntu server running authd for web authentication we may end up having the login qrcode rendered such as

\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210\342\226\210
                        https://ubuntu.com
                               1337

Which is clearly unreadable.

[ Test case ]

## Server preparation

Enable PAM and keyboard interactive authentication in a ssh server:

Add a configuration file such as:
 /etc/ssh/sshd_config.d/test-ssh-pam.conf

Containing:

 UsePAM yes
 KbdInteractiveAuthentication yes# This was working already; here to check potential regressions
 ForceCommand bash -c "echo Hello from SSHD \ We also love 🍕!; $SHELL"

It's also suggested to check for regressions using a `Banner` option in sshd, pointing to a file with utf-8 contents:

 echo "Hello" | qrencode -t ansiutf8 > /tmp/ssh-banner
 Banner /tmp/ssh-banner

Restart the server:

  sudo systemctl restart ssh.service

Edit the sshd PAM configuration file, adding as first line:

  auth requisite pam_echo.so Hello SSHD \ We love 🍕!

Can be done with the command:
  sudo sed '1 iauth requisite pam_echo.so Hello SSHD! \\ We love 🍕!' \
   -i /etc/pam.d/sshd

## Client test

In the same host:

 ssh -o PubkeyAuthentication=no \
     -o PasswordAuthentication=no \
     -o PreferredAuthentications=keyboard-interactive \
     $USER@localhost

The client should show:

Hello SSHD \ We love 🍕!
($USER@localhost) Password:
...
Hello from SSHD \ We also love 🍕!
█████████████████████████████
█████████████████████████████
████ ▄▄▄▄▄ █▀█ █ █ ▄▄▄▄▄ ████
████ █ █ █▄█▄▄▀█ █ █ ████
████ █▄▄▄█ █ ▄▄█ █▄▄▄█ ████
████▄▄▄▄▄▄▄█ █▄█▄█▄▄▄▄▄▄▄████
████ █▄▀▀▄ █▀▄ ▄▀▄ ▄█▄ ▀████
████ ██▀▀ ▄▀▀▄▀▄▀▀ ▄▀ ████
████▄▄▄▄██▄▄ █▄█ ▀█▀██████
████ ▄▄▄▄▄ █▄▀▀▄▄█ ▀ ▄▄ ▀████
████ █ █ █▀█▀█▄ ▀▄▀▀▀ ████
████ █▄▄▄█ █▀ ▄ ▀▄▄█▄█▄█▄████
████▄▄▄▄▄▄▄█▄▄███▄█▄█▄█▄█████
█████████████████████████████
█████████████████████████████

Retry the same with another host and without keyboard authentication enabled in the server side.

To verify the fix in more complex scenario it's possible to follow the instructions of configuring authd:
 - https://github.com/ubuntu/authd/wiki/05--How%E2%80%90to-log-in-over-SSH

Once authd is configured, the user should be able to scan a QrCode from a ssh session.

## Cleanup

Revert the changes done in the cleanup phase, after test is done

sudo sed '/pam_echo\.so/d' -i /etc/pam.d/sshd
sudo rm /etc/ssh/sshd_config.d/test-ssh-pam.conf

# Further testing

It's also required to check if other configurations using keyboard interactive such as TOTP/HOTP access still work:

https://ubuntu.com/server/docs/openssh-server#two-factor-authentication-with-totphotp

# Authd testing

- Configurand install authd and MsEntraID broker as described at:
  https://github.com/ubuntu/authd/wiki/01---Get-started-with-authd

- Configure SSHd and try to login using the Qrcode as documented at
  https://github.com/ubuntu/authd/wiki/05--How%E2%80%90to-log-in-over-SSH

- Accessing via SSH to a machine via QrCode should show a properly rendered
  qrcode

[ Regression potential ]

SSH info messages are not shown by the client. Even though those aren't covered by this change, it's important to check for regressions in any output that SSH exposes to the user. So banners and other messages should be checked for regressions.

These kind of messages are normally shown only when PAM *and* keyboard interaction are enabled in the server side, so it should not affect the default ubuntu servers behavior.

[Robie Basak (racb)]

This seems like quite an invasive change. It has not yet been accepted upstream. It touches PAM, and it looks to me like it might affect behaviour before authentication is complete. It affects escaping. Injection of malicious data into a stream to be parsed by the terminal has security implications. There is no security analysis or opinion of the security team presented.

If we're going to make changes in stable releases, or even a distribution patch, I think we need particularly strong justification given the above factors.

To consider that, we need to consider the actual impact to users. But that doesn't seem to have been presented here.

> Non-ascii visible chars are not properly rendered by clients, showing their octal visualization.

That's not really an explanation of impact to user.

What are we looking at here? Just the ability to include emoji in messages that, according to the SRU documentation provided, won't even be seen by the user? That sounds like a feature to me, and therefore doesn't seem appropriate to change a stable release for given that no justification has been provided.

> SSH info messages are not shown by the client.

This seems to be contradicted by the provided Test Plan, which runs the client and checks for the message. Please explain.

> These kind of messages are normally shown only when PAM is enabled in the server side, so it should not affect the normal behavior.

PAM is enabled by default on openssh on Ubuntu, no?

For SRU purposes, -1 based on the lack of an acceptable justification to SRU. If there is one, please present it, otherwise these uploads should be rejected from the queue.

[Marco Trevisan (Treviño) (3v1n0)]

> This seems like quite an invasive change. It has not yet been accepted upstream.

Nope, but reviewed and approved by at least one upstream developer both upstream and downstream (Tobias), while we're using it in noble for few months already with no issue reported so far.

> It touches PAM, and it looks to me like it might affect behaviour before authentication is complete. It affects escaping. Injection of malicious data into a stream to be parsed by the terminal has security implications. There is no security analysis or opinion of the security team presented.

It doesn't touch PAM authentication by default, it's triggered by (some) PAM authentications.

However, we can definitely get the security team in the loop here.

> What are we looking at here? Just the ability to include emoji in messages that, according to the SRU documentation provided, won't even be seen by the user?

I can clarify this in the description, but showing emoji that's clearly not the root cause, it's just the simplest and easier reproducer for this issue.

Indeed, the reason for this is that in authd we are presenting a qrcode to perform weblogin and that doesn't work.

We are basically affected by the same issue of https://github.com/ubuntu/authd/issues/497

The test case for it, in a such complex setup isn't as easy to test as the minimal reproducer that it's enough to cover the fix and to ensure there are no regressions in the normal behavior.

We're going to handle that in the openssh server too (starting from noble), but still we should be able to support this capability in the right way in ubuntu.

> That sounds like a feature to me, and therefore doesn't seem appropriate to change a stable release for given that no justification has been provided.

Well, all the bug fixes can be features, if you are strict enough. But this is not the case IMHO, the client is clearly broken at handling some particular text, we're fixing it.

There are no changes in behavior, and other major SSH clients (putty) handle this case already as the proposed change does for years.

>> > SSH info messages are not shown by the client.
> This seems to be contradicted by the provided Test Plan, which runs the client and checks for the message. Please explain.

PAM info messsages are exposed as generic SSH messages, they're the only ones of this kind AFAIK, but I wanted to be generic enough so that normal ssh usage should be checked too.

>> These kind of messages are normally shown only when PAM is enabled in the server side, so it should not affect the normal behavior.
> PAM is enabled by default on openssh on Ubuntu, no?

PAM it is, but keyboard interactive authentication (which is the only case that triggers these messages) is not, so the change doesn't affect PAM behavior of ssh per se, only if the server has enabled keyboard interactive authentication, which PAM modules can trigger.

But again, this is not default.

[Seth Arnold (seth-arnold)]

This change makes me uneasy:

- I see no terminal-aware filtering applied in the notify_start() -> xvasprintf() -> writemsg() -> write() path. The remote server may not be entirely untrusted but it's also not exactly trusted, either, especially on the first use. There's a long and glorious history of surprising outcomes due to terminal escape sequences https://www.cyberark.com/resources/threat-research-blog/dont-trust-this-title-abusing-terminal-emulators-with-ansi-escape-characters

- I'm not sure it's even necessary: my phone was easily able to read pure-ascii QR codes as easily as the (admittedly much better looking) utf-8 based codes. Try a few:

sudo apt install qrencode
u=`cat /proc/sys/kernel/random/uuid` ; for t in ANSI ANSI256 ASCII ASCIIi UTF8 ANSIUTF8 ; do qrencode -t $t $u ; done ; echo $u ; unset u

Are ascii-encoded qr codes known to be insufficient?

- As for the actual code changes, they seemed fairly straightforward, and I found no issues. I'm very wary about undoing a safety mechanism from the past, put in place by people who thought about this significantly more than I have.

- Upstream might have been engaging for a while but now appears entirely silent. This reminds me too much of dpkg+zstd, where a similar train of engagement lead to Ubuntu forking the dpkg ecosystem and carrying a patch without a clear way to reunify the ecosystem. Will we do the same to OpenSSH? (We might have already passed this point if we chose to ship this in Noble rather than wait for Oracular to test this out.)

Thanks

[Marco Trevisan (Treviño) (3v1n0)]

Mh, yeah looks like the terminal filtering is something we should do in openssh anyways, at various levels though.

Fact is that if a server is malicious, nothing prevents to do the same through a simpler sshd banner or command, that is still able to act on remote terminal.

It's true that PAM modules are sneakier than that, as they may be checked less than the pure openssh server configuration, but still it's all server duty, and if compromised (intentionally or not), it can be still a driver for such issues in the current state.

ASCII based solutions for qr code are acceptable, they can just quite big though. Being a problem if you use them from a tty. However that's another possibility we were considering already.

Regarding upstream diversion, I totally agree (and I would have loved to avoid it)... Ideally we had a plan, and the fix was proposed way earlier upstream than downstream. So we were just waiting...

[Robie Basak (racb)]

Leaving my draft here both so you can read it and so I don't lose it:

> Indeed, the reason for this is that in authd we are presenting a qrcode to perform weblogin and that doesn't work.

This seems a reasonable justification for an SRU in principle then, now that it's documented. Thank you for the update.

The remaining questions are whether this way of fixing it is the best one for the SRU. And even though I agree the SRU is justified in principle, we should remain open to the possibility that the risk associated with this particular change outweighs the benefits. Given the security sensitive area being touched I'd like a firm +1 from the security team please, that considers both the code changes and the risk.

Seth> Are ascii-encoded qr codes known to be insufficient?

I think this is a good question but leave it to the desktop team to decide.

> The test case for it, in a such complex setup isn't as easy to test as the minimal reproducer that it's enough to cover the fix and to ensure there are no regressions in the normal behavior.

I think that's fine for the usual requirement that the test case be reproducible by anybody and it looks like a good initial proxy test for the issue being resolved. However, you're requesting this SRU for a particular purpose, and it's an invasive change. I'd like you to verify that your actual purpose is also fulfilled end-to-end during SRU verification please, to ensure that before we publish to -updates that further tweaks won't be required and there will be no reason to abandon the change. Otherwise we would have burdened users with a download, update and regression risk for no reason. Please add a verification that your final purpose actually works to the Test Plan. If changes to other packages are also needed for that, then they should be tested in -proposed all together.

> PAM info messsages are exposed as generic SSH messages, they're the only ones of this kind AFAIK, but I wanted to be generic enough so that normal ssh usage should be checked too.

> PAM it is, but keyboard interactive authentication (which is the only case that triggers these messages) is not, so the change doesn't affect PAM behavior of ssh per se, only if the server has enabled keyboard interactive authentication, which PAM modules can trigger.

OK, but there are cases where we recommend doing that, such as for HOTP/TOTP configuration: https://ubuntu.com/server/docs/openssh-server. Maybe it's worth adding to the Test Plan to ensure that those steps still work?

> So banners and other messages should be checked for regressions.

Please could you add steps on how you specifically intend to do this to the Test Plan?

I asked around the SRU team for a second opinion, and I got back some additional concerns:

0) This should probably have received a review from the security team before uploading to the development release.

1) If it filtered out control characters and restricted itself to UTF-8 printable that would be less scary. Could you consider that, please?

2) What happens if the client and server encodings do not agree? This isn't entirely theoretical. I'm told that Japan has a national encoding standard that is not Unicode wh...

[Marco Trevisan (Treviño) (3v1n0)]

> Please add a verification that your final purpose actually works to the Test Plan

Ok, I've added details how to test using Authd, linking to its documentation for all the setup details

> OK, but there are cases where we recommend doing that, such as for HOTP/TOTP configuration: https://ubuntu.com/server/docs/openssh-server. Maybe it's worth adding to the Test Plan to ensure that those steps still work?

Done

> Please could you add steps on how you specifically intend to do this to the Test Plan?

Banners added too

0) IIRC one of the reviewers of the MR was part of the security team at the time it was reviewed

1) Yep, that's definitely something that I should also propose upstream, although that's something that as said SSHd didn't care so far yet.

2) This is something to test, however the characters we care about here are in a group that should generally be available everywhere (even in very limited terminals character sets).

[Steve Langasek (vorlon)]

Ok that's correct, at the time of review Tobias was a member of the security team so I think we can take that as the necessary sign-off.

[Steve Langasek (vorlon)]

Also it doesn't matter if these characters are part of all the character sets if they're passed through to the client terminal in the wrong ENCODING. non ASCII characters almost entirely do not have the same binary representation in UTF-8 as in other character sets, including these.

[Timo Aaltonen (tjaalton)]

please rebase for focal/jammy on current -security

[Timo Aaltonen (tjaalton)]

An upload of openssh to focal-proposed has been rejected from the upload queue for the following reason: "rebase on current focal-security".

[Lukas Märdian (slyon)]

For Jammy, please consider rebasing on top of https://code.launchpad.net/~slyon/ubuntu/+source/openssh/+git/openssh/+merge/478457

So we can bundle this and the other (GSSAPI) SRU into a single upload.