Handle opening the same file

We introduced a regression in 4.0.1 when closing the TOCTOU issue. We
incorrectly refuse to open the same file twice to save both a CA and
cert into the same location.

This changes the opening logic to always open for  both reading  and
writing to check if it is zero-length.

Also switches the EC curve tests to output the CA and service cert to a
common path to ensure we are testing this case properly.

Updates some incorrect expected behaviors in the test_dhparams_creation
test as well.

Fixes: #99

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Author: sgallagher

src/io_utils.c

@@ -37,6 +37,7 @@
 #include <string.h>
 #include <talloc.h>
 #include <sys/stat.h>
+#include <unistd.h>
 
 #include "config.h"
 #ifdef HAVE_GETTEXT
@@ -102,17 +103,85 @@ static int
 sscg_io_utils_open_file (const char *path, bool overwrite, FILE **fp)
 {
   FILE *_fp = NULL;
-  if (overwrite)
-    _fp = fopen (path, "w");
-  else
-    _fp = fopen (path, "wx");
+  struct stat st;
+  int ret;
+  int fd;
+
+  /* Try to open with r+ mode (file must exist) */
+  _fp = fopen (path, "r+");
   if (!_fp)
     {
-      SSCG_ERROR ("Could not open file %s: %s\n", path, strerror (errno));
-      return errno;
+      /* If file doesn't exist, create it with w+ mode */
+      if (errno == ENOENT)
+        {
+          _fp = fopen (path, "w+");
+          if (!_fp)
+            {
+              SSCG_ERROR (
+                "Could not create file %s: %s\n", path, strerror (errno));
+              return errno;
+            }
+          /* New file has size 0, so we can proceed */
+          ret = EOK;
+          goto done;
+        }
+      else
+        {
+          SSCG_ERROR ("Could not open file %s: %s\n", path, strerror (errno));
+          ret = errno;
+          goto done;
+        }
+    }
+
+  /* File exists and was opened successfully, check its size */
+  fd = fileno (_fp);
+  ret = fstat (fd, &st);
+  if (ret != 0)
+    {
+      SSCG_ERROR ("Could not stat file %s: %s\n", path, strerror (errno));
+      ret = errno;
+      goto done;
     }
-  *fp = _fp;
-  return EOK;
+
+  /* Check if file size is greater than zero */
+  if (st.st_size > 0)
+    {
+      if (overwrite)
+        {
+          /* Truncate the file */
+          ret = ftruncate (fd, 0);
+          if (ret != 0)
+            {
+              SSCG_ERROR (
+                "Could not truncate file %s: %s\n", path, strerror (errno));
+              ret = errno;
+              goto done;
+            }
+          /* Rewind to beginning after truncation */
+          rewind (_fp);
+        }
+      else
+        {
+          /* File exists and overwrite is false - return error */
+          SSCG_ERROR ("File %s already exists\n", path);
+          ret = EEXIST;
+          goto done;
+        }
+    }
+
+  /* File size is zero or has been truncated */
+  ret = EOK;
+
+done:
+  if (ret == EOK)
+    {
+      *fp = _fp;
+      _fp = NULL;
+    }
+
+  if (_fp)
+    fclose (_fp);
+  return ret;
 }
 
 

src/sscg.c

@@ -75,10 +75,11 @@ main (int argc, const char **argv)
 
   struct sscg_stream *stream = NULL;
 
-  /* Always use umask 0577 for generating certificates and keys
-       This means that it's opened as write-only by the effective
-       user. */
-  umask (0577);
+  /* Always use umask 0177 for generating certificates and keys
+     This means that it's opened as read/write only by the effective
+     user.
+  */
+  umask (0177);
 
 #ifdef HAVE_GETTEXT
   /* Initialize internationalization */

test/test_dhparams_creation.sh

@@ -54,6 +54,7 @@ set -e
 DHPARAMS_TMPDIR=$(mktemp --directory --tmpdir=$GITHUB_WORKSPACE sscg_dhparams_test_XXXXXX)
 WRITABLE_DIR="$DHPARAMS_TMPDIR/writable"
 READONLY_DIR="$DHPARAMS_TMPDIR/readonly"
+READONLY_WITH_DHPARAMS_DIR="$DHPARAMS_TMPDIR/readonly_with_dhparams"
 DHPARAMS_DIR="$DHPARAMS_TMPDIR/dhparams"
 
 function cleanup {
@@ -69,14 +70,16 @@ trap cleanup EXIT
 # Set up test directories
 mkdir -p "$WRITABLE_DIR"
 mkdir -p "$READONLY_DIR"
+mkdir -p "$READONLY_WITH_DHPARAMS_DIR"
 mkdir -p "$DHPARAMS_DIR"
 
 # Create a pre-existing dhparams.pem file for some tests
-touch "$DHPARAMS_DIR/dhparams.pem"
+echo "preexisting dhparams.pem" > "$DHPARAMS_DIR/dhparams.pem"
 
 # Copy pre-existing dhparams.pem to readonly directory before making it readonly
-cp "$DHPARAMS_DIR/dhparams.pem" "$READONLY_DIR/dhparams.pem"
-chmod 555 "$READONLY_DIR"
+cp "$DHPARAMS_DIR/dhparams.pem" "$READONLY_WITH_DHPARAMS_DIR/dhparams.pem"
+chmod 0555 "$READONLY_DIR"
+chmod 0555 "$READONLY_WITH_DHPARAMS_DIR"
 
 failed_tests=0
 total_tests=8
@@ -260,9 +263,9 @@ run_test \
     8 \
     "--dhparams-file to non-writable path, existing file" \
     "$WRITABLE_DIR" \
-    "$READONLY_DIR/dhparams.pem" \
+    "$READONLY_WITH_DHPARAMS_DIR/dhparams.pem" \
     17 \
-    "$READONLY_DIR/dhparams.pem" \
+    "$READONLY_WITH_DHPARAMS_DIR/dhparams.pem" \
     "false" \
     "$WRITABLE_DIR"
 

test/test_ecdsa_cert_validity.sh

@@ -185,18 +185,18 @@ pushd "$TMPDIR"
 						 --key-type ecdsa \
 						 --ec-curve "$_arg_ec_curve" \
 						 --cert-key-password mypassword \
-						 --dhparams-file "dhparams.pem"
+						 --dhparams-file "dhparams.pem" \
+						 --ca-file "combined.crt" \
+						 --cert-file "combined.crt"
 
 # Verify that the expected files were created
 test -e dhparams.pem
-test -e ca.crt
-test -e service.pem
+test -e combined.crt
 test -e service-key.pem
 
 # Verify that they have the correct file format
 openssl dhparam -noout -in dhparams.pem
-openssl x509    -noout -in ca.crt
-openssl x509    -noout -in service.pem
+openssl x509    -noout -in combined.crt
 
 grep "ENCRYPTED PRIVATE KEY" service-key.pem
 openssl pkey    -noout -in service-key.pem -passin pass:mypassword
@@ -210,7 +210,7 @@ curve_info=$(openssl pkey -text -noout -in service-key.pem -passin pass:mypasswo
 echo "$curve_info" | grep "$_arg_ec_curve"
 
 # Validate the certificates
-openssl verify -x509_strict -CAfile ca.crt service.pem
+openssl verify -x509_strict -CAfile combined.crt combined.crt
 
 popd # $TMPDIR