diff --git a/.gitignore b/.gitignore index a10d1f1aa..097190f02 100644 --- a/.gitignore +++ b/.gitignore @@ -86,6 +86,9 @@ test.dat # Scratch file the api tests write in the working directory. Removed on a # clean run, left behind when one aborts. ossh-cert-line.tmp +# Same for the regression tests' known_hosts fixtures, named by pid. +wolfssh_kh_*.tmp +regress_known_hosts*.tmp # test output tests/*.test @@ -96,6 +99,7 @@ client.plist # misc .DS_Store +compile_commands.json # Visual Studio Code Workspace Files *.vscode diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index d98990eac..fd5c1d57d 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -211,6 +211,31 @@ static int IsFieldStorable(const char* field) } +/* A known_hosts entry has to start on its own line. + * Returns 1 when a separating newline has to go out ahead of the entry. */ +static int AppendNeedsNewline(const char* filename) +{ + WFILE *f = WBADFILE; + char last = '\n'; + int needs = 0; + + if (WFOPEN(NULL, &f, filename, "rb") != 0 || f == WBADFILE) { + /* No file to run into; the append creates it. */ + return 0; + } + + /* The seek fails on an empty file, which needs no separator either. */ + if (WFSEEK_SUCCESS(WFSEEK(NULL, f, -1, WSEEK_END)) + && WFREAD(NULL, &last, 1, 1, f) == 1) { + needs = (last != '\n'); + } + + WFCLOSE(NULL, f); + + return needs; +} + + static int AppendKeyToFile(const char* filename, const char* name, const char* type, const char* key) { @@ -230,13 +255,16 @@ static int AppendKeyToFile(const char* filename, const char* name, ret = IsFieldStorable(key); } if (ret == WS_SUCCESS) { + const int needsNewline = AppendNeedsNewline(filename); + ret = WFOPEN(NULL, &f, filename, "a"); if (ret == 0 && f != WBADFILE) { /* Check the write and the close so a failed or truncated entry * (for example on a full disk) is reported rather than appearing * to pin the key. The close flushes buffered output, so a write * error can surface there. */ - if (fprintf(f, "%s %s %s\n", name, type, key) < 0) { + if (fprintf(f, "%s%s %s %s\n", needsNewline ? "\n" : "", + name, type, key) < 0) { ret = WS_BAD_FILE_E; } if (WFCLOSE(NULL, f) != 0 && ret == WS_SUCCESS) { @@ -417,6 +445,14 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) lineCount++; line = WSTRSEP(&cursor, "\n"); if (line != NULL && *line) { + /* Non-empty was checked above, so the last byte is a real one. */ + word32 lineSz = (word32)WSTRLEN(line); + + /* Remove trailing CR if present for comparison below */ + if (line[lineSz - 1] == '\r') { + line[lineSz - 1] = 0; + } + name = WSTRSEP(&line, " "); keyType = WSTRSEP(&line, " "); key = WSTRSEP(&line, " "); diff --git a/tests/regress.c b/tests/regress.c index ad9c6a5df..a5e7cf39c 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -36,7 +36,9 @@ #include #include #include +#include +#include #include #include #include @@ -5712,6 +5714,27 @@ static void TestClientParseDestination(void) } +#if defined(WOLFSSH_TEST_INTERNAL) || defined(WOLFSSL_BASE64_ENCODE) +/* Write contents to path exactly as given, with no terminator added, so a + * test can seed a file whose last line ends without a newline. */ +static void WriteTextFile(const char* path, const char* contents) +{ + WFILE* f = WBADFILE; + word32 sz = (word32)WSTRLEN(contents); + + AssertIntEQ(WFOPEN(NULL, &f, path, "wb"), 0); + AssertTrue(f != WBADFILE); + /* With WOLFSSH_NO_ABORT the asserts above do not stop the run, so return + * rather than write through a handle the open never produced. */ + if (f == WBADFILE) { + return; + } + AssertIntEQ((word32)WFWRITE(NULL, contents, 1, sz, f), sz); + AssertIntEQ(WFCLOSE(NULL, f), 0); +} +#endif + + #ifdef WOLFSSH_TEST_INTERNAL /* AppendKeyToFile must refuse a host name or key type that carries whitespace * or control bytes, so an attacker-controlled value cannot inject extra fields @@ -5812,9 +5835,249 @@ static void TestAppendKeyToFile(void) (void)remove(path); } + + +/* POSIX lets the last line of a text file end without a newline. An appended + * entry has to start on its own line, otherwise it runs onto the last stored + * entry and both are corrupted: the old host ends up pinned to a key it never + * had, and the new host is never stored at all. */ +static void TestAppendNoTrailingNewline(void) +{ + const char* path = "regress_known_hosts_nl.tmp"; + static const struct { + const char* seed; + const char* expected; + } cases[] = { + /* Unterminated last line: the entry gets a separator of its own. */ + { "a.example.com ssh-rsa AAAA", + "a.example.com ssh-rsa AAAA\nb.example.com ssh-rsa BBBB\n" }, + /* Already terminated: no separator, so no blank line. */ + { "a.example.com ssh-rsa AAAA\n", + "a.example.com ssh-rsa AAAA\nb.example.com ssh-rsa BBBB\n" }, + /* CRLF ends in a newline too, and the seed is kept byte for byte. */ + { "a.example.com ssh-rsa AAAA\r\n", + "a.example.com ssh-rsa AAAA\r\nb.example.com ssh-rsa BBBB\n" }, + /* An empty file has no last line to run onto. */ + { "", "b.example.com ssh-rsa BBBB\n" } + }; + char buf[128]; + word32 readSz; + unsigned int i; + + for (i = 0; i < sizeof(cases)/sizeof(cases[0]); i++) { + (void)remove(path); + WriteTextFile(path, cases[i].seed); + AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "b.example.com", + "ssh-rsa", "BBBB"), WS_SUCCESS); + WMEMSET(buf, 0, sizeof(buf)); + readSz = LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1); + AssertTrue(readSz > 0); + AssertStrEQ(buf, cases[i].expected); + } + + /* An absent file is created, and needs no separator either. */ + (void)remove(path); + AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "b.example.com", + "ssh-rsa", "BBBB"), WS_SUCCESS); + WMEMSET(buf, 0, sizeof(buf)); + readSz = LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1); + AssertTrue(readSz > 0); + AssertStrEQ(buf, "b.example.com ssh-rsa BBBB\n"); + + (void)remove(path); +} #endif /* WOLFSSH_TEST_INTERNAL */ +#ifdef WOLFSSL_BASE64_ENCODE + +/* Every known_hosts rejection returns -1: a known host with the wrong key, and + * an unrecognized host whose "add it?" prompt reads EOF. Only the message + * tells them apart, so run the check with stdout captured and let the caller + * assert on what was printed. Returns the check's own return value. */ +static int KnownHostsCheckCapture(const byte* pubKey, word32 pubKeySz, + char* targetName, char* out, word32 outSz) +{ + char capPath[64]; + int savedStdout, capFd, ret; + long readSz = 0; + WFILE* f = WBADFILE; + + WSNPRINTF(capPath, sizeof(capPath), "wolfssh_kh_out_%d.tmp", (int)getpid()); + out[0] = 0; + + capFd = open(capPath, O_RDWR | O_CREAT | O_TRUNC, 0600); + AssertTrue(capFd >= 0); + savedStdout = dup(STDOUT_FILENO); + AssertTrue(savedStdout >= 0); + fflush(stdout); + AssertTrue(dup2(capFd, STDOUT_FILENO) >= 0); + + ret = ClientPublicKeyCheck(pubKey, pubKeySz, targetName); + + /* stdout is a file here, so it is fully buffered; flush before restoring */ + fflush(stdout); + AssertTrue(dup2(savedStdout, STDOUT_FILENO) >= 0); + close(savedStdout); + close(capFd); + + if (WFOPEN(NULL, &f, capPath, "rb") == 0 && f != WBADFILE) { + readSz = (long)WFREAD(NULL, out, 1, outSz - 1, f); + WFCLOSE(NULL, f); + } + if (readSz < 0) { + readSz = 0; + } + out[readSz] = 0; + (void)remove(capPath); + + return ret; +} + + +/* known_hosts is a text file and POSIX lets its last line end without a + * newline, and a file written on Windows ends its lines with CRLF. Match the + * last entry with a trailing newline, without one, and with CRLF line + * endings, then check that a wrong key on that same last entry is still + * rejected. */ +static void TestKnownHostsLastEntry(void) +{ + /* string("ssh-rsa"), then a zero certificate count so the RFC 6187 parse + * declines this blob, then filler. Only the name and the base64 of the + * whole blob matter to the known_hosts search. */ + static const byte pubKey[] = { + 0x00, 0x00, 0x00, 0x07, 's', 's', 'h', '-', 'r', 's', 'a', + 0x00, 0x00, 0x00, 0x00, 0x01, 0x02, 0x03, 0x04 + }; + static const struct { + const char* sep; + const char* tail; + const char* label; + } cases[] = { + { "\n", "\n", "trailing newline" }, + { "\n", "", "no trailing newline" }, + { "\r\n", "\r\n", "CRLF endings" }, + }; + char targetName[] = "last.example.com"; + char homeDir[64]; + char sshDir[80]; + char hostsPath[112]; + char encoded[64]; + char wrongKey[64]; + char contents[256]; + char captured[512]; + char* savedHome = NULL; + const char* home; + word32 encodedSz = (word32)sizeof(encoded); + int savedStdin, devNull, ready; + unsigned int i; + + WSNPRINTF(homeDir, sizeof(homeDir), "wolfssh_kh_%d.tmp", (int)getpid()); + WSNPRINTF(sshDir, sizeof(sshDir), "%s/.ssh", homeDir); + WSNPRINTF(hostsPath, sizeof(hostsPath), "%s/known_hosts", sshDir); + + AssertIntEQ(Base64_Encode_NoNl(pubKey, (word32)sizeof(pubKey), + (byte*)encoded, &encodedSz), 0); + AssertTrue(encodedSz < sizeof(encoded)); + encoded[encodedSz] = 0; + + /* Same length and alphabet, different key, for the rejection case. */ + WMEMCPY(wrongKey, encoded, encodedSz + 1); + wrongKey[0] = (encoded[0] == 'A') ? 'B' : 'A'; + + home = getenv("HOME"); + if (home != NULL) { + savedHome = (char*)WMALLOC(WSTRLEN(home) + 1, NULL, 0); + AssertNotNull(savedHome); + WSTRCPY(savedHome, home); + } + + /* The name only varies by pid, so an aborted run can leave the tree + * behind and make the mkdir below fail. Clear it first. */ + (void)remove(hostsPath); + (void)rmdir(sshDir); + (void)rmdir(homeDir); + + /* Use a single flag to avoid duplicate errors below. */ + ready = (mkdir(homeDir, 0700) == 0) + && (mkdir(sshDir, 0700) == 0) + && (setenv("HOME", homeDir, 1) == 0); + AssertTrue(ready); + + /* A regression falls through to the "add it to known hosts?" prompt, so + * point stdin at EOF: the test then fails rather than waiting forever. + * Check each step, otherwise a failure here leaves the prompt reading + * the real stdin. */ + savedStdin = dup(STDIN_FILENO); + devNull = open("/dev/null", O_RDONLY); + ready = ready && (savedStdin >= 0) && (devNull >= 0) + && (dup2(devNull, STDIN_FILENO) >= 0); + AssertTrue(ready); + + for (i = 0; ready && i < sizeof(cases)/sizeof(cases[0]); i++) { + printf(" known_hosts with %s.\n", cases[i].label); + + /* An entry for a different host goes first, so the match lands on the + * last line, the one the terminator used to overwrite. */ + WSNPRINTF(contents, sizeof(contents), + "other.example.com ssh-rsa AAAA%s%s ssh-rsa %s%s", + cases[i].sep, targetName, encoded, cases[i].tail); + WriteTextFile(hostsPath, contents); + AssertIntEQ(ClientPublicKeyCheck(pubKey, (word32)sizeof(pubKey), + targetName), 0); + + /* The same host listed with a different key is a known host with an + * unknown key, which must be rejected rather than prompted for. A + * regression that never parses the last entry also returns non-zero, + * by prompting and reading EOF, so require the message that only the + * known-host-wrong-key path prints and reject the prompt text. */ + WSNPRINTF(contents, sizeof(contents), + "other.example.com ssh-rsa AAAA%s%s ssh-rsa %s%s", + cases[i].sep, targetName, wrongKey, cases[i].tail); + WriteTextFile(hostsPath, contents); + AssertTrue(KnownHostsCheckCapture(pubKey, (word32)sizeof(pubKey), + targetName, captured, (word32)sizeof(captured)) != 0); + AssertNotNull(WSTRSTR(captured, + "That server is known, but that key is not.")); + AssertNull(WSTRSTR(captured, "Shall I add it to the known hosts?")); + + /* The CR strip makes a non-matching host's key compare equal under + * CRLF, which is the only way the "matches other servers" branch is + * reached with those endings. Same key on both lines: the first + * reports the other server, the last one still matches the target. */ + WSNPRINTF(contents, sizeof(contents), + "other.example.com ssh-rsa %s%s%s ssh-rsa %s%s", + encoded, cases[i].sep, targetName, encoded, cases[i].tail); + WriteTextFile(hostsPath, contents); + AssertIntEQ(KnownHostsCheckCapture(pubKey, (word32)sizeof(pubKey), + targetName, captured, (word32)sizeof(captured)), 0); + AssertNotNull(WSTRSTR(captured, "This key matches other servers:")); + AssertNotNull(WSTRSTR(captured, "other.example.com")); + } + + if (savedStdin >= 0) { + AssertTrue(dup2(savedStdin, STDIN_FILENO) >= 0); + close(savedStdin); + } + if (devNull >= 0) { + close(devNull); + } + + if (savedHome != NULL) { + AssertIntEQ(setenv("HOME", savedHome, 1), 0); + WFREE(savedHome, NULL, 0); + } + else { + unsetenv("HOME"); + } + + (void)remove(hostsPath); + (void)rmdir(sshDir); + (void)rmdir(homeDir); +} +#endif /* WOLFSSL_BASE64_ENCODE */ + + int main(int argc, char** argv) { WOLFSSH_CTX* ctx; @@ -5846,6 +6109,10 @@ int main(int argc, char** argv) TestClientParseDestination(); #ifdef WOLFSSH_TEST_INTERNAL TestAppendKeyToFile(); + TestAppendNoTrailingNewline(); +#endif +#ifdef WOLFSSL_BASE64_ENCODE + TestKnownHostsLastEntry(); #endif TestAuthMessageBlockedDuringKeying(ssh); TestUserauthFailureDuringKeying(ssh);