diff --git a/src/internal.c b/src/internal.c index 636cde839..ed700748f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8014,6 +8014,11 @@ static int DoUserAuthRequestNone(WOLFSSH* ssh, WS_UserAuthData* authData, } if (ret == WS_SUCCESS) { + /* The opening "none" probe every client sends to learn the method list + * is exempt from the auth-attempt cap, matching the invalid-method + * else-branch in DoUserAuthRequest() and the documented contract. */ + int countIt = (ssh->authRequests != 1); + authData->type = WOLFSSH_USERAUTH_NONE; if (ssh->ctx->userAuthCb != NULL) { WLOG(WS_LOG_DEBUG, "DUARN: Calling the userauth callback"); @@ -8029,7 +8034,7 @@ static int DoUserAuthRequestNone(WOLFSSH* ssh, WS_UserAuthData* authData, #ifndef NO_FAILURE_ON_REJECTED /* Count before failing: a send that blocks returns * WS_WANT_WRITE, which isn't fatal on its own. */ - (void)SendUserAuthFailureCount(ssh, 0, 1); + (void)SendUserAuthFailureCount(ssh, 0, countIt); #endif ret = WS_USER_AUTH_E; } @@ -8039,12 +8044,12 @@ static int DoUserAuthRequestNone(WOLFSSH* ssh, WS_UserAuthData* authData, } else { WLOG(WS_LOG_DEBUG, "DUARN: none check failed, retry"); - ret = SendUserAuthFailureCount(ssh, 0, 1); + ret = SendUserAuthFailureCount(ssh, 0, countIt); } } else { WLOG(WS_LOG_DEBUG, "DUARN: No user auth callback"); - (void)SendUserAuthFailureCount(ssh, 0, 1); + (void)SendUserAuthFailureCount(ssh, 0, countIt); ret = WS_FATAL_ERROR; } } @@ -8202,7 +8207,7 @@ static int DoUserAuthInfoResponse(WOLFSSH* ssh, } } else if (ret == WOLFSSH_USERAUTH_SUCCESS_ANOTHER) { - ret = SendUserAuthKeyboardRequest(ssh, &authData, 0); + ret = SendUserAuthKeyboardRequest(ssh, &authData); } else if (ret == WS_SUCCESS) { ssh->clientState = CLIENT_USERAUTH_DONE; @@ -9781,25 +9786,31 @@ static int DoUserAuthRequest(WOLFSSH* ssh, ssh->authId = authNameId; ssh->authRequests++; +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE + /* An outstanding INFO_REQUEST the peer abandons is charged as a failed + * attempt no matter which method it switches to next; charging only + * inside the keyboard branch below let a peer dodge the cap by + * interleaving abandoned keyboard exchanges with other requests. The + * new request dispatched below still counts its own failure. */ + if (ssh->kbSetupPending) { + ssh->kbSetupPending = 0; + ret = CountUserAuthFailure(ssh); + } +#endif + /* Each method branch counts its own failures; it can't be centralized * here since handlers return WS_SUCCESS for both success and * failure-sent. */ - if (authNameId == ID_USERAUTH_PASSWORD) + if (ret != WS_SUCCESS) { + /* Charging the abandoned keyboard exchange hit the cap and the + * disconnect is already sent; don't dispatch this request. */ + begin = len; + } + else if (authNameId == ID_USERAUTH_PASSWORD) ret = DoUserAuthRequestPassword(ssh, &authData, buf, len, &begin); #ifdef WOLFSSH_KEYBOARD_INTERACTIVE else if (authNameId == ID_USERAUTH_KEYBOARD) { - int counted = 0; - - /* Restarting before answering the prior INFO_REQUEST counts as a - * failure; else a peer loops here without ever responding. Clear - * the flag first so the send below doesn't charge it twice. */ - if (ssh->kbSetupPending) { - ssh->kbSetupPending = 0; - counted = 1; - ret = CountUserAuthFailure(ssh); - } - if (ret == WS_SUCCESS) - ret = SendUserAuthKeyboardRequest(ssh, &authData, counted); + ret = SendUserAuthKeyboardRequest(ssh, &authData); } #endif #if !defined(WOLFSSH_NO_RSA) || !defined(WOLFSSH_NO_ECDSA) \ @@ -16242,8 +16253,7 @@ static int BuildUserAuthRequestKeyboard(WOLFSSH* ssh, byte* output, word32* idx, return ret; } -int SendUserAuthKeyboardRequest(WOLFSSH* ssh, WS_UserAuthData* authData, - int counted) +int SendUserAuthKeyboardRequest(WOLFSSH* ssh, WS_UserAuthData* authData) { byte* output; word32 idx; @@ -16279,10 +16289,10 @@ int SendUserAuthKeyboardRequest(WOLFSSH* ssh, WS_UserAuthData* authData, else { WLOG(WS_LOG_DEBUG, "Issue with keyboard auth setup, try another " "auth type"); - /* No INFO_REQUEST sent, so charge the rejection here unless the - * caller already did; else a peer loops unbounded. */ + /* No INFO_REQUEST sent, so charge the setup rejection here; else a + * peer loops unbounded on rejected setups. */ ssh->kbSetupPending = 0; - return SendUserAuthFailureCount(ssh, 0, !counted); + return SendUserAuthFailureCount(ssh, 0, 1); } } diff --git a/tests/api.c b/tests/api.c index 7d8ec0ffe..577e134b4 100644 --- a/tests/api.c +++ b/tests/api.c @@ -4004,6 +4004,18 @@ static void test_wolfSSH_CheckAlgoList(void) #ifndef WOLFSSH_ALLOW_NONE_CIPHER AssertIntEQ(wolfSSH_CTX_SetAlgoListCipher(ctx, "none"), WS_INVALID_ALGO_ID); AssertIntEQ(wolfSSH_CTX_SetAlgoListMac(ctx, "none"), WS_INVALID_ALGO_ID); +#else + /* With --enable-none-cipher, "none" is a valid cipher/MAC selection (an + * insecure, testing-only plaintext transport) and must round-trip through + * the setters. The setters store the caller's pointer, so a static literal + * is used and a real list restored below before listBuf goes out of scope. */ + AssertIntEQ(wolfSSH_CTX_SetAlgoListCipher(ctx, "none"), WS_SUCCESS); + AssertIntEQ(wolfSSH_CTX_SetAlgoListMac(ctx, "none"), WS_SUCCESS); + AssertStrEQ(wolfSSH_CTX_GetAlgoListCipher(ctx), "none"); + AssertStrEQ(wolfSSH_CTX_GetAlgoListMac(ctx), "none"); + /* "none" mixed into a list is still accepted alongside a real cipher. */ + WSNPRINTF(listBuf, sizeof(listBuf), "none,%s", validCipher); + AssertIntEQ(wolfSSH_CTX_SetAlgoListCipher(ctx, listBuf), WS_SUCCESS); #endif /* Restore a static list; the setters store the caller's pointer and diff --git a/tests/unit.c b/tests/unit.c index 3bd6b18de..08998a1ea 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -3783,6 +3783,48 @@ static int UnitAuthAlwaysFail(byte authType, WS_UserAuthData* authData, return WOLFSSH_USERAUTH_INVALID_PASSWORD; } +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE +/* Keyboard setup that accepts, sending an INFO_REQUEST with no prompts so the + * exchange goes outstanding (kbSetupPending). Every other outcome fails. */ +static int UnitAuthKbSetupOk(byte authType, WS_UserAuthData* authData, + void* ctx) +{ + (void)ctx; + if (authType == WOLFSSH_USERAUTH_KEYBOARD_SETUP) { + authData->sf.keyboard.promptCount = 0; + return WOLFSSH_USERAUTH_SUCCESS; + } + return WOLFSSH_USERAUTH_INVALID_PASSWORD; +} + +/* Build a USERAUTH_REQUEST payload for auth method `method` with no method- + * specific fields, as DoUserAuthRequest() sees it with the message id already + * consumed. The keyboard and none dispatch paths don't parse trailing fields, + * so three strings (user, service, method) suffice. Returns the payload size, + * 0 on overflow. */ +static word32 BuildAuthMethodRequest(byte* buf, word32 bufSz, + const char* method) +{ + const char* fields[3]; + word32 idx = 0, i, fieldSz; + + fields[0] = "jill"; + fields[1] = "ssh-connection"; + fields[2] = method; + + for (i = 0; i < 3; i++) { + fieldSz = (word32)WSTRLEN(fields[i]); + if (idx + UINT32_SZ + fieldSz > bufSz) + return 0; + PutU32BE(buf + idx, fieldSz); + idx += UINT32_SZ; + WMEMCPY(buf + idx, fields[i], fieldSz); + idx += fieldSz; + } + return idx; +} +#endif /* WOLFSSH_KEYBOARD_INTERACTIVE */ + /* Build a "password" USERAUTH_REQUEST payload, as DoUserAuthRequest() sees it * with the message id already consumed. Returns the payload size. */ static word32 BuildAuthPwRequest(byte* buf, word32 bufSz) @@ -3874,6 +3916,160 @@ static int test_MaxAuthAttempts(void) wolfSSH_CTX_free(ctx); } + /* The opening "none" probe clients send to learn the method list is exempt + * from the cap. With the limit at 1: the first "none" must leave the + * connection up (not counted), and the second must be counted and trip the + * cap. This covers the invalid-method else-branch, and DoUserAuthRequestNone + * under WOLFSSH_ALLOW_USERAUTH_NONE, both of which share the exemption. */ + if (result == 0) { + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte noneReq[64]; + word32 noneReqSz = 0; + const char* fields[3]; + word32 idx = 0, f, fieldSz; + + fields[0] = "jill"; fields[1] = "ssh-connection"; fields[2] = "none"; + for (f = 0; f < 3; f++) { + fieldSz = (word32)WSTRLEN(fields[f]); + PutU32BE(noneReq + idx, fieldSz); + idx += UINT32_SZ; + WMEMCPY(noneReq + idx, fields[f], fieldSz); + idx += fieldSz; + } + noneReqSz = idx; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) { result = -740; } + else { + wolfSSH_SetUserAuth(ctx, UnitAuthAlwaysFail); + wolfSSH_SetIOSend(ctx, UnitIoSendSink); + if (wolfSSH_CTX_SetMaxAuthAttempts(ctx, 1) != WS_SUCCESS) + result = -741; + ssh = (result == 0) ? wolfSSH_new(ctx) : NULL; + if (result == 0 && ssh == NULL) + result = -742; + if (result == 0) { + word32 di = 0; + int ret = wolfSSH_TestDoUserAuthRequest(ssh, noneReq, + noneReqSz, &di); + if (ret != WS_SUCCESS) { + printf("MaxAuthAttempts[none]: probe ret=%d expected %d\n", + ret, WS_SUCCESS); + result = -743; + } + } + if (result == 0) { + word32 di = 0; + int ret = wolfSSH_TestDoUserAuthRequest(ssh, noneReq, + noneReqSz, &di); + if (ret != WS_USER_AUTH_E) { + printf("MaxAuthAttempts[none]: 2nd ret=%d expected %d\n", + ret, WS_USER_AUTH_E); + result = -744; + } + } + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + } + } + +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE + /* An abandoned keyboard-interactive exchange is charged no matter which + * method the peer switches to next. With the limit at 2: an accepted + * keyboard request leaves an INFO_REQUEST outstanding (uncounted), then a + * failed password request charges both the abandoned exchange and itself, + * tripping the cap on that single password request. Before the fix the + * abandoned exchange escaped counting on a non-keyboard follow-up and the + * password returned WS_SUCCESS. */ + if (result == 0) { + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte kbReq[64]; + word32 kbReqSz = BuildAuthMethodRequest(kbReq, (word32)sizeof(kbReq), + "keyboard-interactive"); + + if (kbReqSz == 0) + result = -750; + + ctx = (result == 0) ? wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL) + : NULL; + if (result == 0 && ctx == NULL) result = -751; + if (result == 0) { + wolfSSH_SetUserAuth(ctx, UnitAuthKbSetupOk); + wolfSSH_SetIOSend(ctx, UnitIoSendSink); + if (wolfSSH_CTX_SetMaxAuthAttempts(ctx, 2) != WS_SUCCESS) + result = -752; + ssh = (result == 0) ? wolfSSH_new(ctx) : NULL; + if (result == 0 && ssh == NULL) result = -753; + + if (result == 0) { + word32 di = 0; + int ret = wolfSSH_TestDoUserAuthRequest(ssh, kbReq, kbReqSz, + &di); + if (ret != WS_SUCCESS) { + printf("MaxAuthAttempts[kb]: setup ret=%d expected %d\n", + ret, WS_SUCCESS); + result = -754; + } + } + if (result == 0) { + word32 di = 0; + int ret = wolfSSH_TestDoUserAuthRequest(ssh, request, + requestSz, &di); + if (ret != WS_USER_AUTH_E) { + printf("MaxAuthAttempts[kb]: pw-after-abandon ret=%d " + "expected %d\n", ret, WS_USER_AUTH_E); + result = -755; + } + } + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + } + } + + /* A rejected keyboard setup (no INFO_REQUEST sent) is charged. With the + * limit at 2: the first rejected request leaves the connection up, the + * second trips the cap. */ + if (result == 0) { + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte kbReq[64]; + word32 kbReqSz = BuildAuthMethodRequest(kbReq, (word32)sizeof(kbReq), + "keyboard-interactive"); + int attempt; + + if (kbReqSz == 0) + result = -760; + + ctx = (result == 0) ? wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL) + : NULL; + if (result == 0 && ctx == NULL) result = -761; + if (result == 0) { + wolfSSH_SetUserAuth(ctx, UnitAuthAlwaysFail); /* setup rejects */ + wolfSSH_SetIOSend(ctx, UnitIoSendSink); + if (wolfSSH_CTX_SetMaxAuthAttempts(ctx, 2) != WS_SUCCESS) + result = -762; + ssh = (result == 0) ? wolfSSH_new(ctx) : NULL; + if (result == 0 && ssh == NULL) result = -763; + + for (attempt = 1; attempt <= 2 && result == 0; attempt++) { + word32 di = 0; + int expect = (attempt < 2) ? WS_SUCCESS : WS_USER_AUTH_E; + int ret = wolfSSH_TestDoUserAuthRequest(ssh, kbReq, kbReqSz, + &di); + if (ret != expect) { + printf("MaxAuthAttempts[kb-reject]: attempt %d ret=%d " + "expected %d\n", attempt, ret, expect); + result = -764 - attempt; + } + } + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + } + } +#endif /* WOLFSSH_KEYBOARD_INTERACTIVE */ + return result; } diff --git a/wolfssh/internal.h b/wolfssh/internal.h index e57f84116..d7fc355f5 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1322,7 +1322,7 @@ WOLFSSH_LOCAL int SendUserAuthKeyboardResponse(WOLFSSH* ssh); * caller already charged a failure for this message, so a declined setup * callback doesn't charge it twice. */ WOLFSSH_LOCAL int SendUserAuthKeyboardRequest(WOLFSSH* ssh, - WS_UserAuthData* authData, int counted); + WS_UserAuthData* authData); #endif WOLFSSH_LOCAL int SendUserAuthSuccess(WOLFSSH* ssh); WOLFSSH_LOCAL int SendUserAuthFailure(WOLFSSH* ssh, byte partialSuccess);