Hello, These are only comments - I'm not Matt :)
Le mardi 28 octobre 2014 à 14:11 +0100, Peter Korsgaard a écrit : > From: Peter Korsgaard <[email protected]> > > Otherwise we can end up with an empty host key, breaking logins. > > E.G.: > > Run dropbear -R and pull power before the host key is writting to disk. > After reboot we have: > > ls -l /etc/dropbear/ > -rw------- 1 root root 0 Oct 28 10:41 dropbear_ecdsa_host_key > > Which dropbear will try to read and fail: > > [599] Oct 28 10:43:43 Early exit: Bad buf_getptr > > Signed-off-by: Peter Korsgaard <[email protected]> > --- > gensignkey.c | 1 + > svr-kex.c | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/gensignkey.c b/gensignkey.c > index 338bbef..06fdfd3 100644 > --- a/gensignkey.c > +++ b/gensignkey.c > @@ -41,6 +41,7 @@ static int buf_writefile(buffer * buf, const char * > filename) { > > out: > if (fd >= 0) { > + fsync(fd); Instead of fsync, I think it sould be better to open the file with O_SYNC on the systems that allows it (and add a fsync fallback on the systems where O_SYNC is not available). While it will degrade the performance a bit (since the file is quite small, I guess that adding O_SYNC would not be that bad), it will ensure that the write will not return until the data + metadata is present on disk - meaning that we'll avoid race conditions that could occur between the write and the fsync operation. > m_close(fd); > } > return ret; > diff --git a/svr-kex.c b/svr-kex.c > index 337c377..8e76489 100644 > --- a/svr-kex.c > +++ b/svr-kex.c > @@ -89,6 +89,7 @@ static void svr_ensure_hostkey() { > > const char* fn = NULL; > char *fn_temp = NULL; > + char *fn_dir = NULL; > enum signkey_type type = ses.newkeys->algo_hostkey; > void **hostkey = signkey_key_ptr(svr_opts.hostkey, type); > int ret = DROPBEAR_FAILURE; > @@ -142,6 +143,22 @@ static void svr_ensure_hostkey() { > } > } > > +#ifdef HAVE_LIBGEN_H > + /* ensure directory update is flushed to disk */ > + fn_dir = strdup(fn); > + if (fn_dir != NULL) { if (fn_dir) ? Checking for NULL is not necessar - the C standard says that we have a known path to promote pointers to boolean ( Anyway, it sound weird to not do a check if a malloc failed (which is what is tested here) - thus I see 3 possibilities: * stick with this solution * use a PATH_MAX on-stack variable to store a copy of fn * return an error if strdup() (or malloc) fails. I'm not sure there is one good answer to this specific point. > + char *dir = dirname(fn_dir); > + int dirfd = open(dir, O_RDONLY); The O_DIRECTORY flag is missing (at least on Linux) to make sure that the pathname is indeed a directory (it /might/ be a file). > + > + if (dirfd != -1) { > + fsync(dirfd); O_RDONLY file descriptors can be fsysnc only on Linux (that's a well documented POSIX conformance issue). We should make sure that the fd is writable before we fsync it. > + close(dirfd); > + } > + > + free(fn_dir); > + } > +#endif > + > ret = readhostkey(fn, svr_opts.hostkey, &type); > > if (ret == DROPBEAR_SUCCESS) { Best regards, -- Emmanuel Deloget
