F-8824: enable config files without trailing newline

.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

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, " ");

tests/regress.c

@@ -36,7 +36,9 @@
 #include <string.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <sys/stat.h>
 
+#include <wolfssl/wolfcrypt/coding.h>
 #include <wolfssh/port.h>
 #include <wolfssh/ssh.h>
 #include <wolfssh/internal.h>
@@ -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);