From a1e07e316139ff7f3b8f16427f9b176723c3045f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 23 Jun 2016 11:46:32 -0600 Subject: [PATCH 1/8] 1. Added extra debugging logs to the user authentication. 2. Reject invalid user names. 3. Fix the readme with regards to the public key log in testing. --- README.md | 8 +++++++- src/internal.c | 48 +++++++++++++++++++++++++++++++++++++++--------- wolfssh/error.h | 1 + 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index cd55524f..e87ad597 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,12 @@ If the characters are echoed twice, the client has local echo enabled. testing notes ------------- +After cloning the repository, be sure to make the testing private keys read- +only for the user, otherwise ssh_client will tell you to do it. + + $ chmod 0600 ./certs/key-gretel.pem ./certs/key-hansel.pem \ + ./certs/key-ecc.pem + Authentication against the example echoserver can be done with a password or public key. To use a password the command line: @@ -53,7 +59,7 @@ Where the `USER` and password pairs are: To use public key authentication use the command line: - $ ssh_client -l ./certs/key-USER.pem -p 22222 USER@localhost + $ ssh_client -i ./certs/key-USER.pem -p 22222 USER@localhost Where the user can be `gretel` or `hansel`. diff --git a/src/internal.c b/src/internal.c index dc9f7069..d3ea9dbe 100644 --- a/src/internal.c +++ b/src/internal.c @@ -129,6 +129,9 @@ const char* GetErrorString(int err) case WS_RESOURCE_E: return "insufficient resources for new channel"; + case WS_INVALID_USERNAME: + return "invalid user name"; + default: return "Unknown error code"; } @@ -1389,20 +1392,30 @@ static int DoUserAuthRequestPassword(WOLFSSH* ssh, WS_UserAuthData* authData, } if (ssh->ctx->userAuthCb != NULL) { - WLOG(WS_LOG_DEBUG, "DUAR: Checking the password"); + WLOG(WS_LOG_DEBUG, "DUARPW: Calling the userauth callback"); ret = ssh->ctx->userAuthCb(WOLFSSH_USERAUTH_PASSWORD, authData, ssh->userAuthCtx); - if (ret == WS_SUCCESS) { - WLOG(WS_LOG_DEBUG, "DUAR: password check successful"); + if (ret == WOLFSSH_USERAUTH_SUCCESS) { + WLOG(WS_LOG_DEBUG, "DUARPW: password check successful"); ssh->clientState = CLIENT_USERAUTH_DONE; } else { - WLOG(WS_LOG_DEBUG, "DUAR: password check failed"); - SendUserAuthFailure(ssh, 0); + WLOG(WS_LOG_DEBUG, "DUARPW: password check failed"); + if (ret != WOLFSSH_USERAUTH_SUCCESS) { + switch (ret) { + case WOLFSSH_USERAUTH_INVALID_USER: + SendDisconnect(ssh, + WOLFSSH_DISCONNECT_ILLEGAL_USER_NAME); + break; + default: + SendUserAuthFailure(ssh, 0); + } + } } } else { - WLOG(WS_LOG_DEBUG, "DUAR: No user auth callback"); + WLOG(WS_LOG_DEBUG, "DUARPW: No user auth callback"); + SendUserAuthFailure(ssh, 0); } *idx = begin; @@ -1425,6 +1438,7 @@ static int DoUserAuthRequestRsa(WOLFSSH* ssh, WS_UserAuthData_PublicKey* pk, uint32_t i = 0; int ret; + WLOG(WS_LOG_DEBUG, "Entering DoUserAuthRequestRsa()"); /* First check that the public key's type matches the one we are * expecting. */ GetUint32(&publicKeyTypeSz, pk->publicKey, pk->publicKeySz, &i); @@ -1475,6 +1489,8 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, WS_UserAuthData_PublicKey* pk = &authData->sf.publicKey; int ret = WS_SUCCESS; + WLOG(WS_LOG_DEBUG, "Entering DoUserAuthRequestPublicKey()"); + authData->type = WOLFSSH_USERAUTH_PUBLICKEY; GetBoolean(&pk->hasSignature, buf, len, &begin); GetUint32(&pk->publicKeyTypeSz, buf, len, &begin); @@ -1495,12 +1511,26 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, } if (ssh->ctx->userAuthCb != NULL) { + WLOG(WS_LOG_DEBUG, "DUARPK: Calling the userauth callback"); ret = ssh->ctx->userAuthCb(WOLFSSH_USERAUTH_PUBLICKEY, authData, ssh->userAuthCtx); + WLOG(WS_LOG_DEBUG, "DUARPK: callback result = %d", ret); + if (ret != WOLFSSH_USERAUTH_SUCCESS) { + switch (ret) { + case WOLFSSH_USERAUTH_INVALID_USER: + SendDisconnect(ssh, WOLFSSH_DISCONNECT_ILLEGAL_USER_NAME); + break; + default: + SendUserAuthFailure(ssh, 0); + } + } + } + else { + WLOG(WS_LOG_DEBUG, "DUARPK: no userauth callback set"); } if (pk->signature == NULL) { - WLOG(WS_LOG_DEBUG, "DUAR: Send the PK OK"); + WLOG(WS_LOG_DEBUG, "DUARPK: Send the PK OK"); ret = SendUserAuthPkOk(ssh, pk->publicKeyType, pk->publicKeyTypeSz, pk->publicKey, pk->publicKeySz); } @@ -1555,7 +1585,7 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, sizeCompare = encDigestSz != checkDigestSz; if (compare || sizeCompare || ret < 0) { - WLOG(WS_LOG_DEBUG, "signature compare failure"); + WLOG(WS_LOG_DEBUG, "DUARPK: signature compare failure"); SendUserAuthFailure(ssh, 0); } else { @@ -2860,7 +2890,7 @@ int SendServiceAccept(WOLFSSH* ssh) } -static const char cannedAuths[] = "publickey"; +static const char cannedAuths[] = "publickey,password"; static const uint32_t cannedAuthsSz = sizeof(cannedAuths) - 1; diff --git a/wolfssh/error.h b/wolfssh/error.h index b124c587..2c74e2a4 100644 --- a/wolfssh/error.h +++ b/wolfssh/error.h @@ -66,6 +66,7 @@ enum WS_ErrorCodes { WS_CREATE_MAC_E = -24, WS_RESOURCE_E = -25, /* insufficient resources for new channel */ WS_INVALID_CHANTYPE = -26, /* invalid channel type */ + WS_INVALID_USERNAME = -28 }; From 4dc3c56a887da2d54d24a01b581eccd4b968e2e2 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 24 Jun 2016 14:23:16 -0600 Subject: [PATCH 2/8] fixing RSA public key user auth, failover to password --- examples/echoserver/echoserver.c | 18 ++++++++++++------ src/internal.c | 13 ++++++++----- wolfssh/internal.h | 3 ++- wolfssh/ssh.h | 1 + 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 0ae71247..4648a723 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -542,16 +542,22 @@ static int wsUserAuth(uint8_t authType, map = list->head; while (map != NULL) { - if (authData->type == map->type && - authData->usernameSz == map->usernameSz && + if (authData->usernameSz == map->usernameSz && memcmp(authData->username, map->username, map->usernameSz) == 0) { - if (memcmp(map->p, authHash, SHA256_DIGEST_SIZE) != 0) { - return (authType == WOLFSSH_USERAUTH_PASSWORD ? + + if (authData->type == map->type) { + if (memcmp(map->p, authHash, SHA256_DIGEST_SIZE) == 0) { + return WOLFSSH_USERAUTH_SUCCESS; + } + else { + return (authType == WOLFSSH_USERAUTH_PASSWORD ? WOLFSSH_USERAUTH_INVALID_PASSWORD : WOLFSSH_USERAUTH_INVALID_PUBLICKEY); + } + } + else { + return WOLFSSH_USERAUTH_INVALID_AUTHTYPE; } - - return WOLFSSH_USERAUTH_SUCCESS; } map = map->next; } diff --git a/src/internal.c b/src/internal.c index d3ea9dbe..fe774029 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1491,6 +1491,7 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, WLOG(WS_LOG_DEBUG, "Entering DoUserAuthRequestPublicKey()"); + DumpOctetString(buf + begin, len - begin); authData->type = WOLFSSH_USERAUTH_PUBLICKEY; GetBoolean(&pk->hasSignature, buf, len, &begin); GetUint32(&pk->publicKeyTypeSz, buf, len, &begin); @@ -1510,6 +1511,8 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, pk->signatureSz = 0; } + *idx = begin; + if (ssh->ctx->userAuthCb != NULL) { WLOG(WS_LOG_DEBUG, "DUARPK: Calling the userauth callback"); ret = ssh->ctx->userAuthCb(WOLFSSH_USERAUTH_PUBLICKEY, @@ -1518,10 +1521,12 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, if (ret != WOLFSSH_USERAUTH_SUCCESS) { switch (ret) { case WOLFSSH_USERAUTH_INVALID_USER: - SendDisconnect(ssh, WOLFSSH_DISCONNECT_ILLEGAL_USER_NAME); - break; + return SendDisconnect(ssh, + WOLFSSH_DISCONNECT_ILLEGAL_USER_NAME); default: - SendUserAuthFailure(ssh, 0); + return SendUserAuthFailure(ssh, 0); + /* XXX Need to tell User Auth layer to disallow + * public key user auth */ } } } @@ -1594,8 +1599,6 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, } } - *idx = begin; - return ret; } diff --git a/wolfssh/internal.h b/wolfssh/internal.h index bc5db9fe..7fe0c10a 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -348,7 +348,8 @@ enum WS_MessageIds { MSGID_USERAUTH_FAILURE = 51, MSGID_USERAUTH_SUCCESS = 52, MSGID_USERAUTH_BANNER = 53, - MSGID_USERAUTH_PK_OK = 60, + MSGID_USERAUTH_PK_OK = 60, /* Public Key OK */ + MSGID_USERAUTH_PW_CHRQ = 60, /* Password Change Request */ MSGID_CHANNEL_OPEN = 90, MSGID_CHANNEL_OPEN_CONF = 91, diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 27ddcbd2..cef697cc 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -151,6 +151,7 @@ enum WS_UserAuthTypes { enum WS_UserAuthResults { WOLFSSH_USERAUTH_SUCCESS, WOLFSSH_USERAUTH_FAILURE, + WOLFSSH_USERAUTH_INVALID_AUTHTYPE, WOLFSSH_USERAUTH_INVALID_USER, WOLFSSH_USERAUTH_INVALID_PASSWORD, WOLFSSH_USERAUTH_INVALID_PUBLICKEY From cf2cb5f67b74e5b2885a64869aca83c328842857 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 13 Jul 2016 15:08:30 -0700 Subject: [PATCH 3/8] deleted extra print statements --- src/internal.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index fe774029..89f821e4 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1491,7 +1491,6 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, WLOG(WS_LOG_DEBUG, "Entering DoUserAuthRequestPublicKey()"); - DumpOctetString(buf + begin, len - begin); authData->type = WOLFSSH_USERAUTH_PUBLICKEY; GetBoolean(&pk->hasSignature, buf, len, &begin); GetUint32(&pk->publicKeyTypeSz, buf, len, &begin); From 0994a1816dbb0c41254b32e1a55955d80462e337 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 13 Jul 2016 23:53:13 -0700 Subject: [PATCH 4/8] fix a couple initialization issues found in scan-build --- src/internal.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/internal.c b/src/internal.c index 89f821e4..1f8af4ff 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1430,11 +1430,11 @@ static int DoUserAuthRequestRsa(WOLFSSH* ssh, WS_UserAuthData_PublicKey* pk, { RsaKey key; uint8_t* publicKeyType; - uint32_t publicKeyTypeSz; + uint32_t publicKeyTypeSz = 0; uint8_t* n; - uint32_t nSz; + uint32_t nSz = 0; uint8_t* e; - uint32_t eSz; + uint32_t eSz = 0; uint32_t i = 0; int ret; @@ -1610,6 +1610,8 @@ static int DoUserAuthRequest(WOLFSSH* ssh, uint8_t authNameId; WS_UserAuthData authData; + WMEMSET(&authData, 0, sizeof(authData)); + GetUint32(&authData.usernameSz, buf, len, &begin); authData.username = buf + begin; begin += authData.usernameSz; From d101f1d5376bc1b0ddc9d004c43249cf62e7c89e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 14 Jul 2016 12:58:37 -0700 Subject: [PATCH 5/8] fix a few stray memory leaks --- examples/echoserver/echoserver.c | 6 ++++++ src/ssh.c | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 4648a723..00e860f4 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -313,6 +313,8 @@ static int load_file(const char* fileName, uint8_t* buf, uint32_t bufSz) return 0; } + fclose(file); + return fileSz; } @@ -383,6 +385,7 @@ static void PwMapListDelete(PwMapList* list) PwMap* cur = head; head = head->next; memset(cur, 0, sizeof(PwMap)); + free(cur); } } } @@ -471,10 +474,12 @@ static int LoadPublicKeyBuffer(uint8_t* buf, uint32_t bufSz, PwMapList* list) str = delimiter + 1; delimiter = strchr(str, ' '); publicKey64 = (uint8_t*)str; + *delimiter = 0; publicKey64Sz = (uint32_t)(delimiter - str); str = delimiter + 1; delimiter = strchr(str, '\n'); username = (uint8_t*)str; + *delimiter = 0; usernameSz = (uint32_t)(delimiter - str); str = delimiter + 1; publicKeySz = sizeof(publicKey); @@ -658,6 +663,7 @@ int main(void) } PwMapListDelete(&pwMapList); + wolfSSH_CTX_free(ctx); if (wolfSSH_Cleanup() != WS_SUCCESS) { fprintf(stderr, "Couldn't clean up wolfSSH.\n"); exit(EXIT_FAILURE); diff --git a/src/ssh.c b/src/ssh.c index d964da06..1d96eedb 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -225,12 +225,16 @@ static void SshResourceFree(WOLFSSH* ssh, void* heap) WFREE(ssh->handshake, heap, DYNTYPE_HS); } if (ssh->rng) { - /* FreeRng(ssh->rng); */ + wc_FreeRng(ssh->rng); WFREE(ssh->rng, heap, DYNTYPE_RNG); } if (ssh->userName) { WFREE(ssh->userName, heap, DYNTYPE_STRING); } + if (ssh->channel.inputBuffer.buffer) { + WFREE(ssh->channel.inputBuffer.buffer, + ssh->channel.inputBuffer.heap, DYNTYPE_BUFFER); + } } From a2c5e8e793f52c240f1dff8d1ab3880c97003eba Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 14 Jul 2016 13:53:23 -0700 Subject: [PATCH 6/8] React to invalid username with a regular auth failure, nothing fancy. Fancy was causing a crash. --- src/internal.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/internal.c b/src/internal.c index 1f8af4ff..a470bc37 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1402,20 +1402,13 @@ static int DoUserAuthRequestPassword(WOLFSSH* ssh, WS_UserAuthData* authData, else { WLOG(WS_LOG_DEBUG, "DUARPW: password check failed"); if (ret != WOLFSSH_USERAUTH_SUCCESS) { - switch (ret) { - case WOLFSSH_USERAUTH_INVALID_USER: - SendDisconnect(ssh, - WOLFSSH_DISCONNECT_ILLEGAL_USER_NAME); - break; - default: - SendUserAuthFailure(ssh, 0); - } + return SendUserAuthFailure(ssh, 0); } } } else { WLOG(WS_LOG_DEBUG, "DUARPW: No user auth callback"); - SendUserAuthFailure(ssh, 0); + return SendUserAuthFailure(ssh, 0); } *idx = begin; @@ -1518,19 +1511,12 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, authData, ssh->userAuthCtx); WLOG(WS_LOG_DEBUG, "DUARPK: callback result = %d", ret); if (ret != WOLFSSH_USERAUTH_SUCCESS) { - switch (ret) { - case WOLFSSH_USERAUTH_INVALID_USER: - return SendDisconnect(ssh, - WOLFSSH_DISCONNECT_ILLEGAL_USER_NAME); - default: - return SendUserAuthFailure(ssh, 0); - /* XXX Need to tell User Auth layer to disallow - * public key user auth */ - } + return SendUserAuthFailure(ssh, 0); } } else { WLOG(WS_LOG_DEBUG, "DUARPK: no userauth callback set"); + return SendUserAuthFailure(ssh, 0); } if (pk->signature == NULL) { @@ -1590,7 +1576,7 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, if (compare || sizeCompare || ret < 0) { WLOG(WS_LOG_DEBUG, "DUARPK: signature compare failure"); - SendUserAuthFailure(ssh, 0); + return SendUserAuthFailure(ssh, 0); } else { ssh->clientState = CLIENT_USERAUTH_DONE; From 525a8858c50b4fae3200f87794fb3e3546164bd9 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 14 Jul 2016 14:47:30 -0700 Subject: [PATCH 7/8] Add return code when the peer window is too small on a send. Echoserver wasn't properly checking the return code on the stream receive. --- examples/echoserver/echoserver.c | 4 ++-- src/internal.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 00e860f4..d7b9d682 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -263,8 +263,8 @@ static THREAD_RETURN CYASSL_THREAD server_worker(void* vArgs) WOLFSSH* ssh = (WOLFSSH*)vArgs; SOCKET_T clientFd = wolfSSH_get_fd(ssh); - uint8_t buf[4096]; - uint32_t bufSz; + uint8_t buf[4096]; + int bufSz; if (wolfSSH_accept(ssh) == WS_SUCCESS) { diff --git a/src/internal.c b/src/internal.c index a470bc37..1c36a66d 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3102,6 +3102,7 @@ int SendChannelData(WOLFSSH* ssh, uint32_t peerChannel, if (channel->peerWindowSz < dataSz) { WLOG(WS_LOG_DEBUG, "Peer window too small"); + return WS_OVERFLOW_E; } ret = PreparePacket(ssh, MSG_ID_SZ + UINT32_SZ + LENGTH_SZ + dataSz); From 9ebcd5c44aeda2cfe26ddbee0534b65142c7d064 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 14 Jul 2016 15:42:23 -0700 Subject: [PATCH 8/8] fix trying to decrypt messages where the body fits in with the length block --- src/internal.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index 1c36a66d..cfba2ff1 100644 --- a/src/internal.c +++ b/src/internal.c @@ -2167,12 +2167,17 @@ int ProcessReply(WOLFSSH* ssh) return ret; } - ret = Decrypt(ssh, - ssh->inputBuffer.buffer + - ssh->inputBuffer.idx + peerBlockSz, - ssh->inputBuffer.buffer + - ssh->inputBuffer.idx + peerBlockSz, - ssh->curSz + LENGTH_SZ - peerBlockSz); + if (ssh->curSz + LENGTH_SZ - peerBlockSz > 0) { + ret = Decrypt(ssh, + ssh->inputBuffer.buffer + + ssh->inputBuffer.idx + peerBlockSz, + ssh->inputBuffer.buffer + + ssh->inputBuffer.idx + peerBlockSz, + ssh->curSz + LENGTH_SZ - peerBlockSz); + } + else { + WLOG(WS_LOG_INFO, "Not trying to decrypt short message."); + } /* Verify the buffer is big enough for the data and mac. * Even if the decrypt step fails, verify the MAC anyway.