wallet, seal: create private files 0600 instead of at the umask

include/dogecoin/utils.h

@@ -83,6 +83,13 @@ LIBDOGECOIN_API void print_bits(size_t const size, void const* ptr);
 LIBDOGECOIN_API void prepend(char* s, const char* t);
 LIBDOGECOIN_API void append(char* s, char* t);
 LIBDOGECOIN_API char* concat(char* prefix, char* suffix);
+/* Open a file that should not be readable by other local users, creating it
+   0600 where the platform has POSIX permissions. Use for anything holding key
+   material, encrypted or not, and for the wallet database -- it carries the
+   master public key, which is enough to derive every address and reconstruct
+   the transaction history. An existing file keeps its current mode. */
+LIBDOGECOIN_API FILE* dogecoin_fopen_private(const char* path, const char* mode);
+
 LIBDOGECOIN_API void slice(const char *str, char *result, size_t start, size_t end);
 LIBDOGECOIN_API void replace_last_after_delim(const char *str, char* delim, char* replacement);
 LIBDOGECOIN_API void text_to_hex(char* in, char* out);

src/seal.c

@@ -1244,19 +1244,23 @@ LIBDOGECOIN_API dogecoin_bool dogecoin_encrypt_seed_with_sw(const SEED seed, con
         }
         fp = _wfopen(fullpath, overwrite ? L"wb+" : L"wb");
     #else
-        if (mkdir(CRYPTO_DIR_PATH, 0777) == -1 && errno != EEXIST)
+        if (mkdir(CRYPTO_DIR_PATH, 0700) == -1 && errno != EEXIST)
         {
             fprintf(stderr, "ERROR: Failed to create directory\n");
             return false;
         }
         char fullpath[FILE_PATH_MAX_LEN] = {0};
         snprintf(fullpath, sizeof(fullpath), SEED_SW_FILE_NAME, file_num);
-        if (!overwrite && access(fullpath, F_OK) != -1)
+        /* Exclusive create rather than access() then open(): between those
+           two calls anyone able to write the directory can plant a symlink
+           and redirect an encrypted seed somewhere of their choosing.
+           O_EXCL both reports the collision and refuses to follow one. */
+        fp = dogecoin_fopen_private(fullpath, overwrite ? "wb+" : "wbx");
+        if (!fp && !overwrite && errno == EEXIST)
         {
             fprintf(stderr, "ERROR: File already exists. Use overwrite flag to replace it.\n");
             return false;
         }
