Skip to content

Commit

Permalink
proc/linux: Don't open mmapped files before checking stat results (#2002
Browse files Browse the repository at this point in the history 
)

Instead of checking the filesystem containing the mapped file against
specific magic device numbers, we use stat(2) before actually opening
the file. (The open call can block on certain device nodes as reported
in #1929.)

To overcome the possible TOCTOU issue between stat() and open(), we
double-check device, inode after we have opened the file.

This also reverts commit 50eeef4.

The original problem could also have been fixed by passing O_PATH to
open(2). However, there seems to be no way to atomically upgrade those
path-only fds to real file descriptors that we can mmap or read from,
so this would not helped in overcoming the TOCTOU issue.
  • Loading branch information
@hillu
hillu authored Nov 7, 2023
1 parent 50eeef4 commit 2967e86
Showing 1 changed file with 19 additions and 11 deletions.
30 changes: 19 additions & 11 deletions libyara/proc/linux.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,19 +161,13 @@ YR_API const uint8_t* yr_process_fetch_memory_block_data(YR_MEMORY_BLOCK* block)

// Only try mapping the file if it has a path and belongs to a device
if (strlen(proc_info->map_path) > 0 &&
!(proc_info->map_dmaj == 0 && (proc_info->map_dmin == 0 || proc_info->map_dmin == 5)))
!(proc_info->map_dmaj == 0 && proc_info->map_dmin == 0))
{
struct stat st;
fd = open(proc_info->map_path, O_RDONLY);

if (fd < 0)
{
fd = -1; // File does not exist.
}
else if (fstat(fd, &st) < 0)
if (stat(proc_info->map_path, &st) < 0)
{
// Why should stat fail after file open? Treat like missing.
close(fd);
fd = -1;
}
else if (
Expand All @@ -182,22 +176,36 @@ YR_API const uint8_t* yr_process_fetch_memory_block_data(YR_MEMORY_BLOCK* block)
(st.st_ino != proc_info->map_ino))
{
// Wrong file, may have been replaced. Treat like missing.
close(fd);
fd = -1;
}
else if (st.st_size < proc_info->map_offset + block->size)
{
// Mapping extends past end of file. Treat like missing.
close(fd);
fd = -1;
}
else if ((st.st_mode & S_IFMT) != S_IFREG)
{
// Correct filesystem object, but not a regular file. Treat like
// uninitialized mapping.
close(fd);
fd = -2;
}
else
{
fd = open(proc_info->map_path, O_RDONLY);
// Double-check against race conditions
struct stat st2;
if (fstat(fd, &st2) < 0)
{
close(fd);
fd = -1;
}
else if ((st.st_dev != st2.st_dev) || (st.st_ino != st2.st_ino))
{
// File has been changed from under us, so ignore.
close(fd);
fd = -1;
}
}
}

if (fd >= 0)
Expand Down

1 comment on commit 2967e86

@plusvic
Copy link
Member

@plusvic plusvic commented on 2967e86 Nov 8, 2023

Choose a reason for hiding this comment

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

Coverity started raising two new warnings, one about the TOCTOU issue, and the other about fstat receiving a potentially negative fd. I copy the whole report here:

5YR_API const uint8_t* yr_process_fetch_memory_block_data(YR_MEMORY_BLOCK* block)
146 {
147  const uint8_t* result = NULL;
148  uint64_t* pagemap = NULL;
149
150  YR_PROC_ITERATOR_CTX* context = (YR_PROC_ITERATOR_CTX*) block->context;
151  YR_PROC_INFO* proc_info = (YR_PROC_INFO*) context->proc_info;
152
   	1. Condition context->buffer != NULL, taking true branch.
153  if (context->buffer != NULL)
154  {
155    munmap((void*) context->buffer, context->buffer_size);
156    context->buffer = NULL;
157    context->buffer_size = 0;
158  }
159
160  int fd = -2;  // Assume mapping not connected with a file.
161
162  // Only try mapping the file if it has a path and belongs to a device
   	2. Condition strlen(proc_info->map_path) > 0, taking true branch.
   	3. Condition proc_info->map_dmaj == 0, taking false branch.
163  if (strlen(proc_info->map_path) > 0 &&
164      !(proc_info->map_dmaj == 0 && proc_info->map_dmin == 0))
165  {
166    struct stat st;
167
   	
CID 330534 (#1 of 1): Time of check time of use (TOCTOU)
4. fs_check_call: Calling function stat to perform check on proc_info->map_path.
   	5. Condition stat(proc_info->map_path, &st) < 0, taking false branch.
168    if (stat(proc_info->map_path, &st) < 0)
169    {
170      // Why should stat fail after file open? Treat like missing.
171      fd = -1;
172    }
   	6. Condition gnu_dev_major(st.st_dev) != proc_info->map_dmaj, taking false branch.
   	7. Condition gnu_dev_minor(st.st_dev) != proc_info->map_dmin, taking false branch.
   	8. Condition st.st_ino != proc_info->map_ino, taking false branch.
173    else if (
174        (major(st.st_dev) != proc_info->map_dmaj) ||
175        (minor(st.st_dev) != proc_info->map_dmin) ||
176        (st.st_ino != proc_info->map_ino))
177    {
178      // Wrong file, may have been replaced. Treat like missing.
179      fd = -1;
180    }
   	9. Condition st.st_size < proc_info->map_offset + block->size, taking false branch.
181    else if (st.st_size < proc_info->map_offset + block->size)
182    {
183      // Mapping extends past end of file. Treat like missing.
184      fd = -1;
185    }
   	10. Condition (st.st_mode & 61440) != 32768, taking false branch.
186    else if ((st.st_mode & S_IFMT) != S_IFREG)
187    {
188      // Correct filesystem object, but not a regular file. Treat like
189      // uninitialized mapping.
190      fd = -2;
191    }
192    else
193    {
   	11. toctou: Calling function open that uses proc_info->map_path after a check function. This can cause a time-of-check, time-of-use race condition.
194      fd = open(proc_info->map_path, O_RDONLY);
195      // Double-check against race conditions
196      struct stat st2;
     	CID 330533: Argument cannot be negative (NEGATIVE_RETURNS) [[select issue](https://scan9.scan.coverity.com/defectInstanceId=9450306&fileInstanceId=123282905&mergedDefectId=330533)]
197      if (fstat(fd, &st2) < 0)
198      {
199        close(fd);
200        fd = -1;
201      }
202      else if ((st.st_dev != st2.st_dev) || (st.st_ino != st2.st_ino))
203      {
204        // File has been changed from under us, so ignore.
205        close(fd);
206        fd = -1;
207      }
208    }
209  }

Please sign in to comment.