access + mkdir: a time-of-check, time-of-use race condition

[bagder]

Detected by Coverity.

curl-7.60.0/src/tool_dirhie.c:142: fs_check_call: Calling function "access" to
perform check on "dirbuildup".
curl-7.60.0/src/tool_dirhie.c:143: toctou: Calling function "mkdir" that uses
"dirbuildup" after a check function. This can cause a time-of-check, time-of-use
race condition.
#  141|         }
#  142|         if(access(dirbuildup, F_OK) == -1) {
#  143|->         if(-1 == mkdir(dirbuildup, (mode_t)0000750)) {
#  144|             show_dir_errno(errors, dirbuildup);
#  145|             result = CURLE_WRITE_ERROR;

[bagder]

The question is what a decent fix would be. I'm thinking:

1. call mkdir() unconditionally
2. if mkdir fails, check if the name exists as a directory, if it's not, fail
3. if it is a directory, continue

Then, if the mkdir() fails for another reason but the path is a directory, that's still fine.

[bagder]

The question is what a decent fix would be. I'm thinking:

1. call mkdir() unconditionally
2. if mkdir fails, check if the name exists as a directory, if it's not, fail
3. if it is a directory, continue

Then, if the mkdir() fails for another reason but the path is a directory, that's still fine.

[RootUp]

Thanks @bagder

[RootUp]

Thanks @bagder

[jay]

yes easiest way i think is check EEXIST

diff --git a/src/tool_dirhie.c b/src/tool_dirhie.c
index a01f9dc..5755823 100644
--- a/src/tool_dirhie.c
+++ b/src/tool_dirhie.c
@@ -139,12 +139,10 @@ CURLcode create_dir_hierarchy(const char *outfile, FILE *e
         else
           snprintf(dirbuildup, outlen, "%s%s", DIR_CHAR, tempdir);
       }
-      if(access(dirbuildup, F_OK) == -1) {
-        if(-1 == mkdir(dirbuildup, (mode_t)0000750)) {
-          show_dir_errno(errors, dirbuildup);
-          result = CURLE_WRITE_ERROR;
-          break; /* get out of loop */
-        }
+      if(-1 == mkdir(dirbuildup, (mode_t)0000750) && errno != EEXIST) {
+        show_dir_errno(errors, dirbuildup);
+        result = CURLE_WRITE_ERROR;
+        break; /* get out of loop */
       }
     }
     tempdir = tempdir2;

[jay]

yes easiest way i think is check EEXIST

diff --git a/src/tool_dirhie.c b/src/tool_dirhie.c
index a01f9dc..5755823 100644
--- a/src/tool_dirhie.c
+++ b/src/tool_dirhie.c
@@ -139,12 +139,10 @@ CURLcode create_dir_hierarchy(const char *outfile, FILE *e
         else
           snprintf(dirbuildup, outlen, "%s%s", DIR_CHAR, tempdir);
       }
-      if(access(dirbuildup, F_OK) == -1) {
-        if(-1 == mkdir(dirbuildup, (mode_t)0000750)) {
-          show_dir_errno(errors, dirbuildup);
-          result = CURLE_WRITE_ERROR;
-          break; /* get out of loop */
-        }
+      if(-1 == mkdir(dirbuildup, (mode_t)0000750) && errno != EEXIST) {
+        show_dir_errno(errors, dirbuildup);
+        result = CURLE_WRITE_ERROR;
+        break; /* get out of loop */
       }
     }
     tempdir = tempdir2;

[RootUp]

Hey @bagder
@jay patch works for me this can help, to resolve this issue.

[RootUp]

Hey @bagder
@jay patch works for me this can help, to resolve this issue.

[bagder]

Right, so someone needs to make a PR out of it...

[bagder]

Right, so someone needs to make a PR out of it...

[RootUp]

Thanks @jay for the patch, I made a PR for same.
Request @bagder to please have a look on it and suggest are we good to land :)

[RootUp]

Thanks @jay for the patch, I made a PR for same.
Request @bagder to please have a look on it and suggest are we good to land :)