Skip to content

Commit

Permalink
stat() files before open()ing them. This should help with issues such…
Browse files Browse the repository at this point in the history 
… as opening /dev/watchdog (#745)
  • Loading branch information
@ggreer
ggreer committed Jan 27, 2017
1 parent 7110453 commit 8639866
Showing 1 changed file with 9 additions and 9 deletions.
18 changes: 9 additions & 9 deletions src/search.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,21 +239,14 @@ void search_stream(FILE *stream, const char *path) {
}

void search_file(const char *file_full_path) {
int fd;
int fd = -1;
off_t f_len = 0;
char *buf = NULL;
struct stat statbuf;
int rv = 0;
FILE *fp = NULL;

fd = open(file_full_path, O_RDONLY);

This comment has been minimized.

Sign in to view

Copy link
@jan-ruzicka

jan-ruzicka Jan 30, 2017

Contributor

How much paranoid does your program need to be?
There may be a case for testing the stat of the file before AND the fstat after opening, to prevent TOCTOU issues.
The use case given is a hostile environment where files can be changed by the attacker after you a check the stat of the file name and before you open (e.g. a quick substitute of a link for a regular file).
if (fd < 0) {
/* XXXX: strerror is not thread-safe */
log_err("Skipping %s: Error opening file: %s", file_full_path, strerror(errno));
goto cleanup;
}

rv = fstat(fd, &statbuf);
rv = stat(file_full_path, &statbuf);
if (rv != 0) {
log_err("Skipping %s: Error fstat()ing file.", file_full_path);
goto cleanup;
Expand All @@ -269,6 +262,13 @@ void search_file(const char *file_full_path) {
goto cleanup;
}

fd = open(file_full_path, O_RDONLY);
if (fd < 0) {
/* XXXX: strerror is not thread-safe */
log_err("Skipping %s: Error opening file: %s", file_full_path, strerror(errno));
goto cleanup;
}

print_init_context();

if (statbuf.st_mode & S_IFIFO) {
Expand Down

1 comment on commit 8639866

@jan-ruzicka
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the stat information test before open be more stringent?
Reading the stat(2) :

       The following mask values are defined for the file type of the st_mode field:

           S_IFMT     0170000   bit mask for the file type bit field

           S_IFSOCK   0140000   socket
           S_IFLNK    0120000   symbolic link
           S_IFREG    0100000   regular file
           S_IFBLK    0060000   block device
           S_IFDIR    0040000   directory
           S_IFCHR    0020000   character device
           S_IFIFO    0010000   FIFO

       Thus, to test for a regular file (for example), one could write:

           stat(pathname, &sb);
           if ((sb.st_mode & S_IFMT) == S_IFREG) {
               /* Handle regular file */
           }

Please sign in to comment.