Check that the path passed in to copy_file_secure_dest is a file

[BradWhittington]

This is a copy-pasta proposed fix for the issue I logged in #90

If the path passed into copy_file_secure_dest is a valid, readable directory, it will currently proceed without error, and create a zero byte file in the destination path.

[jk-ozlabs]

This may have a TOCTOU issue; can we fstat instead?

[BradWhittington]

I'm not a c expert... this is my best attempt to try solve the problem, using stackoverflow suggestions...

[vincele]

I guess something like using fstat(source_handle, &statbuf); (maybe @ line 62) after the file has been opened sucessfully @ line 56...

(after reading the linked issue) : But maybe checking that S_ISREG() is the wrong check. What's wrong with sockets or pipes, etc... ?

If the error you want to fix is the file being a directory, just check that...

[jk-ozlabs]

Ack on the fstat point.

I think requiring a regular file (viua S_ISREG is reasonable here; we're parsing config files (from external filesystems), so reading from a socket/pipe is probably also a sign of something going wrong.

[jk-ozlabs]

Just providing a little more context here. I'd suggest something like:

commit 28d05a28b63a3413334a24620de9a9e58d539342 (HEAD -> file-dest)
Author: Brad Whittington <[email protected]>
Date:   Sat Mar 5 19:05:35 2022 +0200

    Check that the path passed in to copy_file_secure_dest is actually a file
    
    This is a copy-pasta proposed fix for the issue I logged in #90
    
    [changed to fstat(), and properly check fstat return value,
    via Jeremy Kerr <[email protected]>]
    
    Signed-off-by: Jeremy Kerr <[email protected]>

diff --git a/lib/file/file.c b/lib/file/file.c
index 6028005..5a7da0b 100644
--- a/lib/file/file.c
+++ b/lib/file/file.c
@@ -41,6 +41,7 @@ int copy_file_secure_dest(void *ctx, const char *source_file,
        char template[] = "/tmp/petitbootXXXXXX";
        FILE *destination_handle, *source_handle;
        int destination_fd, result = 0;
+       struct stat statbuf = { 0 };
        unsigned char *buffer;
        ssize_t r;
        size_t l1;
@@ -52,6 +53,21 @@ int copy_file_secure_dest(void *ctx, const char *source_file,
                        return -1;
        }
 
+       result = fstat(fileno(source_handle), &statbuf);
+       if (result) {
+               pb_log("%s: unable to stat source file '%s': %m\n",
+                      __func__, source_file);
+               fclose(source_handle);
+               return -1;
+       }
+
+       if (!S_ISREG(statbuf.st_mode)) {
+               pb_log("%s: source filename '%s' is not a regular file\n",
+                      __func__, source_file);
+               fclose(source_handle);
+               return -1;
+       }
+
        destination_fd = mkstemp(template);
        if (destination_fd < 0) {
                pb_log_fn("unable to create temp file, %m\n");

If that's OK, could you include your Signed-off-by here? Or if not, feel free to make changes.