Cleanup RAND_load_file,RAND_write_file

Document an internal assumption that these are only for use with files,
and return an error if not. That made the code much simpler.
Leave it as writing 1024 bytes, even though we don't need more than 256
from a security perspective.  But the amount isn't specified, now, so we
can change it later if we want.
Reviewed-by: NRichard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3864)

crypto/err/openssl.txt

@@ -861,6 +861,8 @@ PKCS7_F_PKCS7_SIGN_ADD_SIGNER:137:PKCS7_sign_add_signer
 PKCS7_F_PKCS7_SIMPLE_SMIMECAP:119:PKCS7_simple_smimecap
 PKCS7_F_PKCS7_VERIFY:117:PKCS7_verify
 RAND_F_RAND_BYTES:100:RAND_bytes
+RAND_F_RAND_LOAD_FILE:101:RAND_load_file
+RAND_F_RAND_WRITE_FILE:102:RAND_write_file
 RSA_F_CHECK_PADDING_MD:140:check_padding_md
 RSA_F_ENCODE_PKCS1:146:encode_pkcs1
 RSA_F_INT_RSA_VERIFY:145:int_rsa_verify
@@ -2096,7 +2098,10 @@ PKCS7_R_UNSUPPORTED_CIPHER_TYPE:111:unsupported cipher type
 PKCS7_R_UNSUPPORTED_CONTENT_TYPE:112:unsupported content type
 PKCS7_R_WRONG_CONTENT_TYPE:113:wrong content type
 PKCS7_R_WRONG_PKCS7_TYPE:114:wrong pkcs7 type
+RAND_R_CANNOT_OPEN_FILE:102:Cannot open file
 RAND_R_FUNC_NOT_IMPLEMENTED:101:Function not implemented
+RAND_R_FWRITE_ERROR:103:Error writing file
+RAND_R_NOT_A_REGULAR_FILE:104:Not a regular file
 RAND_R_PRNG_NOT_SEEDED:100:PRNG not seeded
 RSA_R_ALGORITHM_MISMATCH:100:algorithm mismatch
 RSA_R_BAD_E_VALUE:101:bad e value

crypto/rand/rand_err.c

@@ -15,12 +15,18 @@
 
 static const ERR_STRING_DATA RAND_str_functs[] = {
     {ERR_PACK(ERR_LIB_RAND, RAND_F_RAND_BYTES, 0), "RAND_bytes"},
+    {ERR_PACK(ERR_LIB_RAND, RAND_F_RAND_LOAD_FILE, 0), "RAND_load_file"},
+    {ERR_PACK(ERR_LIB_RAND, RAND_F_RAND_WRITE_FILE, 0), "RAND_write_file"},
     {0, NULL}
 };
 
 static const ERR_STRING_DATA RAND_str_reasons[] = {
+    {ERR_PACK(ERR_LIB_RAND, 0, RAND_R_CANNOT_OPEN_FILE), "Cannot open file"},
     {ERR_PACK(ERR_LIB_RAND, 0, RAND_R_FUNC_NOT_IMPLEMENTED),
     "Function not implemented"},
+    {ERR_PACK(ERR_LIB_RAND, 0, RAND_R_FWRITE_ERROR), "Error writing file"},
+    {ERR_PACK(ERR_LIB_RAND, 0, RAND_R_NOT_A_REGULAR_FILE),
+    "Not a regular file"},
     {ERR_PACK(ERR_LIB_RAND, 0, RAND_R_PRNG_NOT_SEEDED), "PRNG not seeded"},
     {0, NULL}
 };

crypto/rand/randfile.c

@@ -27,6 +27,8 @@
 #ifndef OPENSSL_NO_POSIX_IO
 # include <sys/stat.h>
 # include <fcntl.h>
+#endif
+
 /*
  * Following should not be needed, and we could have been stricter
  * and demand S_IS*. But some systems just don't comply... Formally
@@ -34,36 +36,18 @@
  * would look like ((m) & MASK == TYPE), but since MASK availability
  * is as questionable, we settle for this poor-man fallback...
  */
