[PATCH] gensignkey: ensure host keys are flushed to disk

[DELOGET, Emmanuel]

Hello, 

These are only comments - I'm not Matt :)

Le mardi 28 octobre 2014 à 14:11 +0100, Peter Korsgaard a écrit :
> From: Peter Korsgaard <peter at korsgaard.com>
> 
> 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 <peter at korsgaard.com>
> ---
>  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