Several SSH (RFC 4252/4253/4254) Compliance Issues in wolfSSH

[Zhaodl1]

Hi, while reviewing wolfSSH's(e2bfa19) SSH protocol implementation against RFC 4252, 4253, and 4254, we noticed several places where the current behavior appears to differ from the specification. Each item below cites the relevant RFC text alongside the corresponding source code for your reference. We hope this is helpful for improving RFC conformance.

1. Diffie-Hellman Exchange Value Range Not Verified for Modular Groups

RFC Reference: RFC 4253 Section 8 (lines 1199-1201)

"Values of 'e' or 'f' that are not in the range [1, p-1] MUST NOT be sent or accepted by either side. If this condition is violated, the key exchange fails."

Analysis:
In DoKexDhInit, the client-supplied exchange value e is validated only by its encoded length (eSz bounds) before being copied into the handshake state and forwarded to KeyAgreeDh_server, where it is passed directly into wc_DhAgree to compute the shared secret. No check that the value lies in the range [1, p-1] is performed on this modular-exponentiation path (the curve25519 path separately validates the peer public value through wc_curve25519_check_public). As a result, an out-of-range e is currently accepted and a KEXDH_REPLY (containing f and the host-key signature) is produced, whereas the RFC states such values MUST NOT be accepted and the exchange must fail.

Source Code Evidence (src/internal.c):

