[Openvpn-devel,v4,1/7] Introduce buffer_write_file()

Message ID 1533224779-13216-1-git-send-email-steffan.karger@fox-it.com
State Superseded
Headers show
Series [Openvpn-devel,v4,1/7] Introduce buffer_write_file() | expand

Commit Message

Steffan Karger Aug. 2, 2018, 5:46 a.m. UTC
Rewrite buf_write_string_file to buffer_write_file, which is simpler to
use and can deal with not-null-terminated strings.  Mostly implemented so
this can be easily reused for tls-crypt-v2 (client) key files.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
v3: split change out of "generate client key", reuse in write_key_file()
v4: improve doxygen and error handling

 src/openvpn/buffer.c | 31 ++++++++++++++++++++++++-------
 src/openvpn/buffer.h | 12 ++++++++----
 src/openvpn/crypto.c | 33 ++++++---------------------------
 src/openvpn/crypto.h |  5 +++++
 src/openvpn/init.c   |  4 ++++
 5 files changed, 47 insertions(+), 38 deletions(-)

Comments

Antonio Quartulli Aug. 2, 2018, 11:49 p.m. UTC | #1
Hi,

On 02/08/18 23:46, Steffan Karger wrote:
> Rewrite buf_write_string_file to buffer_write_file, which is simpler to
> use and can deal with not-null-terminated strings.  Mostly implemented so
> this can be easily reused for tls-crypt-v2 (client) key files.
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
> ---
> v3: split change out of "generate client key", reuse in write_key_file()
> v4: improve doxygen and error handling
> 
>  src/openvpn/buffer.c | 31 ++++++++++++++++++++++++-------
>  src/openvpn/buffer.h | 12 ++++++++----
>  src/openvpn/crypto.c | 33 ++++++---------------------------
>  src/openvpn/crypto.h |  5 +++++
>  src/openvpn/init.c   |  4 ++++
>  5 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index 0972139..7f540d7 100644
> --- a/src/openvpn/buffer.c
> +++ b/src/openvpn/buffer.c
> @@ -343,16 +343,33 @@ convert_to_one_line(struct buffer *buf)
>      }
>  }
>  
> -/* NOTE: requires that string be null terminated */
> -void
> -buf_write_string_file(const struct buffer *buf, const char *filename, int fd)
> +bool
> +buffer_write_file(const char *filename, const struct buffer *buf)
>  {
> -    const int len = strlen((char *) BPTR(buf));
> -    const int size = write(fd, BPTR(buf), len);
> -    if (size != len)
> +    bool ret = false;
> +    int fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY,
> +                           S_IRUSR | S_IWUSR);
> +    if (fd == -1)
> +    {
> +        msg(M_ERRNO, "Cannot open file '%s' for write", filename);
> +        goto cleanup;

While running some tests I realized that a failure in open() will cause
a double error because close() is performed with fd = -1. The result is:

$ ./src/openvpn/openvpn --genkey --secret /root/test.key
Fri Aug  3 17:35:46 2018 Cannot open file '/root/test.key' for write:
Permission denied (errno=13)
Fri Aug  3 17:35:46 2018 Close error on file /root/test.key: Bad file
descriptor (errno=9)
Fri Aug  3 17:35:46 2018 Failed to write key file
Fri Aug  3 17:35:46 2018 Exiting due to fatal error

As you can see we have two distinct errors, with the second being caused
by the first one.

I think this is one of those cases where in case of failure you just
return false rather than jumping to cleanup, because nothing has been
initialized yet.

The double error is harmless, but not very clean, especially because, no
matter why open() failed, errno will always be changed to EBADF.

(I think this also means that you can get rid of the label at all and
slightly restyle the code)


Cheers,

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 0972139..7f540d7 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -343,16 +343,33 @@  convert_to_one_line(struct buffer *buf)
     }
 }
 
-/* NOTE: requires that string be null terminated */
-void
-buf_write_string_file(const struct buffer *buf, const char *filename, int fd)
+bool
+buffer_write_file(const char *filename, const struct buffer *buf)
 {
-    const int len = strlen((char *) BPTR(buf));
-    const int size = write(fd, BPTR(buf), len);
-    if (size != len)
+    bool ret = false;
+    int fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY,
+                           S_IRUSR | S_IWUSR);
+    if (fd == -1)
+    {
+        msg(M_ERRNO, "Cannot open file '%s' for write", filename);
+        goto cleanup;
+    }
+
+    const int size = write(fd, BPTR(buf), BLEN(buf));
+    if (size != BLEN(buf))
     {
-        msg(M_ERR, "Write error on file '%s'", filename);
+        msg(M_ERRNO, "Write error on file '%s'", filename);
+        goto cleanup;
+    }
+
+    ret = true;
+cleanup:
+    if (close(fd) < 0)
+    {
+        msg(M_ERRNO, "Close error on file %s", filename);
+        ret = false;
     }
