diff --git a/include/dogecoin/utils.h b/include/dogecoin/utils.h index da2d0fdf7..a08ac1a80 100644 --- a/include/dogecoin/utils.h +++ b/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); diff --git a/src/seal.c b/src/seal.c index 398084256..7776d6331 100644 --- a/src/seal.c +++ b/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) { diff --git a/src/utils.c b/src/utils.c index f0af3f94d..7b8f46673 100644 --- a/src/utils.c +++ b/src/utils.c @@ -32,6 +32,11 @@ #endif #include +#ifndef _WIN32 +#include +#include +#include +#endif #include #include #include @@ -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); diff --git a/src/wallet.c b/src/wallet.c index 6c567d3f2..8961da440 100644 --- a/src/wallet.c +++ b/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); } diff --git a/test/unittester.c b/test/unittester.c index 96dd35fae..14ec428a5 100644 --- a/test/unittester.c +++ b/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); diff --git a/test/utils_tests.c b/test/utils_tests.c index af2fc90ea..61c98fccd 100644 --- a/test/utils_tests.c +++ b/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 +#ifndef _WIN32 +#include +#endif #include +#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 +#ifndef _WIN32 +#include +#include +#endif #include /* 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 */ +} + diff --git a/test/wallet_tests.c b/test/wallet_tests.c index 89a812525..e0f823737 100644 --- a/test/wallet_tests.c +++ b/test/wallet_tests.c @@ -19,6 +19,17 @@ static const char *wallettmpfile = "/tmp/dummy"; #endif #endif +#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 #include #include @@ -160,6 +171,46 @@ static const char * wallet_txns[] = { "020000000849afa76e800d86894dc7e6e8adf8986dc4bde2fc3c465dd549977fff5b71664a010000006b483045022100b06ebe40a3a4dde1b83a27a339c573107e2f5797ff461cc99e8e077951895d1702205da2c0ae2c05c2edb0801227fffaea9578eeda0fb831c59410eecbb5b62f8ee90121030c1a563c15d058adec64136f57454b10afb910782c2fbf783c10c9402afef068feffffffece35a2f637923635afbbcc3829704332b73a07fc941e34542c202a9a6975fd51e0000006b4830450221009254f188e3f89664cc7b3c8c2fefa8ec3195542781b029efdf9104e0cf1bc5080220150e9b558fd495b358dbe7462c94a15ead15bfcfff653114effd5021360fce29012103dee8b7607da89842428755f792b5cf683dfa239b20c3940cea85d2b7bdf2a7f2feffffffdf185aae1b0663fc0a06941fd49ed691e194fbf35d301cc750aa00b7c8daf42e000000006b483045022100cd72dac5a73afabe1d776214bc863d0643a518d1ba8062023398adc7175b25da02204c09d4200827c1696f9da13ba7bd0624cbff73b4d1f86affe70bc60e6335efb8012103cc0b3842090d0be5282b03e679839a2b6ae33f8166a8c32c91e548d673cccd81feffffff0901f6deae27dd720d456522f774aa4f98484ba02ab27d7e2f05e0d9b288d39f000000006b483045022100f26c99b2d9472fbddf7bdf305abfc2c9737eb3202a1cb8f5229fe13cde60d89d02207bfcf757bcd2850cf63544df58b521f8d4788294b214db6b9d39d940d8517871012103d5c532eeb7d17931a9d5ffa3a02ab18aa9d63a0e8b00c689b5fdb546dfb08f22feffffffdde5461b1f14d447d3a90f27395d4ebe6821e0e06e560c69b74b2593a27a516f010000006a473044022037ed0f2b2d63054d55b37688dadeebc777c6508d75957dd2e657773c7efa3e6e02204661d024d5dc27deee69dc244f662348720a088f014be311cf04dabf4a6e15ee01210367728538c117ccb63a475f6e896f1f17d87e09259fc02a8b9c47fb6c6343f2c5feffffff76e101242ef08a30654f8195771abfa2b8815cc483414d12ae5b60ecf36d4bc3010000006a4730440220664af6cb65b2613152327347f43b7dfc9015553f215d8d753d2c5d67414d94bf02200e2a36bb7dc825222fd58cc6ad99c17f76f308a690b9b1cd2790c7305dea4bd50121023c224d1dfa52f62b4bb3353f6adf7c1eea27850965ac8fb40e0a34e4701a749bfeffffff676190fcd2677ec1551ac1becad8f616d3ec7aa8d27e7e3828e40059566e4971000000006a473044022047d5afc8a060ec3e056e479287a5066a805a131dbd7ece93c9c92ddcbd130cca022047a41908c6a15603c6b23235e82fda39d27e3c17339db483dfb9cbbbdd200584012103f7402cdfa05ba65f76027f76f468d5ae302995ebc5c1d46b5aab06d6c38a8d4dfeffffffe3a6e193f057e9fb39a25504e17dd1c81b11c7c7619131f590ddbd1bdebbe784010000006a4730440220120ac2306b0aac057903da20f2a4a08a4b407cac416bd3deed015a0c998a935a0220407222f861b3972c3b57981cfedb2280428bfdffe7fb72d69c47c81a3fc978f60121038a607d3f35e020ca3d5ffe86b5eb1ed2dee0ff8918467c580d489a2bc6a36e7bfeffffff02c0bdd204000000001976a9145e6f9105b1a9100686139d0f0898b03517ad7c0588acb6f40c00000000001976a91402fbbaa336f0e82c985a58329059d2d4484f50ec88ac68860700", "010000000187e274649a4316f8ac6c9455d4423e1d66f2d26f0e9750d0989fff31b756bf9400000000da00483045022100a5fb5dbaa5de30b786e919126e6afa0724880a2d7da4e70a8110027ad7d10e9302204a60860c03c16414440fb66856ccbaac6e049838c8f886968f6ccffba7d4435b0147304402202e7bbaf5a81611c456128e573b3de0264aa8cad806ec647452d46f16c33ad00d02207625319a38665e54daad5ad1c73279636fd8869f7d833dc25717135b2b67524101475221032896838cbccf49a5269f551f0a5bb942fa854f183f23995bfb7f563add01a1942103304189fcd189d3245da6f96f4578e49f4a42eaf98dbd78d7376f627824e60fc952ae0000000003edfbd100000000001976a914e195b669de8e49f955749033fa2d79390732c43588ac21258c00000000001976a914ccac0dbbb5e80607e9167fcc1f1d07dcfcc4418b88ac3a37a3000000000017a9142e0065cd27ed91ef25c4d7c74f21d2516598b5f08700000000" }; + +/* The wallet database holds keys. It was created at the process umask, which + on a permissive one (0022) leaves it world-readable -- and the private open + added for it sat in dogecoin_wallet_create(), which never runs on the + creation path because dogecoin_wallet_load() has already set wallet->dbfile + by the time it is called. */ +void test_wallet_file_is_private() +{ +#if !defined(_WIN32) && !defined(USE_OPTEE) + /* mkdtemp, not mkstemp-then-unlink: unlinking and then asking the library + to create the same path is itself a check-then-use, and the name is + already known by then. A 0700 directory nobody else can enter makes the + creation below unambiguous. */ + char dir[] = DOGECOIN_TEST_TMPDIR "/dogecoin_wallet_perm_XXXXXX"; + u_assert_true(mkdtemp(dir) != NULL); + char path[128]; + snprintf(path, sizeof(path), "%s/w.db", dir); + + mode_t old = umask(0022); /* the permissive case that exposed this */ + + dogecoin_wallet *wallet = dogecoin_wallet_new(&dogecoin_chainparams_main); + int error = 0; + dogecoin_bool created = false; + u_assert_int_eq(dogecoin_wallet_load(wallet, path, &error, &created, false), true); + u_assert_true(created); + + /* fstat the handle the wallet is holding rather than re-resolving the + path: this asserts the mode of the file that was actually created. */ + struct stat st; + u_assert_true(wallet->dbfile != NULL); + u_assert_int_eq(fstat(fileno(wallet->dbfile), &st), 0); + u_assert_int_eq((int)(st.st_mode & 0777), 0600); + + dogecoin_wallet_free(wallet); + umask(old); + unlink(path); + rmdir(dir); +#endif +} + void test_wallet() { // test balance of random choosen mainnet address 1MZnPNbhtmRjzAHqEikQYB7ENaRd5ky4aT