diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 790f7ca3e..f6bcf785d 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -798,3 +798,146 @@ void ClientFreeBuffers(void) userPrivateKeySz = 0; wc_ForceZero(userPassword, sizeof(userPassword)); } + + +/* Parse an SSH destination into its parts. Two forms are accepted: + * - [user@]hostname + * - ssh://[user@]hostname[:port] + * The "ssh://" prefix is only recognized at the start of the string. Parsing + * builds into local buffers and only commits to the outputs once the whole + * destination has parsed successfully, so on failure the caller's + * user/hostname/port are left untouched (no partial output, no leak). When a + * user is present, any existing *user is freed and replaced; likewise for + * *hostname. *hostname is set only when host text is present, so a malformed + * "ssh://" or "ssh://user@" leaves it untouched and the caller's NULL check can + * reject it cleanly. *port is overwritten only when an explicit port is given + * (URI form); a port that is non-numeric or outside 1..65535 is rejected with + * WS_BAD_ARGUMENT rather than silently truncated. Returns WS_SUCCESS on + * success, or a negative WS error code. */ +int ClientParseDestination(const char* in, char** user, char** hostname, + word16* port) +{ + int ret = WS_SUCCESS; + const char* uriPrefix = "ssh://"; + char* dest = NULL; + char* cursor = NULL; + char* found = NULL; + char* newUser = NULL; + char* newHostname = NULL; + char* endptr = NULL; + long portVal = 0; + word16 newPort = 0; + size_t sz = 0; + int checkPort = 0; + + if (in == NULL || user == NULL || hostname == NULL || port == NULL) { + ret = WS_BAD_ARGUMENT; + } + + if (ret == WS_SUCCESS) { + newPort = *port; /* keep the caller's default unless overridden */ + sz = WSTRLEN(in) + 1; + dest = (char*)WMALLOC(sz, NULL, 0); + if (dest == NULL) { + ret = WS_MEMORY_E; + } + } + + if (ret == WS_SUCCESS) { + WMEMCPY(dest, in, sz); + cursor = dest; + + if (WSTRNCMP(cursor, uriPrefix, WSTRLEN(uriPrefix)) == 0) { + checkPort = 1; + cursor += WSTRLEN(uriPrefix); + } + + found = WSTRCHR(cursor, '@'); + if (found == cursor) { + fprintf(stderr, "note: empty user name before '@'\n"); + } + if (found != NULL) { + *found = '\0'; + sz = WSTRLEN(cursor) + 1; + newUser = (char*)WMALLOC(sz, NULL, 0); + if (newUser == NULL) { + ret = WS_MEMORY_E; + } + else { + WMEMCPY(newUser, cursor, sz); + cursor = found + 1; + } + } + } + + if (ret == WS_SUCCESS) { + if (checkPort) { + found = WSTRCHR(cursor, ':'); + if (found != NULL) { + *found = '\0'; + } + } + else { + found = NULL; + } + + if (*cursor != 0) { + sz = WSTRLEN(cursor) + 1; + newHostname = (char*)WMALLOC(sz, NULL, 0); + if (newHostname == NULL) { + ret = WS_MEMORY_E; + } + else { + WMEMCPY(newHostname, cursor, sz); + } + } + } + + if (ret == WS_SUCCESS && found != NULL) { + cursor = found + 1; + if (*cursor != 0) { + portVal = strtol(cursor, &endptr, 10); + if (endptr == cursor || *endptr != '\0' + || portVal < 1 || portVal > 65535) { + fprintf(stderr, "invalid port \"%s\"\n", cursor); + ret = WS_BAD_ARGUMENT; + } + else { + newPort = (word16)portVal; + } + } + } + + if (ret == WS_SUCCESS) { + /* Commit: replace the caller's values only now that parsing has fully + * succeeded, freeing any prior allocations. */ + if (newUser != NULL) { + if (*user != NULL) { + WFREE(*user, NULL, 0); + } + *user = newUser; + newUser = NULL; + } + if (newHostname != NULL) { + if (*hostname != NULL) { + WFREE(*hostname, NULL, 0); + } + *hostname = newHostname; + newHostname = NULL; + } + *port = newPort; + } + + /* Free the working buffer and any allocations not committed above. */ + if (dest != NULL) { + WFREE(dest, NULL, 0); + } + if (newUser != NULL) { + WFREE(newUser, NULL, 0); + } + if (newHostname != NULL) { + WFREE(newHostname, NULL, 0); + } + + return ret; +} diff --git a/apps/wolfssh/common.h b/apps/wolfssh/common.h index c5d30b57a..77afeb4ed 100644 --- a/apps/wolfssh/common.h +++ b/apps/wolfssh/common.h @@ -32,5 +32,7 @@ WOLFSSH_LOCAL int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx); WOLFSSH_LOCAL void ClientIPOverride(int flag); WOLFSSH_LOCAL void ClientFreeBuffers(void); +WOLFSSH_LOCAL int ClientParseDestination(const char* in, char** user, + char** hostname, word16* port); #endif /* APPS_WOLFSSH_COMMON_H */ diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index d6384815f..f9414d795 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -829,64 +829,17 @@ static int config_parse_command_line(struct config* config, * - [user@]hostname * - ssh://[user@]hostname[:port] */ if (myoptind < argc) { - const char* uriPrefix = "ssh://"; - char* dest; - char* cursor; - char* found; - size_t sz; - int checkPort; + int ret; myoptarg = argv[myoptind]; - sz = WSTRLEN(myoptarg) + 1; - dest = (char*)WMALLOC(sz, NULL, 0); - WMEMCPY(dest, myoptarg, sz); - cursor = dest; - - if (WSTRSTR(cursor, uriPrefix)) { - checkPort = 1; - cursor += WSTRLEN(uriPrefix); - } - else { - checkPort = 0; - } - - found = WSTRCHR(cursor, '@'); - if (found == cursor) { - fprintf(stderr, "can't start destination with just an @\n"); - } - if (found != NULL) { - *found = '\0'; - if (config->user) { - WFREE(config->user, NULL, 0); - config->user = NULL; - } - sz = WSTRLEN(cursor); - config->user = (char*)WMALLOC(sz + 1, NULL, 0); - strcpy(config->user, cursor); - cursor = found + 1; - } - - if (checkPort) { - found = WSTRCHR(cursor, ':'); - if (found != NULL) { - *found = '\0'; - sz = WSTRLEN(cursor); - config->hostname = (char*)WMALLOC(sz + 1, NULL, 0); - strcpy(config->hostname, cursor); - cursor = found + 1; - if (*cursor != 0) { - config->port = atoi(cursor); - } - } - } - else { - sz = WSTRLEN(cursor); - config->hostname = (char*)WMALLOC(sz + 1, NULL, 0); - strcpy(config->hostname, cursor); + ret = ClientParseDestination(myoptarg, &config->user, + &config->hostname, &config->port); + if (ret != WS_SUCCESS) { + fprintf(stderr, "Couldn't parse the destination.\n"); + exit(EXIT_FAILURE); } - WFREE(dest, NULL, 0); myoptind++; } diff --git a/tests/regress.c b/tests/regress.c index 3190b03ca..e2e9d5d94 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3328,6 +3328,203 @@ static void TestDoKexInitRejectsWhenPeerIsKeying(void) #endif /* first_packet_follows coverage guard */ +/* Regression coverage for issue 5575: the documented ssh://hostname form must + * set the hostname even without an explicit port, and a malformed destination + * with no host text must leave the hostname unset so the client can reject it. + */ +static void TestClientParseDestination(void) +{ + char* user; + char* hostname; + word16 port; + + /* ssh:// without an explicit port: hostname set, default port kept. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + AssertIntEQ(port, 22); + AssertTrue(user == NULL); + WFREE(hostname, NULL, 0); + + /* ssh://user@host without a port: user and hostname set, default port. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://tester@127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "tester"), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + AssertIntEQ(port, 22); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* ssh://host:port: explicit port parsed. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:2222", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + AssertIntEQ(port, 2222); + AssertTrue(user == NULL); + WFREE(hostname, NULL, 0); + + /* ssh://user@host:port: all parts parsed. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://tester@127.0.0.1:2222", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "tester"), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + AssertIntEQ(port, 2222); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* Plain (non-URI) hostname. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + AssertIntEQ(port, 22); + AssertTrue(user == NULL); + WFREE(hostname, NULL, 0); + + /* Plain (non-URI) user@hostname. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("tester@127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "tester"), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + AssertIntEQ(port, 22); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* Malformed URI with no host text: hostname stays unset. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://", + &user, &hostname, &port), WS_SUCCESS); + AssertTrue(hostname == NULL); + AssertTrue(user == NULL); + + /* Malformed URI with a user but no host text: user set, hostname unset. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://tester@", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "tester"), 0); + AssertTrue(hostname == NULL); + WFREE(user, NULL, 0); + + /* A pre-seeded user (as config_init_default does from $USER) is freed and + * replaced when the destination carries its own user. */ + hostname = NULL; port = 22; + user = (char*)WMALLOC(WSTRLEN("seeded") + 1, NULL, 0); + AssertNotNull(user); + WMEMCPY(user, "seeded", WSTRLEN("seeded") + 1); + AssertIntEQ(ClientParseDestination("tester@127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "tester"), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* A leading '@' (no user text) is accepted with an empty user string. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://@127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, ""), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* Non-URI "user@" with no host text: user set, hostname stays unset. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("tester@", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "tester"), 0); + AssertTrue(hostname == NULL); + WFREE(user, NULL, 0); + + /* Non-URI leading '@': empty user, hostname set (no ssh:// prefix). */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("@127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, ""), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "127.0.0.1"), 0); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* An out-of-range port is rejected (not silently truncated) and the + * caller's port and outputs are left untouched. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:70000", + &user, &hostname, &port), WS_BAD_ARGUMENT); + AssertIntEQ(port, 22); + AssertTrue(user == NULL); + AssertTrue(hostname == NULL); + + /* Non-numeric, trailing-garbage, and zero ports are rejected too. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:abc", + &user, &hostname, &port), WS_BAD_ARGUMENT); + AssertIntEQ(port, 22); + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:22x", + &user, &hostname, &port), WS_BAD_ARGUMENT); + AssertIntEQ(port, 22); + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:0", + &user, &hostname, &port), WS_BAD_ARGUMENT); + AssertIntEQ(port, 22); + + /* A valid in-range port is still accepted. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:65535", + &user, &hostname, &port), WS_SUCCESS); + AssertIntEQ(port, 65535); + WFREE(hostname, NULL, 0); + + /* "ssh://" is only a prefix when it starts the string; a later occurrence + * is treated as ordinary host text, not a URI. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("user@ssh://127.0.0.1", + &user, &hostname, &port), WS_SUCCESS); + AssertNotNull(user); + AssertIntEQ(WSTRCMP(user, "user"), 0); + AssertNotNull(hostname); + AssertIntEQ(WSTRCMP(hostname, "ssh://127.0.0.1"), 0); + AssertIntEQ(port, 22); + WFREE(user, NULL, 0); + WFREE(hostname, NULL, 0); + + /* Each NULL output pointer (and a NULL input) is rejected. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination(NULL, &user, &hostname, &port), + WS_BAD_ARGUMENT); + AssertIntEQ(ClientParseDestination("127.0.0.1", NULL, &hostname, &port), + WS_BAD_ARGUMENT); + AssertIntEQ(ClientParseDestination("127.0.0.1", &user, NULL, &port), + WS_BAD_ARGUMENT); + AssertIntEQ(ClientParseDestination("127.0.0.1", &user, &hostname, NULL), + WS_BAD_ARGUMENT); + /* No output should have been touched by the rejected calls. */ + AssertTrue(user == NULL); + AssertTrue(hostname == NULL); +} + + int main(int argc, char** argv) { WOLFSSH_CTX* ctx; @@ -3344,6 +3541,7 @@ int main(int argc, char** argv) ssh = wolfSSH_new(ctx); AssertNotNull(ssh); + TestClientParseDestination(); TestAuthMessageBlockedDuringKeying(ssh); TestUserauthFailureDuringKeying(ssh); TestPasswordLeakAborts(ssh);