-# if !defined(S_ISBLK)
-#  if defined(_S_IFBLK)
-#   define S_ISBLK(m) ((m) & _S_IFBLK)
-#  elif defined(S_IFBLK)
-#   define S_ISBLK(m) ((m) & S_IFBLK)
-#  elif defined(_WIN32)
-#   define S_ISBLK(m) 0 /* no concept of block devices on Windows */
-#  endif
+# if !defined(S_ISREG)
+#   define S_ISREG(m) ((m) & S_IFREG)
 # endif
-# if !defined(S_ISCHR)
-#  if defined(_S_IFCHR)
-#   define S_ISCHR(m) ((m) & _S_IFCHR)
-#  elif defined(S_IFCHR)
-#   define S_ISCHR(m) ((m) & S_IFCHR)
-#  endif
-# endif
-#endif
 
 #ifdef _WIN32
 # define stat    _stat
 # define chmod   _chmod
 # define open    _open
 # define fdopen  _fdopen
-# define fstat   _fstat
-# define fileno  _fileno
 #endif
 
-#undef BUFSIZE
-#define BUFSIZE 1024
-#define RAND_DATA 1024
+#define RAND_FILE_SIZE 1024
 
 #ifdef OPENSSL_SYS_VMS
 /*
@@ -72,23 +56,12 @@
  * __FILE_ptr32 is a type provided by DEC C headers (types.h specifically)
  * to make sure the FILE* is a 32-bit pointer no matter what.  We know that
  * stdio function return this type (a study of stdio.h proves it).
- * Additionally, we create a similar char pointer type for the sake of
- * vms_setbuf below.
  */
 # if __INITIAL_POINTER_SIZE == 64
 #  pragma pointer_size save
 #  pragma pointer_size 32
 typedef char *char_ptr32;
 #  pragma pointer_size restore
-/*
- * On VMS, setbuf() will only take 32-bit pointers, and a compilation
- * with /POINTER_SIZE=64 will give off a MAYLOSEDATA2 warning here.
- * Since we know that the FILE* really is a 32-bit pointer expanded to
- * 64 bits, we also know it's safe to convert it back to a 32-bit pointer.
- * As for the buffer parameter, we only use NULL here, so that passes as
- * well...
- */
-#  define setbuf(fp,buf) (setbuf)((__FILE_ptr32)(fp), (char_ptr32)(buf))
 # endif
 
 /*
@@ -96,8 +69,9 @@ typedef char *char_ptr32;
  * passing in sharing options being disabled by /STANDARD=ANSI89
  */
 static __FILE_ptr32 (*const vms_fopen)(const char *, const char *, ...) =
-      (__FILE_ptr32 (*)(const char *, const char *, ...))fopen;
-# define VMS_OPEN_ATTRS "shr=get,put,upd,del","ctx=bin,stm","rfm=stm","rat=none","mrs=0"
+        (__FILE_ptr32 (*)(const char *, const char *, ...))fopen;
+# define VMS_OPEN_ATTRS \
+        "shr=get,put,upd,del","ctx=bin,stm","rfm=stm","rat=none","mrs=0"
 
 # define openssl_fopen(fname,mode) vms_fopen((fname), (mode), VMS_OPEN_ATTRS)
 #endif
@@ -106,88 +80,74 @@ static __FILE_ptr32 (*const vms_fopen)(const char *, const char *, ...) =
 
 /*
  * Note that these functions are intended for seed files only. Entropy
- * devices and EGD sockets are handled in rand_unix.c
+ * devices and EGD sockets are handled in rand_unix.c  If |bytes| is
+ * -1 read the complete file; otherwise read the specified amount.
  */
