View Issue Details

IDProjectCategoryView StatusLast Update
0009295GNUnetutil librarypublic2024-10-29 13:18
Reporterfefe Assigned ToChristian Grothoff  
PrioritynormalSeverityminorReproducibilityhave not tried
Status assignedResolutionopen 
Target Version0.22.3 
Summary0009295: GNUNET_DISK_file_test is inherently racy
DescriptionThis function is called, for example, from GNUNET_CONFIGURATION_default.
It will call stat() on the filename and then access. The access man page explicitly warns of using it because that would be a race condition. One of the checks is to make sure it's a regular file.

Here is the code from GNUNET_CONFIGURATION_default:

  2501 /* First, try user configuration. */
  2502 if (NULL != xdg)
  2503 GNUNET_asprintf (&cfgname,
  2504 "%s/%s",
  2505 xdg,
  2506 dpd->config_file);
  2507 else
  2508 cfgname = GNUNET_strdup (dpd->user_config_file);
  2509
  2510 /* If user config doesn't exist, try in
  2511 /etc/<projdir>/<cfgfile> and /etc/<cfgfile> */
  2512 if (GNUNET_OK != GNUNET_DISK_file_test (cfgname))
  2513 {
  2514 GNUNET_free (cfgname);
  2515 GNUNET_asprintf (&cfgname,
  2516 "/etc/%s",
  2517 dpd->config_file);
  2518 }
  2519 if (GNUNET_OK != GNUNET_DISK_file_test (cfgname))
  2520 {
  2521 GNUNET_free (cfgname);
  2522 GNUNET_asprintf (&cfgname,
  2523 "/etc/%s/%s",
  2524 dpd->project_dirname,
  2525 dpd->config_file);
  2526 }
  2527 if (GNUNET_OK != GNUNET_DISK_file_test (cfgname))
  2528 {
  2529 LOG (GNUNET_ERROR_TYPE_ERROR,
  2530 "Unable to top-level configuration file.\n");
  2531 GNUNET_OS_init (pd);
  2532 GNUNET_CONFIGURATION_destroy (cfg);
  2533 GNUNET_free (cfgname);
  2534 return NULL;
  2535 }

Note that this does the same stat test multiple times. Also all the stat tests are worthless if the filesystem changes between the access and the open. Here is the open:

  2537 /* We found a configuration file that looks good, try to load it. */
  2538
  2539 LOG (GNUNET_ERROR_TYPE_DEBUG,
  2540 "Loading top-level configuration from '%s'\n",
  2541 cfgname);
  2542 if (GNUNET_OK !=
  2543 GNUNET_CONFIGURATION_load (cfg, cfgname))

The preferred method is to call open directly and then use fstat to check we opened a regular file.
TagsNo tags attached.

Activities

schanzen

2024-10-23 20:36

administrator   ~0023576

Yeah we obviously call stat way too often. And fstat is missing from the open API.

schanzen

2024-10-23 21:12

administrator   ~0023577

Last edited: 2024-10-23 21:12

Ok I took a jab at this. GNUNET_DISK_file_test is not inherently racy. It is just unreliable when used in combination with open() like this.
It also has other uses.
GNUNET_CONFIGURATION_parse uses GNUNET_DISK_fn_read which does not return a file descriptor and can be used with non-regular files as well (e.g. device files).
So, this would require rewriting GNUNET_CONFIGURATION_parse to use another API.

We may want to check all uses of GNUNET_DISK_fn_read, but I don't think this is a critical bug.
Leaving this open for now.

fefe

2024-10-24 12:40

reporter   ~0023589

What I was trying to say is: A function that calls stat and access is worthless. You might as well always return size = 23 and GNUNET_OK.
Between the call to the function and when you actually use the values, the state of the filesystem could have changed.
In particular someone could symlink-race you to trick you into opening a device instead of a regular file.
You are clearly trying to prevent that by having this check. I'm telling you that the check is basically a NOP.

If you want to prevent accidentally opening /dev/zero, then do the open first and then use fstat.

schanzen

2024-10-24 13:15

administrator   ~0023590

Yes, I understand. I am not convinced that the goal is/was to prevent this. the file_test calls are just there to bail out early, for example if the file does not exist/is unreadable.
If we eventually open /dev/zero then the code will properly handle this as part of the parse.
In other words, the code does not require the configuration file to be a regular file. And I am not sure that is a requirement.
If it is, then we should add a new API that allows us to check if the opened file is a regular file, and not use GNUNET_DISK_fn_read but GNUNET_DISK_file_open to use the file handle on that new API.

Issue History

Date Modified Username Field Change
2024-10-23 14:28 fefe New Issue
2024-10-23 20:36 schanzen Note Added: 0023576
2024-10-23 21:12 schanzen Note Added: 0023577
2024-10-23 21:12 schanzen Note Edited: 0023577
2024-10-24 12:40 fefe Note Added: 0023589
2024-10-24 13:15 schanzen Note Added: 0023590
2024-10-24 13:28 schanzen Target Version => 0.22.2
2024-10-24 13:28 schanzen Assigned To => schanzen
2024-10-24 13:28 schanzen Status new => assigned
2024-10-29 13:18 schanzen Assigned To schanzen => Christian Grothoff
2024-10-29 13:18 schanzen Target Version 0.22.2 => 0.22.3