wolfssh/client: reject unsanitized fields before known_hosts write

apps/wolfssh/common.c

@@ -193,22 +193,84 @@ void ClientIPOverride(int flag)
 #endif /* WOLFSSH_CERTS */
 
 
-static int AppendKeyToFile(const char* filename, const char* name,
+/* A known_hosts entry is whitespace-delimited and newline-terminated. Each
-        const char* type, const char* key)
+ * stored field must be non-empty and free of spaces and control bytes,
+ * otherwise it could inject extra fields or a forged entry into the file.
+ * Returns WS_SUCCESS when the field is safe to store, WS_BAD_ARGUMENT
+ * otherwise. */
+static int IsFieldStorable(const char* field)
 {
-    WFILE *f;
+    const char* p;
-    int ret;
+    int ret = WS_SUCCESS;
 
-    ret = WFOPEN(NULL, &f, filename, "a");
+    if (field == NULL || *field == '\0') {
-    if (ret == 0 && f != WBADFILE) {
+        ret = WS_BAD_ARGUMENT;
-        fprintf(f, "%s %s %s\n", name, type, key);
+    }
-        WFCLOSE(NULL, f);
+    else {
+        for (p = field; *p != '\0'; p++) {
+            if ((unsigned char)*p <= ' ' || (unsigned char)*p == 0x7f) {
+                ret = WS_BAD_ARGUMENT;
+                break;
+            }
+        }
     }
 
     return ret;
 }
 
 
+static int AppendKeyToFile(const char* filename, const char* name,
+        const char* type, const char* key)
+{
+    WFILE *f = WBADFILE;
+    int ret;
+
+    /* The host name comes from the command line and the key type comes from
+     * the peer's host-key blob; both are untrusted and must not carry field or
+     * line separators. The key is Base64 and cannot contain a separator, so
+     * the same check on it only guards against a NULL or empty value reaching
+     * fprintf. */
+    ret = IsFieldStorable(name);
+    if (ret == WS_SUCCESS) {
+        ret = IsFieldStorable(type);
+    }
+    if (ret == WS_SUCCESS) {
+        ret = IsFieldStorable(key);
+    }
+    if (ret == WS_SUCCESS) {
+        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) {
+                ret = WS_BAD_FILE_E;
+            }
+            if (WFCLOSE(NULL, f) != 0 && ret == WS_SUCCESS) {
+                ret = WS_BAD_FILE_E;
+            }
+        }
+        else if (ret == 0) {
+            /* WFOPEN reported success but produced no usable handle; surface
+             * an error instead of silently returning success. */
+            ret = WS_BAD_FILE_E;
+        }
+    }
+
+    return ret;
+}
+
+
+#ifdef WOLFSSH_TEST_INTERNAL
+int wolfSSH_TestAppendKeyToFile(const char* filename, const char* name,
+        const char* type, const char* key)
+{
+    return AppendKeyToFile(filename, name, type, key);
+}
+#endif
+
+
 static int FingerprintKey(const byte* pubKey, word32 pubKeySz, char* out)
 {
     wc_Sha256 sha;

apps/wolfssh/common.h

@@ -34,5 +34,9 @@ WOLFSSH_LOCAL void ClientIPOverride(int flag);
 WOLFSSH_LOCAL void ClientFreeBuffers(void);
 WOLFSSH_LOCAL int ClientParseDestination(const char* in, char** user,
         char** hostname, word16* port);
+#ifdef WOLFSSH_TEST_INTERNAL
+WOLFSSH_LOCAL int wolfSSH_TestAppendKeyToFile(const char* filename,
+        const char* name, const char* type, const char* key);
+#endif
 
 #endif /* APPS_WOLFSSH_COMMON_H */

tests/regress.c

@@ -342,6 +342,43 @@ static void FreeChannelOpenHarness(ChannelOpenHarness* harness)
     #endif
 #endif
 
+/* Read a whole file into buf, returning the byte count (0 on any failure).
+ * Used by the DH KEX regression below and by TestAppendKeyToFile, so it is
+ * available whenever either of those is compiled. */
+#if defined(KEXDH_REPLY_REGRESS_KEX_ALGO) || defined(WOLFSSH_TEST_INTERNAL)
+static word32 LoadFileBuffer(const char* path, byte* buf, word32 bufSz)
+{
+    WFILE* file;
+    long fileSz;
+    word32 readSz;
+
+    if (path == NULL || buf == NULL || bufSz == 0) {
+        return 0;
+    }
+
+    if (WFOPEN(NULL, &file, path, "rb") != 0 || file == WBADFILE) {
+        return 0;
+    }
+    WFSEEK(NULL, file, 0, WSEEK_END);
+    fileSz = WFTELL(NULL, file);
+    WREWIND(NULL, file);
+
+    if (fileSz <= 0 || (word32)fileSz > bufSz) {
+        WFCLOSE(NULL, file);
+        return 0;
+    }
+
+    readSz = (word32)WFREAD(NULL, buf, 1, fileSz, file);
+    WFCLOSE(NULL, file);
+
+    if (readSz != (word32)fileSz) {
+        return 0;
+    }
+
+    return readSz;
+}
+#endif /* KEXDH_REPLY_REGRESS_KEX_ALGO || WOLFSSH_TEST_INTERNAL */
+
 #ifdef KEXDH_REPLY_REGRESS_KEX_ALGO
 
 #define REGRESS_DUPLEX_QUEUE_SZ 32768U
@@ -430,38 +467,6 @@ static word32 AppendBlob(byte* buf, word32 bufSz, word32 idx,
     return AppendData(buf, bufSz, idx, data, dataSz);
 }
 
-static word32 LoadFileBuffer(const char* path, byte* buf, word32 bufSz)
-{
-    WFILE* file;
-    long fileSz;
-    word32 readSz;
-
-    if (path == NULL || buf == NULL || bufSz == 0) {
-        return 0;
-    }
-
-    if (WFOPEN(NULL, &file, path, "rb") != 0 || file == WBADFILE) {
-        return 0;
-    }
-    WFSEEK(NULL, file, 0, WSEEK_END);
-    fileSz = WFTELL(NULL, file);
-    WREWIND(NULL, file);
-
-    if (fileSz <= 0 || (word32)fileSz > bufSz) {
-        WFCLOSE(NULL, file);
-        return 0;
-    }
-
-    readSz = (word32)WFREAD(NULL, buf, 1, fileSz, file);
-    WFCLOSE(NULL, file);
-
-    if (readSz != (word32)fileSz) {
-        return 0;
-    }
-
-    return readSz;
-}
-
 static int RegressionClientUserAuth(byte authType,
         WS_UserAuthData* authData, void* ctx)
 {
@@ -4128,6 +4133,109 @@ static void TestClientParseDestination(void)
 }
 
 
+#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
+ * or a forged entry into the known_hosts file. Exercised through the
+ * wolfSSH_TestAppendKeyToFile hook. */
+static void TestAppendKeyToFile(void)
+{
+    const char* path = "regress_known_hosts.tmp";
+    char buf[128];
+    word32 readSz;
+
+    /* A clean name and type write one well-formed, newline-terminated entry. */
+    (void)remove(path);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host.example.com",
+                "ssh-rsa", "AAAA"), WS_SUCCESS);
+    WMEMSET(buf, 0, sizeof(buf));
+    readSz = LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1);
+    AssertTrue(readSz > 0);
+    AssertIntEQ(WSTRCMP(buf, "host.example.com ssh-rsa AAAA\n"), 0);
+
+    /* A second call appends rather than truncating, preserving the first
+     * entry (the file is opened in append mode). */
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host2.example.com",
+                "ssh-ed25519", "BBBB"), WS_SUCCESS);
+    WMEMSET(buf, 0, sizeof(buf));
+    readSz = LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1);
+    AssertTrue(readSz > 0);
+    AssertIntEQ(WSTRCMP(buf,
+                "host.example.com ssh-rsa AAAA\n"
+                "host2.example.com ssh-ed25519 BBBB\n"), 0);
+
+    /* A newline in the name would forge an extra entry; it is rejected and
+     * nothing is written to the file. */
+    (void)remove(path);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path,
+                "127.0.0.1\nevil.example.com ssh-rsa BBBB", "ssh-rsa", "CCCC"),
+            WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+
+    /* A space in the name would forge extra fields; it is rejected. */
+    (void)remove(path);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "real.example.com other",
+                "ssh-rsa", "CCCC"), WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+
+    /* The key type is peer-supplied and is checked the same way: a newline in
+     * it is rejected and nothing is written. */
+    (void)remove(path);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host.example.com",
+                "ssh-rsa\nevil.example.com ssh-rsa DDDD", "EEEE"),
+            WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+
+    /* Tab, carriage return, and DEL (0x7f) are rejected too, and each leaves
+     * the file unwritten. */
+    (void)remove(path);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host\tname", "ssh-rsa",
+                "CCCC"), WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host\rname", "ssh-rsa",
+                "CCCC"), WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host\x7fname", "ssh-rsa",
+                "CCCC"), WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+
+    /* A NULL or empty name, type, or key is rejected and nothing is written.
+     * The key check guards fprintf against a NULL or empty value. */
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, NULL, "ssh-rsa", "CCCC"),
+            WS_BAD_ARGUMENT);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "", "ssh-rsa", "CCCC"),
+            WS_BAD_ARGUMENT);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host.example.com", "",
+                "CCCC"), WS_BAD_ARGUMENT);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host.example.com", "ssh-rsa",
+                NULL), WS_BAD_ARGUMENT);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "host.example.com", "ssh-rsa",
+                ""), WS_BAD_ARGUMENT);
+    AssertIntEQ(LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1), 0);
+
+    /* High-bit (non-ASCII) bytes are only above the control range, so an
+     * internationalized host name is stored intact rather than rejected. This
+     * pins the unsigned-char handling: a signed-char compare would wrongly
+     * reject 0x80-0xff. */
+    (void)remove(path);
+    AssertIntEQ(wolfSSH_TestAppendKeyToFile(path, "h\xC3\xA9st", "ssh-rsa",
+                "AAAA"), WS_SUCCESS);
+    WMEMSET(buf, 0, sizeof(buf));
+    readSz = LoadFileBuffer(path, (byte*)buf, sizeof(buf) - 1);
+    AssertTrue(readSz > 0);
+    AssertIntEQ(WSTRCMP(buf, "h\xC3\xA9st ssh-rsa AAAA\n"), 0);
+
+    /* When the file cannot be opened (here, a path under a directory that does
+     * not exist), the function reports the failure rather than claiming
+     * success. */
+    AssertTrue(wolfSSH_TestAppendKeyToFile("regress_no_such_dir/known_hosts",
+                "host.example.com", "ssh-rsa", "AAAA") != WS_SUCCESS);
+
+    (void)remove(path);
+}
+#endif /* WOLFSSH_TEST_INTERNAL */
+
+
 int main(int argc, char** argv)
 {
     WOLFSSH_CTX* ctx;
@@ -4145,6 +4253,9 @@ int main(int argc, char** argv)
     AssertNotNull(ssh);
 
     TestClientParseDestination();
+#ifdef WOLFSSH_TEST_INTERNAL
+    TestAppendKeyToFile();
+#endif
     TestAuthMessageBlockedDuringKeying(ssh);
     TestUserauthFailureDuringKeying(ssh);
     TestPasswordLeakAborts(ssh);