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.

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

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

Closed
bagder opened this issue Jul 12, 2018 · 6 comments
Closed

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

bagder opened this issue Jul 12, 2018 · 6 comments
Labels
cmdline tool help wanted

Comments

@bagder
Copy link
Member

bagder commented Jul 12, 2018

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
Copy link
Member Author

bagder commented Jul 12, 2018

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
Copy link

RootUp commented Jul 12, 2018

Thanks @bagder

@jay
Copy link
Member

jay commented Jul 12, 2018

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
Copy link

RootUp commented Aug 12, 2018

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

@bagder
Copy link
Member Author

bagder commented Aug 12, 2018

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

@RootUp RootUp mentioned this issue Aug 12, 2018
Closed
@RootUp
Copy link

RootUp commented Aug 12, 2018

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 :)

bagder added a commit that referenced this issue Aug 24, 2018
@bagder
curl: fix time-of-check, time-of-use race in dir creation
eb1592e 
Detected by Coverity
Fixes #2739
@bagder bagder mentioned this issue Aug 24, 2018
Closed
@bagder bagder closed this as completed in f16bed0 Aug 25, 2018
falconindy pushed a commit to falconindy/curl that referenced this issue Sep 10, 2018
@bagder @falconindy
curl: fix time-of-check, time-of-use race in dir creation
04b2f4f 
Patch-by: Jay Satiro
Detected by Coverity
Fixes curl#2739
Closes curl#2912
@lock lock bot locked as resolved and limited conversation to collaborators Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
cmdline tool help wanted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

time-of-check, time-of-use race condition
3 participants
@bagder @jay @RootUp