Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check that the path passed in to copy_file_secure_dest is a file #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Check that the path passed in to copy_file_secure_dest is a file #91

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/file/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ int copy_file_secure_dest(void *ctx, const char *source_file,
ssize_t r;
size_t l1;

struct stat statbuf;
stat(source_file, &statbuf);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@vincele vincele Dec 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!S_ISREG(statbuf.st_mode)) {
pb_log("%s: unable to stat source file '%s': %m\n",
__func__, source_file);
return -1;
}

source_handle = fopen(source_file, "r");
if (!source_handle) {
pb_log("%s: unable to open source file '%s': %m\n",
Expand Down