-
 int RAND_load_file(const char *file, long bytes)
 {
-    /*-
-     * If bytes >= 0, read up to 'bytes' bytes.
-     * if bytes == -1, read complete file.
-     */
-
-    unsigned char buf[BUFSIZE];
+    unsigned char buf[RAND_FILE_SIZE];
 #ifndef OPENSSL_NO_POSIX_IO
     struct stat sb;
 #endif
-    int i, ret = 0, n;
-    FILE *in = NULL;
-
-    if (file == NULL)
-        return 0;
+    int i, n, ret = 0;
+    FILE *in;
 
     if (bytes == 0)
-        return ret;
-
-    in = openssl_fopen(file, "rb");
-    if (in == NULL)
-        goto err;
+        return 0;
 
 #ifndef OPENSSL_NO_POSIX_IO
-    /*
-     * struct stat can have padding and unused fields that may not be
-     * initialized in the call to stat(). We need to clear the entire
-     * structure before calling RAND_add() to avoid complaints from
-     * applications such as Valgrind.
-     */
-    memset(&sb, 0, sizeof(sb));
-    if (fstat(fileno(in), &sb) < 0)
-        goto err;
-    RAND_add(&sb, sizeof(sb), 0.0);
-
+    if (stat(file, &sb) < 0 || !S_ISREG(sb.st_mode)) {
+        RANDerr(RAND_F_RAND_LOAD_FILE, RAND_R_NOT_A_REGULAR_FILE);
+        ERR_add_error_data(2, "Filename=", file);
+        return -1;
+    }
 #endif
-    for (;;) {
+    if ((in = openssl_fopen(file, "rb")) == NULL) {
+        RANDerr(RAND_F_RAND_LOAD_FILE, RAND_R_CANNOT_OPEN_FILE);
+        ERR_add_error_data(2, "Filename=", file);
+        return -1;
+    }
+
+    for ( ; ; ) {
         if (bytes > 0)
-            n = (bytes < BUFSIZE) ? (int)bytes : BUFSIZE;
+            n = (bytes < RAND_FILE_SIZE) ? (int)bytes : RAND_FILE_SIZE;
         else
-            n = BUFSIZE;
+            n = RAND_FILE_SIZE;
         i = fread(buf, 1, n, in);
         if (i <= 0)
             break;
-
         RAND_add(buf, i, (double)i);
         ret += i;
-        if (bytes > 0) {
-            bytes -= n;
-            if (bytes <= 0)
-                break;
-        }
+
+        /* If given a bytecount, and we did it, break. */
+        if (bytes > 0 && (bytes -= i) <= 0)
+            break;
     }
-    OPENSSL_cleanse(buf, BUFSIZE);
- err:
-    if (in != NULL)
-        fclose(in);
+
+    OPENSSL_cleanse(buf, sizeof(buf));
+    fclose(in);
     return ret;
 }
 
 int RAND_write_file(const char *file)
 {
-    unsigned char buf[BUFSIZE];
-    int i, ret = 0, rand_err = 0;
+    unsigned char buf[RAND_FILE_SIZE];
+    int ret = -1;
     FILE *out = NULL;
-    int n;
 #ifndef OPENSSL_NO_POSIX_IO
+    struct stat sb;
 
-# if defined(S_ISBLK) && defined(S_ISCHR)
-# ifdef _WIN32
-    /*
-     * Check for |file| being a driver as "ASCII-safe" on Windows,
-     * because driver paths are always ASCII.
-     */
-# endif
-# endif
+    if (stat(file, &sb) >= 0 && !S_ISREG(sb.st_mode)) {
+        RANDerr(RAND_F_RAND_WRITE_FILE, RAND_R_NOT_A_REGULAR_FILE);
+        ERR_add_error_data(2, "Filename=", file);
+        return -1;
+    }
 #endif
 
+    /* Collect enough random data. */
+    if (RAND_bytes(buf, (int)sizeof(buf)) != 1)
+        return  -1;
+
 #if defined(O_CREAT) && !defined(OPENSSL_NO_POSIX_IO) && \
     !defined(OPENSSL_SYS_VMS) && !defined(OPENSSL_SYS_WINDOWS)
     {
@@ -222,66 +182,54 @@ int RAND_write_file(const char *file)
      * application level. Also consider whether or not you NEED a persistent
      * rand file in a concurrent use situation.
      */
-
     out = openssl_fopen(file, "rb+");
 #endif
+
     if (out == NULL)
         out = openssl_fopen(file, "wb");
     if (out == NULL)
-        goto err;
+        return -1;
 
 #if !defined(NO_CHMOD) && !defined(OPENSSL_NO_POSIX_IO)
+    /*
+     * Yes it's late to do this (see above comment), but better than nothing.
+     */
     chmod(file, 0600);
 #endif
-    n = RAND_DATA;
-    for (;;) {
-        i = (n > BUFSIZE) ? BUFSIZE : n;
-        n -= BUFSIZE;
-        if (RAND_bytes(buf, i) <= 0)
-            rand_err = 1;
-        i = fwrite(buf, 1, i, out);
-        if (i <= 0) {
-            ret = 0;
-            break;
-        }
-        ret += i;
-        if (n <= 0)
-            break;
-    }
 
+    ret = fwrite(buf, 1, RAND_FILE_SIZE, out);
     fclose(out);
-    OPENSSL_cleanse(buf, BUFSIZE);
- err:
-    return (rand_err ? -1 : ret);
+    OPENSSL_cleanse(buf, RAND_FILE_SIZE);
+    return ret;
 }
 
 const char *RAND_file_name(char *buf, size_t size)
 {
     char *s = NULL;
+    size_t len;
     int use_randfile = 1;
 
 #if defined(_WIN32) && defined(CP_UTF8)
-    DWORD len;
-    WCHAR *var, *val;
-
-    if ((var = L"RANDFILE",
-         len = GetEnvironmentVariableW(var, NULL, 0)) == 0
-        && (var = L"HOME", use_randfile = 0,
-            len = GetEnvironmentVariableW(var, NULL, 0)) == 0
-        && (var = L"USERPROFILE",
-            len = GetEnvironmentVariableW(var, NULL, 0)) == 0) {
-        var = L"SYSTEMROOT",
-        len = GetEnvironmentVariableW(var, NULL, 0);
+    DWORD envlen;
+    WCHAR *var;
+
+    /* Look up various environment variables. */
+    if ((envlen = GetEnvironmentVariableW(var = L"RANDFILE", NULL, 0)) == 0) {
+        use_randfile = 0;
+        if ((envlen = GetEnvironmentVariableW(var = L"HOME", NULL, 0)) == 0
+                && (envlen = GetEnvironmentVariableW(var = L"USERPROFILE",
+                                                  NULL, 0)) == 0)
+            envlen = GetEnvironmentVariableW(var = L"SYSTEMROOT", NULL, 0);
     }
 
-    if (len != 0) {
+    /* If we got a value, allocate space to hold it and then get it. */
+    if (envlen != 0) {
         int sz;
+        WCHAR *val = _alloca(envlen * sizeof(WCHAR));
 
-        val = _alloca(len * sizeof(WCHAR));
-
-        if (GetEnvironmentVariableW(var, val, len) < len
-            && (sz = WideCharToMultiByte(CP_UTF8, 0, val, -1, NULL, 0,
-                                         NULL, NULL)) != 0) {
+        if (GetEnvironmentVariableW(var, val, envlen) < envlen
+                && (sz = WideCharToMultiByte(CP_UTF8, 0, val, -1, NULL, 0,
+                                             NULL, NULL)) != 0) {
             s = _alloca(sz);
             if (WideCharToMultiByte(CP_UTF8, 0, val, -1, s, sz,
                                     NULL, NULL) == 0)
@@ -291,35 +239,33 @@ const char *RAND_file_name(char *buf, size_t size)
 #else
     if (OPENSSL_issetugid() != 0) {
         use_randfile = 0;
-    } else {
-        s = getenv("RANDFILE");
-        if (s == NULL || *s == '\0') {
-            use_randfile = 0;
-            s = getenv("HOME");
-        }
+    } else if ((s = getenv("RANDFILE")) == NULL || *s == '\0') {
+        use_randfile = 0;
+        s = getenv("HOME");
     }
 #endif
+
 #ifdef DEFAULT_HOME
-    if (!use_randfile && s == NULL) {
+    if (!use_randfile && s == NULL)
         s = DEFAULT_HOME;
-    }
 #endif
-    if (s != NULL && *s) {
-        size_t len = strlen(s);
-
-        if (use_randfile && len + 1 < size) {
-            if (OPENSSL_strlcpy(buf, s, size) >= size)
-                return NULL;
-        } else if (len + strlen(RFILE) + 2 < size) {
-            OPENSSL_strlcpy(buf, s, size);
+    if (s == NULL || *s == '\0')
+        return NULL;
+
+    len = strlen(s);
+    if (use_randfile) {
+        if (len + 1 >= size)
+            return NULL;
+        strcpy(buf, s);
+    } else {
+        if (len + 1 + strlen(RFILE) + 1 >= size)
+            return NULL;
+        strcpy(buf, s);
 #ifndef OPENSSL_SYS_VMS
-            OPENSSL_strlcat(buf, "/", size);
+        strcat(buf, "/");
 #endif
-            OPENSSL_strlcat(buf, RFILE, size);
-        }
-    } else {
-        buf[0] = '\0';      /* no file name */
+        strcat(buf, RFILE);
     }
 
-    return buf[0] ? buf : NULL;
+    return buf;
 }

doc/man3/RAND_load_file.pod

@@ -8,51 +8,50 @@ RAND_load_file, RAND_write_file, RAND_file_name - PRNG seed file
 
  #include <openssl/rand.h>
 
- const char *RAND_file_name(char *buf, size_t num);
-
  int RAND_load_file(const char *filename, long max_bytes);
 
  int RAND_write_file(const char *filename);
 
+ const char *RAND_file_name(char *buf, size_t num);
+
 =head1 DESCRIPTION
 
+RAND_load_file() reads a number of bytes from file B<filename> and
+adds them to the PRNG. If B<max_bytes> is non-negative,
+up to B<max_bytes> are read;
+if B<max_bytes> is -1, the complete file is read.
+
+RAND_write_file() writes a number of random bytes (currently 256) to
+file B<filename> which can be used to initialize the PRNG by calling
+RAND_load_file() in a later session.
+
 RAND_file_name() generates a default path for the random seed
 file. B<buf> points to a buffer of size B<num> in which to store the
 filename.
 
 On all systems, if the environment variable B<RANDFILE> is set, its
 value will be used as the seed file name.
-
-Otherwise, the file is called ".rnd", found in platform dependent locations:
+Otherwise, the file is called C<.rnd>, found in platform dependent locations:
 
 =over 4
 
 =item On Windows (in order of preference)
 
-%HOME%, %USERPROFILE%, %SYSTEMROOT%, C:\
+ %HOME%, %USERPROFILE%, %SYSTEMROOT%, C:\
 
 =item On VMS
 
-SYS$LOGIN:
+ SYS$LOGIN:
 
 =item On all other systems
 
-$HOME
+ $HOME
 
 =back
 
 If C<$HOME> (on non-Windows and non-VMS system) is not set either, or
 B<num> is too small for the path name, an error occurs.
 
-RAND_load_file() reads a number of bytes from file B<filename> and
-adds them to the PRNG. If B<max_bytes> is non-negative,
-up to B<max_bytes> are read;
-if B<max_bytes> is -1, the complete file is read.
-
-RAND_write_file() writes a number of random bytes (currently 1024) to
-file B<filename> which can be used to initialize the PRNG by calling
-RAND_load_file() in a later session.
-
 =head1 RETURN VALUES
 
 RAND_load_file() returns the number of bytes read.
@@ -67,6 +66,13 @@ error.
 
 L<RAND_bytes(3)>, L<RAND_add(3)>, L<RAND_cleanup(3)>
 
+=head1 HISTORY
+
+A comment in the source since at least OpenSSL version 1.0.2 said that
+RAND_load_file() and RAND_write_file() were only intended for regular files,
+and not really device special files such as C</dev/random>.  This was
+poorly enforced before OpenSSL version 1.1.1.
+
 =head1 COPYRIGHT
 
 Copyright 2000-2016 The OpenSSL Project Authors. All Rights Reserved.

include/openssl/randerr.h

@@ -23,11 +23,16 @@ int ERR_load_RAND_strings(void);
  * RAND function codes.
  */
 # define RAND_F_RAND_BYTES                                100
+# define RAND_F_RAND_LOAD_FILE                            101
+# define RAND_F_RAND_WRITE_FILE                           102
 
 /*
  * RAND reason codes.
  */
+# define RAND_R_CANNOT_OPEN_FILE                          102
 # define RAND_R_FUNC_NOT_IMPLEMENTED                      101
+# define RAND_R_FWRITE_ERROR                              103
+# define RAND_R_NOT_A_REGULAR_FILE                        104
 # define RAND_R_PRNG_NOT_SEEDED                           100
 
 #endif