Re: [PATCH] read-file: add variants that clear internal memory

[Daiki Ueno <ueno@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Bruno Haible <bruno@clisp.org> writes:

> If you agree, I'd like to see two commits:
>   1. the introduction of the flags and RF_BINARY,
>   2. the RF_SENSITIVE flag.
>
> Do you want me to code the first commit, or do you want to do it?

Sure, that would make things much simpler.  I'm attaching a patch along
these lines.

Regards,
-- 
Daiki Ueno

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-read-file-add-flags-to-modify-reading-behavior.patch --]
[-- Type: text/x-patch, Size: 9547 bytes --]

From 5a40a392b3b551cf92d4be022b4efd900f339e39 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
Date: Tue, 26 May 2020 10:22:37 +0200
Subject: [PATCH] read-file: add flags to modify reading behavior

* lib/read-file.h (RF_BINARY, RF_SENSITIVE): New define.
(fread_file, read_file): Take FLAGS argument.
(read_binary_file): Remove.
* lib/read-file.c (internal_fread_file): Take into account of
RF_SENSITIVE flag.
(internal_read_file): Take into account of RF_BINARY and
RF_SENSITIVE flags.
* modules/read-file (Depends-on): Add explicit_bzero.
This adds an alternative behavior of those functions to explicitly
clear the internal memory block when it becomes unused.  This is
useful for reading sensitive information from a file.
---
 ChangeLog               | 15 ++++++++
 lib/read-file.c         | 85 ++++++++++++++++++++++++++++-------------
 lib/read-file.h         | 10 +++--
 modules/read-file       |  1 +
 modules/read-file-tests |  1 +
 tests/test-read-file.c  | 17 +++++++--
 6 files changed, 97 insertions(+), 32 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 07d4d5124..d1ee01a54 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-05-26  Daiki Ueno  <ueno@gnu.org>
+
+	read-file: add flags to modify reading behavior
+	* lib/read-file.h (RF_BINARY, RF_SENSITIVE): New define.
+	(fread_file, read_file): Take FLAGS argument.
+	(read_binary_file): Remove.
+	* lib/read-file.c (internal_fread_file): Take into account of
+	RF_SENSITIVE flag.
+	(internal_read_file): Take into account of RF_BINARY and
+	RF_SENSITIVE flags.
+	* modules/read-file (Depends-on): Add explicit_bzero.
+	This adds an alternative behavior of those functions to explicitly
+	clear the internal memory block when it becomes unused.  This is
+	useful for reading sensitive information from a file.
+
 2020-05-26  Daiki Ueno  <ueno@gnu.org>
 
 	read-file: make use of fopen-gnu
diff --git a/lib/read-file.c b/lib/read-file.c
index 293bc3e8a..d95aae168 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -31,16 +31,17 @@
 /* Get malloc, realloc, free. */
 #include <stdlib.h>
 
+/* Get explicit_bzero, memcpy. */
+#include <string.h>
+
 /* Get errno. */
 #include <errno.h>
 
-/* Read a STREAM and return a newly allocated string with the content,
-   and set *LENGTH to the length of the string.  The string is
-   zero-terminated, but the terminating zero byte is not counted in
-   *LENGTH.  On errors, *LENGTH is undefined, errno preserves the
-   values set by system functions (if any), and NULL is returned.  */
-char *
-fread_file (FILE *stream, size_t *length)
+/* Get assert. */
+#include <assert.h>
+
+static char *
+internal_fread_file (FILE *stream, int flags, size_t *length)
 {
   char *buf = NULL;
   size_t alloc = BUFSIZ;
@@ -94,7 +95,12 @@ fread_file (FILE *stream, size_t *length)
             /* Shrink the allocated memory if possible.  */
             if (size < alloc - 1)
               {
-                char *smaller_buf = realloc (buf, size + 1);
+                char *smaller_buf;
+
+                if (flags & RF_SENSITIVE)
+                  explicit_bzero (buf + size, alloc - size);
+
+                smaller_buf = realloc (buf, size + 1);
                 if (smaller_buf != NULL)
                   buf = smaller_buf;
               }
@@ -106,6 +112,7 @@ fread_file (FILE *stream, size_t *length)
 
         {
           char *new_buf;
+          size_t save_alloc = alloc;
 
           if (alloc == PTRDIFF_MAX)
             {
@@ -118,7 +125,21 @@ fread_file (FILE *stream, size_t *length)
           else
             alloc = PTRDIFF_MAX;
 
-          if (!(new_buf = realloc (buf, alloc)))
+          if (flags & RF_SENSITIVE)
+            {
+              new_buf = malloc (alloc);
+              if (!new_buf)
+                {
+                  /* BUF should be cleared below after the loop.  */
+                  save_errno = errno;
+                  break;
+                }
+              memcpy (new_buf, buf, save_alloc);
+              explicit_bzero (buf, save_alloc);
+              free (buf);
+              buf = new_buf;
+            }
+          else if (!(new_buf = realloc (buf, alloc)))
             {
               save_errno = errno;
               break;
@@ -128,14 +149,32 @@ fread_file (FILE *stream, size_t *length)
         }
       }
 
+    if (flags & RF_SENSITIVE)
+      explicit_bzero (buf, alloc);
+
     free (buf);
     errno = save_errno;
     return NULL;
   }
 }
 
