diff --git a/ssh.h b/ssh.h index 56bd4339..1cdfe3b8 100644 --- a/ssh.h +++ b/ssh.h @@ -1412,6 +1412,7 @@ void platform_ssh_share_cleanup(const char *name); X(y, SSH2_MSG_DEBUG, 4) \ X(y, SSH2_MSG_SERVICE_REQUEST, 5) \ X(y, SSH2_MSG_SERVICE_ACCEPT, 6) \ + X(y, SSH2_MSG_EXT_INFO, 7) \ X(y, SSH2_MSG_KEXINIT, 20) \ X(y, SSH2_MSG_NEWKEYS, 21) \ K(y, SSH2_MSG_KEXDH_INIT, 30, SSH2_PKTCTX_DHGROUP) \ diff --git a/ssh2bpp-bare.c b/ssh2bpp-bare.c index 93a7fa16..90f196e8 100644 --- a/ssh2bpp-bare.c +++ b/ssh2bpp-bare.c @@ -110,6 +110,18 @@ static void ssh2_bare_bpp_handle_input(BinaryPacketProtocol *bpp) s->packetlen--; BinarySource_INIT(s->pktin, s->data, s->packetlen); + if (s->pktin->type == SSH2_MSG_EXT_INFO) { + /* + * Mild layer violation: EXT_INFO is not permitted in the + * bare ssh-connection protocol. Faulting it here means + * that ssh2_common_filter_queue doesn't receive it in the + * first place unless it's legal to have sent it. + */ + ssh_proto_error(s->bpp.ssh, "Remote side sent SSH2_MSG_EXT_INFO " + "in bare connection protocol"); + return; + } + /* * Log incoming packet, possibly omitting sensitive fields. */ diff --git a/ssh2bpp.c b/ssh2bpp.c index 756eac47..09b23e5a 100644 --- a/ssh2bpp.c +++ b/ssh2bpp.c @@ -37,6 +37,9 @@ struct ssh2_bpp_state { bool is_server; bool pending_newkeys; bool pending_compression, seen_userauth_success; + bool enforce_next_packet_is_userauth_success; + unsigned nnewkeys; + int prev_type; BinaryPacketProtocol bpp; }; @@ -594,9 +597,25 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) { int type = s->pktin->type; + int prev_type = s->prev_type; + s->prev_type = type; s->pktin = NULL; + if (s->enforce_next_packet_is_userauth_success) { + /* See EXT_INFO handler below */ + if (type != SSH2_MSG_USERAUTH_SUCCESS) { + ssh_proto_error(s->bpp.ssh, + "Remote side sent SSH2_MSG_EXT_INFO " + "not either preceded by NEWKEYS or " + "followed by USERAUTH_SUCCESS"); + return; + } + s->enforce_next_packet_is_userauth_success = false; + } + if (type == SSH2_MSG_NEWKEYS) { + if (s->nnewkeys < 2) + s->nnewkeys++; /* * Mild layer violation: in this situation we must * suspend processing of the input byte stream until @@ -627,6 +646,44 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) s->seen_userauth_success = true; } + if (type == SSH2_MSG_EXT_INFO) { + /* + * And another: enforce that an incoming EXT_INFO is + * either the message immediately after the initial + * NEWKEYS, or (if we're the client) the one + * immediately before USERAUTH_SUCCESS. + */ + if (prev_type == SSH2_MSG_NEWKEYS && s->nnewkeys == 1) { + /* OK - this is right after the first NEWKEYS. */ + } else if (s->is_server) { + /* We're the server, so they're the client. + * Clients may not send EXT_INFO at _any_ other + * time. */ + ssh_proto_error(s->bpp.ssh, + "Remote side sent SSH2_MSG_EXT_INFO " + "that was not immediately after the " + "initial NEWKEYS"); + return; + } else if (s->nnewkeys > 0 && s->seen_userauth_success) { + /* We're the client, so they're the server. In + * that case they may also send EXT_INFO + * immediately before USERAUTH_SUCCESS. Error out + * immediately if this can't _possibly_ be that + * moment (because we haven't even seen NEWKEYS + * yet, or because we've already seen + * USERAUTH_SUCCESS). */ + ssh_proto_error(s->bpp.ssh, + "Remote side sent SSH2_MSG_EXT_INFO " + "after USERAUTH_SUCCESS"); + return; + } else { + /* This _could_ be OK, provided the next packet is + * USERAUTH_SUCCESS. Set a flag to remember to + * fault it if not. */ + s->enforce_next_packet_is_userauth_success = true; + } + } + if (s->pending_compression && userauth_range(type)) { /* * Receiving any userauth message at all indicates diff --git a/ssh2transport.c b/ssh2transport.c index 7d2b1631..4bc45824 100644 --- a/ssh2transport.c +++ b/ssh2transport.c @@ -378,6 +378,37 @@ bool ssh2_common_filter_queue(PacketProtocolLayer *ppl) pq_pop(ppl->in_pq); break; + case SSH2_MSG_EXT_INFO: { + /* + * The BPP enforces that these turn up only at legal + * points in the protocol. In particular, it will not pass + * an EXT_INFO on to us if it arrives before encryption is + * enabled (which is when a MITM could inject one + * maliciously). + * + * However, one of the criteria for legality is that a + * server is permitted to send this message immediately + * _before_ USERAUTH_SUCCESS. So we may receive this + * message not yet knowing whether it's legal to have sent + * it - we won't know until the BPP processes the next + * packet. + * + * But that should be OK, because firstly, an + * out-of-sequence EXT_INFO that's still within the + * encrypted session is only a _protocol_ violation, not + * an attack; secondly, any data we set in response to + * such an illegal EXT_INFO won't have a chance to affect + * the session before the BPP aborts it anyway. + */ + uint32_t nexts = get_uint32(pktin); + for (uint32_t i = 0; i < nexts && !get_err(pktin); i++) { + ptrlen extname = get_string(pktin); + ptrlen extvalue = get_string(pktin); + } + pq_pop(ppl->in_pq); + break; + } + default: return false; } @@ -762,6 +793,12 @@ static void ssh2_write_kexinit_lists( add_to_commasep(list, kexlists[i][j].name); } } + if (i == KEXLIST_KEX) { + if (our_hostkeys) /* we're the server */ + add_to_commasep(list, "ext-info-s"); + else /* we're the client */ + add_to_commasep(list, "ext-info-c"); + } put_stringsb(pktout, list); } /* List client->server languages. Empty list. */ diff --git a/sshbpp.h b/sshbpp.h index 45662b62..87e7d7e7 100644 --- a/sshbpp.h +++ b/sshbpp.h @@ -36,7 +36,10 @@ struct BinaryPacketProtocol { * the callback on out_pq. */ IdempotentCallback ic_out_pq; + /* Information that all packet layers sharing this BPP will + * potentially be interested in. */ int remote_bugs; + bool ext_info_rsa_sha256_ok, ext_info_rsa_sha512_ok; /* Set this if remote connection closure should not generate an * error message (either because it's not to be treated as an