# # # add_dir "tests/key_files_created_securely" # # add_file "tests/key_files_created_securely/__driver__.lua" # content [c1e2b815fcc95a7ecda0d30f68156af8e8049c92] # # patch "file_io.cc" # from [7d1a38d3b91641d18ed693e95a17cebf1a16d657] # to [1edaa600f491b3cddb7628be8f0ab5d218971815] # # patch "file_io.hh" # from [8f941a533383f855723343c352dc6aba3f0c9542] # to [a9c4eac0ba8fd78a612dc775dcef76424769e2be] # # patch "key_store.cc" # from [fda46d5fa8a5b2a52421c1f83413a208e2c6401f] # to [5baf95cf24e95172f259319f64c683560ecbcd17] # # patch "platform.hh" # from [3f3e42cdc841892c1e0e5e53e8041959defbe2ed] # to [aa2f74b91132218c53c92c035a177893b56a75c6] # # patch "unix/fs.cc" # from [e42a37d298d1386bbe9ae2629532f2e79600956d] # to [ba93ca82fc6f6012b20fa0b63c2f639a6857b820] # # patch "win32/fs.cc" # from [980b3be3de2727abe892d069ed998f2d6695b385] # to [ab766a3ee7e4cc525b4d2053307f2ba34bf37351] # ============================================================ --- tests/key_files_created_securely/__driver__.lua c1e2b815fcc95a7ecda0d30f68156af8e8049c92 +++ tests/key_files_created_securely/__driver__.lua c1e2b815fcc95a7ecda0d30f68156af8e8049c92 @@ -0,0 +1,22 @@ +-- FIXME implement for Windows. +skip_if(ostype == "Windows") +skip_if(not existsonpath("stat")) + +function quote_for_shell(argv) + ret = "" + for _,arg in ipairs(argv) do + ret = ret .. " '" .. string.gsub(arg, "'", "'\\''") .. "'" + end + return ret +end + +mtn_setup() + +cmd = quote_for_shell(raw_mtn("--keydir=keys", "genkey", "foobar")) + +-- force a permissive umask to be in effect +check({ "sh", "-c", "umask 0000; exec" .. cmd }, + 0, false, false, string.rep("foobar\n", 2)) + +check({ "ls", "-l", "keys/foobar" }, 0, true, nil) +check(qgrep("^-rw------- .*keys/foobar", "stdout")) ============================================================ --- file_io.cc 7d1a38d3b91641d18ed693e95a17cebf1a16d657 +++ file_io.cc 1edaa600f491b3cddb7628be8f0ab5d218971815 @@ -392,53 +392,34 @@ read_data_for_command_line(utf8 const & // trying, and I'm not sure it's really a strict requirement of this tool, // but you might want to make this code a bit tighter. - static void write_data_impl(any_path const & p, data const & dat, - any_path const & tmp) + any_path const & tmp, + bool user_private) { N(!directory_exists(p), F("file '%s' cannot be overwritten as data; it is a directory") % p); make_dir_for(p); - { - // data.tmp opens - ofstream file(tmp.as_external().c_str(), - ios_base::out | ios_base::trunc | ios_base::binary); - N(file, F("cannot open file %s for writing") % tmp); - Botan::Pipe pipe(new Botan::DataSink_Stream(file)); - pipe.process_msg(dat()); - // data.tmp closes - } - - rename_clobberingly(tmp, p); + write_data_worker(p.as_external(), dat(), tmp.as_external(), user_private); } -static void -write_data_impl(any_path const & p, - data const & dat) -{ - // we write, non-atomically, to _MTN/data.tmp. - // nb: no mucking around with multiple-writer conditions. we're a - // single-user single-threaded program. you get what you paid for. - assert_path_is_directory(bookkeeping_root); - bookkeeping_path tmp = bookkeeping_root / (FL("data.tmp.%d") % - get_process_id()).str(); - write_data_impl(p, dat, tmp); -} - void write_data(file_path const & path, data const & dat) { - write_data_impl(path, dat); + // use the bookkeeping root as the temporary directory. + assert_path_is_directory(bookkeeping_root); + write_data_impl(path, dat, bookkeeping_root, false); } void write_data(bookkeeping_path const & path, data const & dat) { - write_data_impl(path, dat); + // use the bookkeeping root as the temporary directory. + assert_path_is_directory(bookkeeping_root); + write_data_impl(path, dat, bookkeeping_root, false); } void @@ -446,10 +427,17 @@ write_data(system_path const & path, data const & data, system_path const & tmpdir) { - write_data_impl(path, data, tmpdir / (FL("data.tmp.%d") % - get_process_id()).str()); + write_data_impl(path, data, tmpdir, false); } +void +write_data_userprivate(system_path const & path, + data const & data, + system_path const & tmpdir) +{ + write_data_impl(path, data, tmpdir, true); +} + tree_walker::~tree_walker() {} static inline bool ============================================================ --- file_io.hh 8f941a533383f855723343c352dc6aba3f0c9542 +++ file_io.hh a9c4eac0ba8fd78a612dc775dcef76424769e2be @@ -94,6 +94,12 @@ void write_data(system_path const & path data const & data, system_path const & tmpdir); +// Identical to the above, but the file will be inaccessible to anyone but +// the user. Use for things like private keys. +void write_data_userprivate(system_path const & path, + data const & data, + system_path const & tmpdir); + class tree_walker { public: ============================================================ --- key_store.cc fda46d5fa8a5b2a52421c1f83413a208e2c6401f +++ key_store.cc 5baf95cf24e95172f259319f64c683560ecbcd17 @@ -1,5 +1,4 @@ #include -#include #include "key_store.hh" #include "file_io.hh" @@ -212,11 +211,9 @@ key_store::write_key(rsa_keypair_id cons system_path file; get_key_file(ident, file); - // set a restrictive umask, write the file and reset umask - mode_t mask = umask(S_IRWXG|S_IRWXO); + // Make sure the private key is not readable by anyone other than the user. L(FL("writing key '%s' to file '%s' in dir '%s'") % ident % file % key_dir); - write_data(file, dat, key_dir); - umask(mask); + write_data_userprivate(file, dat, key_dir); } bool ============================================================ --- platform.hh 3f3e42cdc841892c1e0e5e53e8041959defbe2ed +++ platform.hh aa2f74b91132218c53c92c035a177893b56a75c6 @@ -121,6 +121,10 @@ void rename_clobberingly(std::string con path::status get_path_status(std::string const & path); void rename_clobberingly(std::string const & from, std::string const & to); +void write_data_worker(std::string const & p, + std::string const & dat, + std::string const & tmpdir, + bool user_private); // strerror wrapper for OS-specific errors (e.g. use FormatMessage on Win32) std::string os_strerror(os_err_t errnum); ============================================================ --- unix/fs.cc e42a37d298d1386bbe9ae2629532f2e79600956d +++ unix/fs.cc ba93ca82fc6f6012b20fa0b63c2f639a6857b820 @@ -8,9 +8,10 @@ #include #include #include +#include #include #include -#include +#include #include #include @@ -125,6 +126,139 @@ rename_clobberingly(std::string const & F("renaming '%s' to '%s' failed: %s") % from % to % os_strerror(errno)); } +// Create a temporary file in directory DIR, writing its name to NAME and +// returning a read-write file descriptor for it. If unable to create +// the file, throws an E(). +// + +// N.B. None of the standard temporary-file creation routines in libc do +// what we want (mkstemp almost does, but it doesn't let us specify the +// mode). This logic borrowed from libiberty's mkstemps(). To avoid grief +// with case-insensitive file systems (*cough* OSX) we use only lowercase +// letters for the name. This reduces the number of possible temporary +// files from 62**6 to 36**6, oh noes. + +static int +make_temp_file(std::string const & dir, std::string & name, mode_t mode) +{ + static const char letters[] + = "abcdefghijklmnopqrstuvwxyz0123456789"; + + const u32 base = sizeof letters - 1; + const u32 limit = base*base*base * base*base*base; + + static u32 value; + struct timeval tv; + std::string tmp = dir + "/mtxxxxxx.tmp"; + + gettimeofday(&tv, 0); + value += ((u32) tv.tv_usec << 16) ^ tv.tv_sec ^ getpid(); + value %= limit; + + for (u32 i = 0; i < limit; i++) + { + u32 v = value; + + tmp.at(tmp.size() - 11) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 10) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 9) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 8) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 7) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 6) = letters[v % base]; + v /= base; + + int fd = open(tmp.c_str(), O_RDWR|O_CREAT|O_EXCL, mode); + + if (fd >= 0) + { + name = tmp; + return fd; + } + + // EEXIST means we should go 'round again. Any other errno value is a + // plain error. (ENOTDIR is a bug, and so are some ELOOP and EACCES + // conditions - caller's responsibility to make sure that 'dir' is in + // fact a directory to which we can write - but we get better + // diagnostics from this E() than we would from an I().) + + E(errno == EEXIST, + F("cannot create temp file %s: %s") % tmp % os_strerror(errno)); + + // This increment is relatively prime to 'limit', therefore 'value' + // will visit every number in its range. + value += 7777; + value %= limit; + } + + // we really should never get here. + E(false, F("all %d possible temporary file names are in use") % limit); +} + + +// Write string DAT atomically to file FNAME, using TMP as the location to +// create a file temporarily. rename(2) from an arbitrary filename in TMP +// to FNAME must work (i.e. they must be on the same filesystem). +// If USER_PRIVATE is true, the file will be potentially accessible only to +// the user, else it will be potentially accessible to everyone (i.e. open() +// will be passed mode 0600 or 0666 -- the actual permissions are modified +// by umask as usual). +void +write_data_worker(std::string const & fname, + std::string const & dat, + std::string const & tmpdir, + bool user_private) +{ + struct auto_closer + { + int fd; + auto_closer(int fd) : fd(fd) {} + ~auto_closer() { close(fd); } + }; + + std::string tmp; + int fd = make_temp_file(tmpdir, tmp, user_private ? 0600 : 0666); + + { + auto_closer guard(fd); + + char const * ptr = dat.data(); + size_t remaining = dat.size(); + int deadcycles = 0; + + L(FL("writing %s via temp %s") % fname % tmp); + + do + { + ssize_t written = write(fd, ptr, remaining); + E(written >= 0, + F("error writing to temp file %s: %s") % tmp % os_strerror(errno)); + if (written == 0) + { + deadcycles++; + E(deadcycles < 4, + FP("giving up after four zero-length writes to %s " + "(%d byte written, %d left)", + "giving up after four zero-length writes to %s " + "(%d bytes written, %d left)", + ptr - dat.data()) + % tmp % (ptr - dat.data()) % remaining); + } + ptr += written; + remaining -= written; + } + while (remaining > 0); + } + // fd is now closed + + rename_clobberingly(tmp, fname); +} + + // Local Variables: // mode: C++ // fill-column: 76 ============================================================ --- win32/fs.cc 980b3be3de2727abe892d069ed998f2d6695b385 +++ win32/fs.cc ab766a3ee7e4cc525b4d2053307f2ba34bf37351 @@ -145,8 +145,10 @@ rename_clobberingly_impl(const char * fr // more compatible DeleteFile/MoveFile pair as a compatibility fall-back. typedef BOOL (WINAPI *MoveFileExFun)(LPCTSTR, LPCTSTR, DWORD); static MoveFileExFun fnMoveFileEx = 0; - static bool MoveFileExAvailable = false; - if (fnMoveFileEx == 0) + + static enum { UNKNOWN, YES, NO } MoveFileExAvailable = UNKNOWN; + + if (MoveFileExAvailable == UNKNOWN) { HMODULE hModule = LoadLibrary("kernel32"); if (hModule) @@ -155,30 +157,33 @@ rename_clobberingly_impl(const char * fr if (fnMoveFileEx) { L(FL("using MoveFileEx for renames")); - MoveFileExAvailable = true; + MoveFileExAvailable = YES; } else - L(FL("using DeleteFile/MoveFile fallback for renames")); + { + L(FL("using DeleteFile/MoveFile fallback for renames")); + MoveFileExAvailable = NO; + } + if (hModule) + FreeLibrary(hModule); } - if (MoveFileExAvailable) + if (MoveFileExAvailable == YES) { if (fnMoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING)) return true; - else if (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) - { - MoveFileExAvailable = false; - L(FL("MoveFileEx failed with CALL_NOT_IMPLEMENTED, using fallback")); + else if (GetLastError() != ERROR_CALL_NOT_IMPLEMENTED) + return false; + else + { + MoveFileExAvailable = NO; + L(FL("MoveFileEx failed with CALL_NOT_IMPLEMENTED, using fallback")); } } - else - { - // This is not even remotely atomic, but what can you do? - DeleteFile(to); - if (MoveFile(from, to)) - return true; - } - return false; + + // This is not even remotely atomic, but what can you do? + DeleteFile(to); + return MoveFile(from, to); } void @@ -206,6 +211,149 @@ rename_clobberingly(std::string const & % os_strerror(lastError) % lastError); } +// Create a temporary file in directory DIR, writing its name to NAME and +// returning a read-write file descriptor for it. If unable to create +// the file, throws an E(). +// +// N.B. We could use GetTempFileName but it wouldn't help significantly, as +// we want to do the CreateFile ourselves (eventually we will want to +// specify security attributes). This logic borrowed from libiberty's +// mkstemps(), with uppercase characters removed from 'letters' as Windows +// has a case insensitive file system. + +static HANDLE +make_temp_file(std::string const & dir, std::string & name) +{ + static const char letters[] + = "abcdefghijklmnopqrstuvwxyz0123456789"; + + const u32 base = sizeof letters - 1; + const u32 limit = base*base*base * base*base*base; + + static u32 value; + std::string tmp = dir + "/mtxxxxxx.tmp"; + + value += GetTickCount() ^ GetCurrentProcessId(); + + for (u64 i = 0; i < limit; i++) + { + char *X = tmp.c_str() + tmp.size() - 7; + u64 v = value; + + tmp.at(tmp.size() - 11) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 10) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 9) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 8) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 7) = letters[v % base]; + v /= base; + tmp.at(tmp.size() - 6) = letters[v % base]; + v /= base; + + HANDLE h = CreateFile(tmp.c_str(), GENERIC_READ|GENERIC_WRITE, + 0, // exclusive access + (LPSECURITY_ATTRIBUTES)0, // default security + CREATE_NEW, FILE_ATTRIBUTE_NORMAL, + (HANDLE)0); // no template file + + if (h != INVALID_HANDLE_VALUE) + { + name = tmp; + return h; + } + + // ERROR_ALREADY_EXISTS means we should go 'round again. Any other + // GetLastError() value is a plain error. (Presumably, just as for + // Unix, there are values that would represent bugs.) + E(GetLastError() == ERROR_ALREADY_EXISTS, + F("cannot create temp file %s: %s") + % tmp % os_strerror(GetLastError())); + + // This increment is relatively prime to any power of two, therefore + // 'value' will visit every number in its range. + value += 7777; + } + E(false, + F("cannot find a temporary file (tried %d possibilities)") % limit); +} + + +// Write string DAT atomically to file FNAME, using TMP as the location to +// create a file temporarily. rename(2) from an arbitrary filename in TMP +// to FNAME must work (i.e. they must be on the same filesystem). +// If USER_PRIVATE is true, the file will be potentially accessible only to +// the user, else it will be potentially accessible to everyone. +void +write_data_worker(std::string const & fname, + std::string const & dat, + std::string const & tmpdir, + bool user_private) +{ + // USER_PRIVATE true is not implemented for Windows. It is a thing that + // can be done, at least under NT-family Windows - we would need to pass a + // SECURITY_ATTRIBUTES structure to the CreateFile call, specifying a + // discretionary ACL that denies access to anyone other than the owner - + // but from what little sense I can make of the MSDN documentation, + // constructing such an ACL is quite complicated and I am not confident I + // would get it right. Better someone who knows Windows should code it. + // [ Code at http://groups.google.com/group/comp.protocols.kerberos/ + // browse_thread/thread/9e37e931de022791/c5172d5b8c5aa48e%23c5172d5b8c5aa48e + // might be recyclable to the purpose. ] + + W(user_private, + F("%s will be accessible to all users of this computer\n") % fname); + + struct auto_closer + { + HANDLE h; + auto_closer(HANDLE h) : h(h) {} + ~auto_closer() { CloseHandle(h); } + }; + + string tmp; + HANDLE h = make_temp_file(tmpdir, tmp); + + { + auto_closer guard(h); + + char const * ptr = dat.data(); + DWORD remaining = dat.size(); + int deadcycles = 0; + + L(FL("writing %s via temp %s") % fname % tmp); + + do + { + DWORD written; + E(WriteFile(h, (LPCVOID)ptr, remaining, &written, (LPOVERLAPPED)0), + F("error writing to temp file %s: %s") + % tmp % os_strerror(GetLastError())); + + if (written == 0) + { + deadcycles++; + E(deadcycles < 4, + FP("giving up after four zero-length writes to %s " + "(%d byte written, %d left)", + "giving up after four zero-length writes to %s " + "(%d bytes written, %d left)", + ptr - dat.data()) + % tmp % (ptr - dat.data()) % remaining); + } + ptr += written; + remaining -= written; + } + while (remaining > 0); + } + // fd is now closed + + rename_clobberingly(tmp, fname); + +} + // Local Variables: // mode: C++ // fill-column: 76