+/* Read a STREAM and return a newly allocated string with the content,
+   and set *LENGTH to the length of the string.  The string is
+   zero-terminated, but the terminating zero byte is not counted in
+   *LENGTH.  On errors, *LENGTH is undefined, errno preserves the
+   values set by system functions (if any), and NULL is returned.
+
+   If the RF_SENSITIVE flag is set in FLAGS, the memory buffer
+   internally allocated will be cleared upon failure.  */
+char *
+fread_file (FILE *stream, int flags, size_t *length)
+{
+  return internal_fread_file (stream, flags, length);
+}
+
 static char *
-internal_read_file (const char *filename, size_t *length, const char *mode)
+internal_read_file (const char *filename, int flags, size_t *length,
+		    const char *mode)
 {
   FILE *stream = fopen (filename, mode);
   char *out;
@@ -144,7 +183,7 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
   if (!stream)
     return NULL;
 
-  out = fread_file (stream, length);
+  out = fread_file (stream, flags, length);
 
   save_errno = errno;
 
@@ -153,6 +192,8 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
       if (out)
         {
           save_errno = errno;
+	  if (flags & RF_SENSITIVE)
+	    explicit_bzero (out, *length);
           free (out);
         }
       errno = save_errno;
@@ -167,22 +208,14 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
    the string.  The string is zero-terminated, but the terminating
    zero byte is not counted in *LENGTH.  On errors, *LENGTH is
    undefined, errno preserves the values set by system functions (if
-   any), and NULL is returned.  */
-char *
-read_file (const char *filename, size_t *length)
-{
-  return internal_read_file (filename, length, "re");
-}
+   any), and NULL is returned.
 
-/* Open (on non-POSIX systems, in binary mode) and read the contents
-   of FILENAME, and return a newly allocated string with the content,
-   and set LENGTH to the length of the string.  The string is
-   zero-terminated, but the terminating zero byte is not counted in
-   the LENGTH variable.  On errors, *LENGTH is undefined, errno
-   preserves the values set by system functions (if any), and NULL is
-   returned.  */
+   If the RF_BINARY flag is set in FLAGS, the file is opened in binary
+   mode.  If the RF_SENSITIVE flag is set in FLAGS, the memory buffer
+   internally allocated will be cleared upon failure.  */
 char *
-read_binary_file (const char *filename, size_t *length)
+read_file (const char *filename, int flags, size_t *length)
 {
-  return internal_read_file (filename, length, "rbe");
+  const char *mode = (flags & RF_BINARY) ? "rbe" : "re";
+  return internal_read_file (filename, flags, length, mode);
 }
diff --git a/lib/read-file.h b/lib/read-file.h
index bb28abd65..c2454ef68 100644
--- a/lib/read-file.h
+++ b/lib/read-file.h
@@ -24,10 +24,14 @@
 /* Get FILE.  */
 #include <stdio.h>
 
-extern char *fread_file (FILE * stream, size_t * length);
+/* Indicate that the file is treated as binary.  */
+#define RF_BINARY 0x1
 
-extern char *read_file (const char *filename, size_t * length);
+/* Indicate that the file content contains sensitive information.  */
+#define RF_SENSITIVE 0x2
 
-extern char *read_binary_file (const char *filename, size_t * length);
+extern char *fread_file (FILE * stream, int flags, size_t * length);
+
+extern char *read_file (const char *filename, int flags, size_t * length);
 
 #endif /* READ_FILE_H */
diff --git a/modules/read-file b/modules/read-file
index a6e7faf0a..5d2be5bbf 100644
--- a/modules/read-file
+++ b/modules/read-file
@@ -7,6 +7,7 @@ lib/read-file.c
 m4/read-file.m4
 
 Depends-on:
+explicit_bzero
 fopen-gnu
 fstat
 ftello
diff --git a/modules/read-file-tests b/modules/read-file-tests
index 361bca806..299631644 100644
--- a/modules/read-file-tests
+++ b/modules/read-file-tests
@@ -1,5 +1,6 @@
 Files:
 tests/test-read-file.c
+tests/macros.h
 
 Depends-on:
 
diff --git a/tests/test-read-file.c b/tests/test-read-file.c
index 930cf4acb..154fb36b1 100644
--- a/tests/test-read-file.c
+++ b/tests/test-read-file.c
@@ -23,11 +23,13 @@
 #include <stdlib.h>
 #include <sys/stat.h>
 
+#include "macros.h"
+
 #define FILE1 "/etc/resolv.conf"
 #define FILE2 "/dev/null"
 
 int
-main (void)
+test_read_file (int flags)
 {
   struct stat statbuf;
   int err = 0;
@@ -37,7 +39,7 @@ main (void)
   if (stat (FILE1, &statbuf) >= 0)
     {
       size_t len;
-      char *out = read_file (FILE1, &len);
+      char *out = read_file (FILE1, flags, &len);
 
       if (!out)
         {
@@ -80,7 +82,7 @@ main (void)
   if (stat (FILE2, &statbuf) >= 0)
     {
       size_t len;
-      char *out = read_file (FILE2, &len);
+      char *out = read_file (FILE2, flags, &len);
 
       if (!out)
         {
@@ -109,3 +111,12 @@ main (void)
 
   return err;
 }
+
+int
+main (void)
+{
+  ASSERT (!test_read_file (0));
+  ASSERT (!test_read_file (RF_BINARY));
+  ASSERT (!test_read_file (RF_SENSITIVE));
+  ASSERT (!test_read_file (RF_BINARY | RF_SENSITIVE));
+}
-- 
2.26.2