// src/internal.c:4964-4972
    if (ret == WS_SUCCESS) {
        /* Validate eSz */
        if (eSz > (word32)sizeof(ssh->handshake->e) || eSz == 0)
            ret = WS_PUBKEY_REJECTED_E;     // length-only validation
    }

    if (ret == WS_SUCCESS) {
        WMEMCPY(ssh->handshake->e, e, eSz);
        ssh->handshake->eSz = eSz;

// src/internal.c:12180-12184  (KeyAgreeDh_server)
        if (ret == 0) {
            PRIVATE_KEY_UNLOCK();
            ret = wc_DhAgree(privKey, ssh->k, &ssh->kSz, y_ptr, ySz,
                    ssh->handshake->e, ssh->handshake->eSz);   // e used without range check
            PRIVATE_KEY_LOCK();
        }

---

2. "forwarded-tcpip" Channel Open Not Verified Against a Prior Remote-Forward Request

RFC Reference: RFC 4254 Section 7.2 (lines 975-977)

"Implementations MUST reject these messages unless they have previously requested a remote TCP/IP port forwarding with the given port number."

Analysis:
In DoChannelOpen, a CHANNEL_OPEN of type "forwarded-tcpip" is handled by DoChannelOpenForward, which parses the host/port/originator fields, and then falls through the common acceptance path to SendChannelOpenConf. The forward-callback (fwdCb) gate that governs "direct-tcpip" is applied only to ID_CHANTYPE_TCPIP_DIRECT; the "forwarded-tcpip" branch has no equivalent check, and the session does not maintain a record of previously-requested remote forwards to match the port against. A "forwarded-tcpip" channel is therefore confirmed without verifying that the local side previously requested remote forwarding for that port, whereas the RFC requires these messages to be rejected unless a prior request exists.

Source Code Evidence (src/internal.c):

// src/internal.c:9016-9020  (no per-port prior-forward verification)
            case ID_CHANTYPE_TCPIP_FORWARD:
                ret = DoChannelOpenForward(ssh,
                                &host, &hostPort, &origin, &originPort,
                                buf, len, &begin);
                break;

// src/internal.c:9061  (the fwdCb gate applies to the DIRECT type only)
            if (typeId == ID_CHANTYPE_TCPIP_DIRECT) {
                ...
                if (ssh->ctx->fwdCb) { ... }

// src/internal.c:9091-9093  (reached for forwarded-tcpip on the common path)
    if (ret == WS_SUCCESS) {
        ret = SendChannelOpenConf(ssh, newChannel);
    }

---

3. "tcpip-forward" / "cancel-tcpip-forward" Global Requests Handled Without a Role Guard

RFC Reference: RFC 4254 Section 7.1 (lines 928-929 for tcpip-forward, lines 949-950 for cancel-tcpip-forward)

"Client implementations SHOULD reject these messages; they are normally only sent by the client."

Analysis:
DoGlobalRequest dispatches both "tcpip-forward" and "cancel-tcpip-forward" to DoGlobalRequestFwd regardless of the endpoint role. When wantReply is set and the registered forward callback succeeds, DoGlobalRequestFwd transmits REQUEST_SUCCESS (or REQUEST_FAILURE on failure). No role-based guard restricts these requests to the server side. A client-side endpoint that has registered a forward callback therefore processes a "tcpip-forward" global request received from the peer and responds affirmatively, whereas the RFC notes these messages are normally only sent by the client and SHOULD be rejected by client implementations.

Source Code Evidence (src/internal.c):

// src/internal.c:8875-8883
        switch (globReqId) {
#ifdef WOLFSSH_FWD
            case ID_GLOBREQ_TCPIP_FWD:
                ret = DoGlobalRequestFwd(ssh, buf, len, &begin, wantReply, 0);
                wantReply = 0;
                break;
            case ID_GLOBREQ_TCPIP_FWD_CANCEL:
                ret = DoGlobalRequestFwd(ssh, buf, len, &begin, wantReply, 1);
                wantReply = 0;
                break;

// src/internal.c:8817-8826
    if (wantReply) {
        if (ret == WS_SUCCESS) {
            if (isCancel) {
                ret = SendRequestSuccess(ssh, 1);
            }
            else {
                ret = SendGlobalRequestFwdSuccess(ssh, 1, bindPort);  // MSGID_REQUEST_SUCCESS
            }
        }
        else {
            ret = SendRequestSuccess(ssh, 0);                         // MSGID_REQUEST_FAILURE
        }
    }

---

4. "direct-tcpip" Channel Open Accepted When a Forward Callback Opts In

RFC Reference: RFC 4254 Section 7.2 (lines 1015-1016)

"Client implementations SHOULD reject direct TCP/IP open requests for security reasons."

Analysis:
In DoChannelOpen, a CHANNEL_OPEN of type "direct-tcpip" is rejected with OPEN_ADMINISTRATIVELY_PROHIBITED when no forward callback (fwdCb) is registered. When an application has registered a fwdCb that returns WS_SUCCESS for WOLFSSH_FWD_LOCAL_SETUP, the channel is accepted and SendChannelOpenConf emits CHANNEL_OPEN_CONFIRMATION. No role-based guard distinguishes client from server on this path. The default configuration therefore rejects these requests; acceptance occurs only through an application-provided callback, whereas the RFC suggests client implementations SHOULD reject direct TCP/IP open requests for security reasons.

Source Code Evidence (src/internal.c):

// src/internal.c:9061-9077
            if (typeId == ID_CHANTYPE_TCPIP_DIRECT) {
                ChannelUpdateForward(newChannel,
                        host, hostPort, origin, originPort, isDirect);

                if (ssh->ctx->fwdCb) {
                    ret = ssh->ctx->fwdCb(WOLFSSH_FWD_LOCAL_SETUP,
                            ssh->fwdCbCtx, host, hostPort);
                    if (ret == WS_SUCCESS) {
                        ret = ssh->ctx->fwdCb(WOLFSSH_FWD_CHANNEL_ID,
                                ssh->fwdCbCtx, NULL, newChannel->channel);
                    }
                }
                else {
                    WLOG(WS_LOG_WARN, "No forward callback set for direct-tcpip channel,"
                            " failing channel open");
                    fail_reason = OPEN_ADMINISTRATIVELY_PROHIBITED;
                    ret = WS_ERROR;
                }
            }

---

5. Binary Packet Padding Length Below Four Bytes Not Rejected on Receive

RFC Reference: RFC 4253 Section 6 (lines 399-401)

"There MUST be at least four bytes of padding. The padding SHOULD consist of random bytes. The maximum amount of padding is 255 bytes."

Analysis:
On the receive path, DoPacket reads the padding_length byte (padSz) and validates only that it fits within the declared packet length ((PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > ssh->curSz); no check that padSz >= 4 is performed. The send path (PreparePacket) separately enforces a minimum of MIN_PAD_LENGTH (4), so the constant exists but is not applied to received packets. A received packet with fewer than four padding bytes is therefore accepted and dispatched to its message handler, whereas the RFC requires at least four bytes of padding.

Source Code Evidence (src/internal.c):

// src/internal.c:10077-10086
    idx += UINT32_SZ;
    padSz = buf[idx++];

    /* check for underflow */
    if ((word32)(PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > ssh->curSz) {
        return WS_OVERFLOW_E;            // bounds-only check; no padSz >= 4
    }

    payloadSz = ssh->curSz - PAD_LENGTH_SZ - padSz - MSG_ID_SZ;

---

6. Password-Change Request Handled as Ordinary Password Authentication

RFC Reference: RFC 4252 Section 8 (lines 585-589)

"However, if the password has expired, the server SHOULD indicate this by responding with SSH_MSG_USERAUTH_PASSWD_CHANGEREQ. In any case, the server MUST NOT allow an expired password to be used for authentication."

Analysis:
DoUserAuthRequestPassword parses the client's password-change flag (hasNewPassword) and, when set, parses the optional newPassword field; the in-code comment notes that password-change handling is not currently supported. The function then invokes the user-auth callback with the original password and, on success, advances toward SSH_MSG_USERAUTH_SUCCESS. MSGID_USERAUTH_PW_CHRQ (60) is defined but not sent on this path. A request carrying the change flag is therefore handled like an ordinary password authentication, whereas the RFC states an expired password MUST NOT be used for authentication and prefers a PASSWD_CHANGEREQ response.

Source Code Evidence (src/internal.c):

// src/internal.c:7229,7235-7239
        ret = GetBoolean(&pw->hasNewPassword, buf, len, &begin);   // parses the change flag
    ...
    if (ret == WS_SUCCESS) {
        if (pw->hasNewPassword) {
            /* Skip the password change. Maybe error out since we aren't
             * supporting password changes at this time. */
            ret = GetStringRef(&pw->newPasswordSz, &pw->newPassword,
                    buf, len, &begin);   // field parsed; no CHANGEREQ, no rejection
        }

---

7. High-Numbered Message Before Authentication Closed Locally Without a Disconnect Message

RFC Reference: RFC 4252 Section 6 (lines 409-413)

"Message numbers of 80 and higher are reserved for protocols running after this authentication protocol, so receiving one of them before authentication is complete is an error, to which the server MUST respond by disconnecting, preferably with a proper disconnect message sent to ease troubleshooting."

Analysis:
IsMessageAllowedServer rejects message numbers >= 80 when authentication is incomplete (returning 0), and DoReceive converts the resulting WS_MSGID_NOT_ALLOWED_E into a local WS_FATAL_ERROR that tears down the connection. SendDisconnect is invoked only for an invalid service name (within DoServiceRequest/DoServiceAccept); it is not invoked on the high-numbered-message path. The connection is therefore closed locally without transmitting an SSH_MSG_DISCONNECT message, whereas the RFC prefers "a proper disconnect message sent to ease troubleshooting."

Source Code Evidence (src/internal.c):

// src/internal.c:715-724
    /* Is server userauth complete? */
    if (ssh->acceptState < ACCEPT_SERVER_USERAUTH_SENT) {
        ...
        if (MSGIDLIMIT_POST_USERAUTH(msg)) {           // msg >= 80
            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
                    msg, "server", "before user authentication is complete");
            return 0;                                   // no SendDisconnect on this path
        }

---