Subject: OpenSSH sabotages protocol extension

From: denis bider <[email-masked]>
Date: Thu, 5 Nov 2015 03:25:36 +0000

Well, I'm slightly pissed.

Why does OpenSSL do stupid shit like this?

    type = packet_read();
    if (type != SSH2_MSG_SERVICE_ACCEPT)
        fatal("Server denied authentication request: %d", type);
    if (packet_remaining() > 0) {
        char *reply = packet_get_string(NULL);
        debug2("service_accept: %s", reply);
        free(reply);
    } else {
        debug2("buggy server: service_accept w/o service");
    }
    packet_check_eom();
    debug("SSH2_MSG_SERVICE_ACCEPT received");

Note the genius inclusion of packet_check_eom() after decoding SERVICE_ACCEPT. Guess what this line does?

    #define ssh_packet_check_eom(ssh) \
    do { \
        int _len = ssh_packet_remaining(ssh); \
        if (_len > 0) { \
            logit("Packet integrity error (%d bytes remaining) at %s:%d", \
                _len ,__FILE__, __LINE__); \
            ssh_packet_disconnect(ssh, \
                "Packet integrity error."); \
        } \
    } while (0)

    #define packet_check_eom() \
        ssh_packet_check_eom(active_state)

Yes. It disconnects if there's any extra data after the recognized field in SERVICE_ACCEPT.

What possible purpose does this serve?

What possible purpose at all, other than to sabotage future extension?

Thanks to this, we cannot add a field to SERVICE_ACCEPT so that the server could advertise what signature algorithms it accepts for user authentication.

Thank you, OpenSSH. /s

Again.

---

From: denis bider <[email-masked]>
Date: Thu, 5 Nov 2015 03:36:11 +0000

Obviously, same thing for SSH2_MSG_USERAUTH_FAILURE:

    authlist = packet_get_string(NULL);
    partial = packet_get_char();
    packet_check_eom();

Awesome work, guys! You have locked down every possible clean way that the protocol could be extended, so as to spare the client several round-trips to discover which signing algorithms it can use for RSA keys.

You've made it so that the only way to convey such information, and not risk a client disconnect, is to use a specially crafted SSH_MSG_IGNORE.

Tremendous, just tremendous.

denis bider <[email-masked]> , 11/5/2015 3:21 AM:
Well, I'm slightly pissed.

    type = packet_read();

Thank you, OpenSSH. /s

Again.

---

From: [email-masked] (Niels Möller)
Date: Fri, 06 Nov 2015 06:30:45 +0100

> What possible purpose at all, other than to sabotage future extension?

FYI, my implementation does the same. To me, the spec is pretty clear
that a SSH_MSG_SERVICE_ACCEPT can't include any extra data (unlike,
e.g., SSH_MSG_REQUEST_SUCCESS, SSH_MSG_CHANNEL_OPEN, and
SSH_MSG_CHANNEL_OPEN_CONFIRMATION).

There's the liberal tradition in protocol implementation to allow random
garbage at the end of messages. This has it's merit in some cases, but
in security protocols I tend to require that the protcol is adhered to
to the last bit.

Regards,
/Niels

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid C0B98E26.
Internet email is subject to wholesale government surveillance.

---

From: Peter Gutmann <[email-masked]>
Date: Fri, 6 Nov 2015 09:02:09 +0000

>What possible purpose does this serve?

It's perfectly sensible, if the spec requires that a packet be x, y, z then
getting a packet containing x, y, z, extra garbage is at best a sign of data
corruption, at worst a sign of an active attack.  Rejecting the packet and
closing the connection is good practice, it makes it harder for an attacker to
use you as an oracle.

For an example of what happens if you do ignore extra garbage at the end of
your data, look at the padding attacks on PKCS #1...

Peter.

---

From: Damien Miller <[email-masked]>
Date: Fri, 6 Nov 2015 13:19:59 +1100 (AEDT)

RFC4242 section 5.1. I'll quote it for you:

> 5.1.  Responses to Authentication Requests
> 
>    If the server rejects the authentication request, it MUST respond
>    with the following:
> 
>       byte         SSH_MSG_USERAUTH_FAILURE
>       name-list    authentications that can continue
>       boolean      partial success

Note the lack of extension fields and the "MUST".

> Awesome work, guys! You have locked down every possible clean way that the
> protocol could be extended, so as to spare the client several round-trips to
> discover which signing algorithms it can use for RSA keys.

It sounds like you want to redefine the information flow of public key
authentication. If you want to do this (and I don't think it is a good
idea - I'll comment separately on the draft), then the right way to do
it is to design a new authentication method that can be negotiated by
implementations that support it.

By implementing a new "publickey2" (or whatever) method, you get to
define new SSH_MSG_USERAUTH messages, which is basically what you are
trying to do here.

> You've made it so that the only way to convey such information, and not risk
> a client disconnect, is to use a specially crafted SSH_MSG_IGNORE.

Again, RFC4252 made it this way. We just implemented it as it was written.

-d

---

From: Damien Miller <[email-masked]>
Date: Fri, 6 Nov 2015 13:19:42 +1100 (AEDT)

> Why does OpenSSL do stupid shit like this?

[snip]

> Note the genius inclusion of packet_check_eom() after decoding
> SERVICE_ACCEPT. Guess what this line does?
> 
...
>                 "Packet integrity error."); \
...
> What possible purpose does this serve?

It verifies that the message is conformant to RFC4253 section 10 (which
specifies only a single string in a SSH_MSG_SERVICE_REQUEST) and not
gibberish.

> What possible purpose at all, other than to sabotage future extension?

... because an implementation of a security protocol that runs with
high privilege should strongly validate well-formedness.

> Thanks to this, we cannot add a field to SERVICE_ACCEPT so that the server
> could advertise what signature algorithms it accepts for user
> authentication.

No, you can't add one because RFC4253 specified it differently and you
don't get to retcon protocols by hoping that implementations don't
validate their input.

-d