-        fp = fopen(fullpath, overwrite ? "wb+" : "wb");
     #endif
         if (!fp)
         {
@@ -2101,19 +2105,23 @@ LIBDOGECOIN_API dogecoin_bool dogecoin_generate_hdnode_encrypt_with_sw(dogecoin_
         }
         fp = _wfopen(fullpath, overwrite ? L"wb+" : L"wb");
 #else
-        if (mkdir(CRYPTO_DIR_PATH, 0777) == -1 && errno != EEXIST)
+        if (mkdir(CRYPTO_DIR_PATH, 0700) == -1 && errno != EEXIST)
         {
             fprintf(stderr, "ERROR: Failed to create directory\n");
             return false;
         }
         char fullpath[FILE_PATH_MAX_LEN] = {0};
         snprintf(fullpath, sizeof(fullpath), MASTER_SW_FILE_NAME, file_num);
-        if (!overwrite && access(fullpath, F_OK) != -1)
+        /* Exclusive create rather than access() then open(): between those
+           two calls anyone able to write the directory can plant a symlink
+           and redirect an encrypted seed somewhere of their choosing.
+           O_EXCL both reports the collision and refuses to follow one. */
+        fp = dogecoin_fopen_private(fullpath, overwrite ? "wb+" : "wbx");
+        if (!fp && !overwrite && errno == EEXIST)
         {
             fprintf(stderr, "ERROR: File already exists. Use overwrite flag to replace it.\n");
             return false;
         }
-        fp = fopen(fullpath, overwrite ? "wb+" : "wb");
 #endif
         if (!fp)
         {
@@ -2984,19 +2992,23 @@ LIBDOGECOIN_API dogecoin_bool dogecoin_generate_mnemonic_encrypt_with_sw(MNEMONI
         }
         fp = _wfopen(fullpath, overwrite ? L"wb+" : L"wb");
 #else
-        if (mkdir(CRYPTO_DIR_PATH, 0777) == -1 && errno != EEXIST)
+        if (mkdir(CRYPTO_DIR_PATH, 0700) == -1 && errno != EEXIST)
         {
             fprintf(stderr, "ERROR: Failed to create directory\n");
             return false;
         }
         char fullpath[FILE_PATH_MAX_LEN] = {0};
         snprintf(fullpath, sizeof(fullpath), MNEMONIC_SW_FILE_NAME, file_num);
-        if (!overwrite && access(fullpath, F_OK) != -1)
+        /* Exclusive create rather than access() then open(): between those
+           two calls anyone able to write the directory can plant a symlink
+           and redirect an encrypted seed somewhere of their choosing.
+           O_EXCL both reports the collision and refuses to follow one. */
+        fp = dogecoin_fopen_private(fullpath, overwrite ? "wb+" : "wbx");
+        if (!fp && !overwrite && errno == EEXIST)
         {
             fprintf(stderr, "ERROR: File already exists. Use overwrite flag to replace it.\n");
             return false;
         }
-        fp = fopen(fullpath, overwrite ? "wb+" : "wb");
 #endif
         if (!fp)
         {

src/utils.c

@@ -32,6 +32,11 @@
 #endif
 
 #include <ctype.h>
+#ifndef _WIN32
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdbool.h>
@@ -678,6 +683,71 @@ char* concat(char* prefix, char* suffix) {
     return file;
 }
 
+
+FILE* dogecoin_fopen_private(const char* path, const char* mode)
+{
+#if defined(USE_OPTEE) || defined(_WIN32)
+    /* OP-TEE provides fopen but not the POSIX open/fdopen/close this uses --
+       referencing them fails the TA link even though nothing in the TA calls
+       this function. Fall back rather than return NULL: the same library is
+       linked for the host side of an OP-TEE build, where wallet.c does open
+       files, and stubbing this out made those calls fail and took the mode
+       assertions with them.
+       Windows has no umask, so a new file inherits the directory ACL and plain
+       fopen already gets whatever the parent grants.
+       Neither platform gets the 0600-on-create guarantee; on OP-TEE the TA has
+       no filesystem to guarantee it for. */
+    return fopen(path, mode);
+#else
+    int flags;
+    int fd;
+    FILE* fp;
+
+    if (!path || !mode) {
+        return NULL;
+    }
+
+    /* Translate the stdio mode rather than assuming it creates.
+     *
+     * O_CREAT was previously unconditional, so "r"/"r+" -- open an existing
+     * file -- would create one instead of failing, and a read-only file could
+     * not be opened at all because O_RDWR was also unconditional. Both matter
+     * here: this is the wallet path, where "open the existing wallet" and
+     * "start a new one" are different operations and conflating them can
+     * present an empty wallet as a real one. */
+    switch (mode[0]) {
+        case 'r':
+            flags = strchr(mode, '+') ? O_RDWR : O_RDONLY;
+            break;
+        case 'w':
+            flags = (strchr(mode, '+') ? O_RDWR : O_WRONLY) | O_CREAT;
+            /* C11 'x': create exclusively, fail if the path exists. Callers use
+               it to replace a check-then-open, which is a race an attacker with
+               write access to the directory wins by planting a symlink between
+               the two calls. O_EXCL also refuses to follow one. */
+            flags |= strchr(mode, 'x') ? O_EXCL : O_TRUNC;
+            break;
+        case 'a':
+            flags = (strchr(mode, '+') ? O_RDWR : O_WRONLY) | O_CREAT | O_APPEND;
+            break;
+        default:
+            return NULL;
+    }
+    /* 0600. The mode only applies when the file is created, so an existing
+       file keeps whatever permissions it already had -- this tightens new
+       files without silently changing anyone's existing ones. */
+    fd = open(path, flags, S_IRUSR | S_IWUSR);
+    if (fd < 0) {
+        return NULL;
+    }
+    fp = fdopen(fd, mode);
+    if (!fp) {
+        close(fd);
+    }
+    return fp;
+#endif
+}
+
 void slice(const char *str, char *result, size_t start, size_t end)
 {
     strncpy(result, str + start, end - start);

src/wallet.c

@@ -980,7 +980,7 @@ dogecoin_bool dogecoin_wallet_create(dogecoin_wallet* wallet, const char* file_p
 
     // open wallet file if not already open
     if (!wallet->dbfile) {
-        wallet->dbfile = fopen(file_path, "a+b");
+        wallet->dbfile = dogecoin_fopen_private(file_path, "a+b");
         if (wallet->dbfile) {
             snprintf((char*)wallet->filename, sizeof(wallet->filename), "%s", file_path);
         }
@@ -1146,7 +1146,13 @@ dogecoin_bool dogecoin_wallet_load(dogecoin_wallet* wallet, const char* file_pat
         }
     }
 
-    wallet->dbfile = fopen(file_path, *created ? "a+b" : "r+b");
+    /* The creation path runs here, not in dogecoin_wallet_create(): that
+       function opens the file only when wallet->dbfile is still NULL, and by
+       the time it is called below this line has already set it. Opening
+       privately there tightened nothing, so the wallet database was created at
+       the process umask -- world-readable under a permissive one. */
+    wallet->dbfile = *created ? dogecoin_fopen_private(file_path, "a+b")
+                              : fopen(file_path, "r+b");
     if (wallet->dbfile) {
         snprintf((char*)wallet->filename, sizeof(wallet->filename), "%s", file_path);
     }

test/unittester.c

@@ -97,6 +97,7 @@ extern void test_invalid_tx_deser();
 extern void test_tx_sign();
 extern void test_scripts();
 extern void test_utils();
+extern void test_utils_fopen_private();
 extern void test_vector();
 extern void test_qr();
 
@@ -131,6 +132,7 @@ extern void test_examples();
 #ifdef WITH_WALLET
 extern void test_wallet_basics();
 extern void test_wallet();
+extern void test_wallet_file_is_private();
 extern void test_wallet_malformed_reclen();
 extern void test_wallet_reorg_utxo_update();
 extern void test_wallet_utxo_idx_not_reused();
@@ -232,6 +234,7 @@ int main()
     u_run_test(test_script_parse);
     u_run_test(test_script_op_codeseperator);
     u_run_test(test_utils);
+    u_run_test(test_utils_fopen_private);
     u_run_test(test_vector);
     u_run_test(test_qr);
 
@@ -266,6 +269,7 @@ int main()
 #ifdef WITH_WALLET
     u_run_test(test_wallet_basics);
     u_run_test(test_wallet);
+    u_run_test(test_wallet_file_is_private);
     u_run_test(test_wallet_malformed_reclen);
     u_run_test(test_wallet_reorg_utxo_update);
     u_run_test(test_wallet_utxo_idx_not_reused);

test/utils_tests.c

@@ -5,9 +5,29 @@
  * Distributed under the MIT software license, see the accompanying   *
  * file COPYING or http://www.opensource.org/licenses/mit-license.php.*
  **********************************************************************/
+#include <errno.h>
+#ifndef _WIN32
+#include <unistd.h>
+#endif
 #include <test/utest.h>
 
+#ifndef _WIN32
+/* Android has no writable /tmp; the rest of this suite already special-cases
+   it for wallettmpfile. */
+#ifdef __ANDROID__
+#define DOGECOIN_TEST_TMPDIR "/data/local/tmp"
+#else
+#define DOGECOIN_TEST_TMPDIR "/tmp"
+#endif
+#endif
+
+
+
 #include <assert.h>
+#ifndef _WIN32
+#include <sys/stat.h>
+#include <sys/types.h>
+#endif
 #include <dogecoin/utils.h>
 
   /* test a buffer overflow protection */
@@ -132,3 +152,89 @@ void test_dit()
         debug_print("%s", "DIT test: disabled (DIT not supported)\n");
     }
 }
+
+
+/*
+ * The wallet database and the sealed seed files were created with plain
+ * fopen(), so their mode came from the process umask -- 0664 under the common
+ * 0002, i.e. readable by every local user and writable by the group.
+ *
+ * The wallet database holds the master public key, which is enough to derive
+ * every address and reconstruct the wallet's transaction history. The seal
+ * files hold encrypted seeds and mnemonics; encryption means a leak is not an
+ * immediate compromise, but handing out the ciphertext invites an offline
+ * attack on a password-derived key.
+ */
+void test_utils_fopen_private()
+{
+/* The 0600-on-create guarantee only exists on the POSIX path. Windows has no
+   umask, and OP-TEE has no filesystem in the TA and no open/fdopen/close to
+   build it from, so both fall back to plain fopen. */
+#if !defined(_WIN32) && !defined(USE_OPTEE)
+    /* A private directory of our own, rather than a fixed name in the working
+       directory: two test runs in the same tree would otherwise share the file,
+       and every stat()-then-open below would be a real check-then-use against a
+       path anyone can pre-create. mkdtemp gives us 0700 and a name nobody can
+       predict. */
+    char dir[] = DOGECOIN_TEST_TMPDIR "/dogecoin_fopen_priv_XXXXXX";
+    u_assert_true(mkdtemp(dir) != NULL);
+    char path[128];
+    snprintf(path, sizeof(path), "%s/f.tmp", dir);
+    struct stat st;
+    FILE* fp;
+
+    fp = dogecoin_fopen_private(path, "wb");
+    u_assert_true(fp != NULL);
+    fputc('x', fp);
+    /* fstat the descriptor we hold, not the path. Re-resolving the name would
+       ask about whatever is there now rather than the file that was opened --
+       the same check-then-use the function under test exists to avoid. */
+    u_assert_int_eq(fstat(fileno(fp), &st), 0);
+    /* Owner read/write only: no group or other bits at all. */
+    u_assert_int_eq((int)(st.st_mode & 07777), 0600);
+    u_assert_int_eq((int)(st.st_mode & (S_IRWXG | S_IRWXO)), 0);
+    fclose(fp);
+
+    /* Reopening must not widen the mode of a file that already exists. */
+    fp = dogecoin_fopen_private(path, "a+b");
+    u_assert_true(fp != NULL);
+    u_assert_int_eq(fstat(fileno(fp), &st), 0);
+    u_assert_int_eq((int)(st.st_mode & 07777), 0600);
+    fclose(fp);
+
+    remove(path);
+
+    /* NULL arguments are refused rather than passed through. */
+    u_assert_true(dogecoin_fopen_private(NULL, "wb") == NULL);
+    u_assert_true(dogecoin_fopen_private(path, NULL) == NULL);
+
+    /* "r" must not create. O_RDWR|O_CREAT used to be unconditional, so asking
+       to open an existing file created an empty one instead of failing -- on
+       the wallet path that turns "open my wallet" into "start a new one". */
+    remove(path);
+    fp = dogecoin_fopen_private(path, "rb");
+    u_assert_true(fp == NULL);
+    /* Nothing was created: a second open of the same mode still fails. Asking
+       stat() instead would re-resolve the path, which is the pattern being
+       tested against. */
+    fp = dogecoin_fopen_private(path, "rb");
+    u_assert_true(fp == NULL);
+
+    /* "wx": exclusive create, which is what replaced the access()-then-open
+       race in seal.c. Refuses an existing file and will not follow a symlink
+       planted between the two calls the old pattern needed. */
+    fp = dogecoin_fopen_private(path, "wbx");
+    u_assert_true(fp != NULL);
+    u_assert_int_eq(fstat(fileno(fp), &st), 0);
+    u_assert_int_eq((int)(st.st_mode & 07777), 0600);
+    fclose(fp);
+
+    fp = dogecoin_fopen_private(path, "wbx");
+    u_assert_true(fp == NULL);
+    u_assert_int_eq(errno, EEXIST);
+
+    remove(path);
+    rmdir(dir);
+#endif /* _WIN32 */
+}
+