+    return ret;
 }
 
 /*
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index 6b6025e..704129a 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -469,11 +469,15 @@  const char *skip_leading_whitespace(const char *str);
 
 void string_null_terminate(char *str, int len, int capacity);
 
-/*
- * Write string in buf to file descriptor fd.
- * NOTE: requires that string be null terminated.
+/**
+ * Write buffer contents to file.
+ *
+ * @param filename  The filename to write the buffer to.
+ * @param buf       Thu buffer to write to the file.
+ *
+ * @return true on success, false otherwise.
  */
-void buf_write_string_file(const struct buffer *buf, const char *filename, int fd);
+bool buffer_write_file(const char *filename, const struct buffer *buf);
 
 /*
  * write a string to the end of a buffer that was
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 5381ef2..c8d95f3 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1418,36 +1418,24 @@  read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
     gc_free(&gc);
 }
 
-/*
- * Write key to file, return number of random bits
- * written.
- */
 int
 write_key_file(const int nkeys, const char *filename)
 {
     struct gc_arena gc = gc_new();
 
-    int fd, i;
-    int nbits = 0;
+    int nbits = nkeys * sizeof(struct key) * 8;
 
     /* must be large enough to hold full key file */
     struct buffer out = alloc_buf_gc(2048, &gc);
-    struct buffer nbits_head_text = alloc_buf_gc(128, &gc);
 
     /* how to format the ascii file representation of key */
     const int bytes_per_line = 16;
 
-    /* open key file */
-    fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY, S_IRUSR | S_IWUSR);
-
-    if (fd == -1)
-    {
-        msg(M_ERR, "Cannot open shared secret file '%s' for write", filename);
-    }
-
+    /* write header */
+    buf_printf(&out, "#\n# %d bit OpenVPN static key\n#\n", nbits);
     buf_printf(&out, "%s\n", static_key_head);
 
-    for (i = 0; i < nkeys; ++i)
+    for (int i = 0; i < nkeys; ++i)
     {
         struct key key;
         char *fmt;
@@ -1463,9 +1451,6 @@  write_key_file(const int nkeys, const char *filename)
                             "\n",
                             &gc);
 
-        /* increment random bits counter */
-        nbits += sizeof(key) * 8;
-
         /* write to holding buffer */
         buf_printf(&out, "%s\n", fmt);
 
@@ -1476,16 +1461,10 @@  write_key_file(const int nkeys, const char *filename)
 
     buf_printf(&out, "%s\n", static_key_foot);
 
-    /* write number of bits */
-    buf_printf(&nbits_head_text, "#\n# %d bit OpenVPN static key\n#\n", nbits);
-    buf_write_string_file(&nbits_head_text, filename, fd);
-
     /* write key file, now formatted in out, to file */
-    buf_write_string_file(&out, filename, fd);
-
-    if (close(fd))
+    if(!buffer_write_file(filename, &out))
     {
-        msg(M_ERR, "Close error on shared secret file %s", filename);
+        nbits = -1;
     }
 
     /* zero memory which held file content (memory will be freed by GC) */
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 9417fa1..0dc597f 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -271,6 +271,11 @@  struct crypto_options
 #define RKF_INLINE       (1<<1)
 void read_key_file(struct key2 *key2, const char *file, const unsigned int flags);
 
+/**
+ * Write nkeys 1024-bits keys to file.
+ *
+ * @returns number of random bits written, or -1 on failure.
+ */
 int write_key_file(const int nkeys, const char *filename);
 
 int read_passphrase_hash(const char *passphrase_file,
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index f432106..2933d55 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1041,6 +1041,10 @@  do_genkey(const struct options *options)
         }
 
         nbits_written = write_key_file(2, options->shared_secret_file);
+        if (nbits_written < 0)
+        {
+            msg(M_FATAL, "Failed to write key file");
+        }
 
         msg(D_GENKEY | M_NOPREFIX,
             "Randomly generated %d bit key written to %s", nbits_written,