Fix Unix.utimes not working for directories on Win32. by dmbaturin · Pull Request #8796 · ocaml/ocaml

dmbaturin · 2019-07-08T21:37:43Z

On Win32, Unix.utimes fails with EACCESS.

# Unix.stat "testdir";;
- : Unix.stats =
{Unix.st_dev = 68940724; st_ino = 336654; st_kind = Unix.S_DIR;
 st_perm = 511; st_nlink = 1; st_uid = 0; st_gid = 0; st_rdev = 68940724;
 st_size = 0; st_atime = 1562621282.6993136; st_mtime = 1562621282.6993136;
 st_ctime = 1562621282.6993136}

# Unix.utimes "testdir" 0.0 0.0 ;;
Exception: Unix.Unix_error (Unix.EACCES, "utimes", "testdir").

The cause is that CreateFile* functions need a special flag to work with directories, FILE_FLAG_BACKUP_SEMANTICS. Without it, it returns errno=6 (INVALID_HANDLE).
See https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#directories

It appears to have no ill effects for normal files, and Python's os.utime implementation on Win32 uses it: https://github.com/python/cpython/blob/master/Modules/posixmodule.c#L4830-L4859

I'm by no means a Windows expert though, so this requires a review from someone who is.

dmbaturin · 2019-07-08T22:36:50Z

I see that the Travis build failed due to missing Changes entry. Somehow I have missed that section in CONTRIBUTING.md, sorry.
I'm confused by the process though. If it requires "review by", should it be only added after the PR is reviewed?

nojb · 2019-07-09T04:03:19Z

The change looks reasonable to me. Let's wait and see what @dra27 thinks.

Yes, you need to add an entry to the Changes file (in the Bug fixes section) and fill it out with the names of the reviewers before it is merged.

Thanks!

dra27 · 2019-07-09T15:23:08Z

I'm afraid using FILE_FLAG_BACKUP_SEMANTICS this way is a bit like embedding a sudo call inside the library - especially for files, it allows the function to access directories and files which should not be accessible without explicit authorisation (the main job of FILE_FLAG_BACKUP_SEMANTICS is to disable security checks). It's a horrible hack that directory handles are also accessed this way - it's necessary to ensure that SeRestorePrivilege is not active for the process during the CreateFile call, so that you can be sure that it's opened a handle to a directory to which you normally have access. I've put a full implementation of it in dra27@128bbb7 which ensures that SeRestorePrivilege is correctly dropped, even in the presence of multiple threads.

I'm mildly surprised that the Python and Go guys weren't more suspicious.

This could be partially mitigated by only specifying FILE_FLAG_BACKUP_SEMANTICS for directories. It's not exploitable, per se, since if the current user has SeRestorePrivilege and has enabled it then several manual steps have already been taken, but I don't think it's great that that's hidden inside a general-purpose function.

dra27 · 2019-10-11T13:25:29Z

It would be good to have a fix for this in 4.10, but I'm afraid I'm not going to have time to deal with @stedolan's review comment in #8797. I propose one of these two:

Merge this PR as-is
Include a test for whether wpath is a directory and specify FILE_FLAG_BACKUP_SEMANTICS only for a directory, and not a file

I prefer the second, since it makes utimes behave in the same slightly incorrect way as stat and readlink presently do. I will give some time to some of my outstanding PRs after the 4.10 branch and then the proper fix should be in 4.11.0.

What do you think @nojb?

nojb · 2019-10-11T13:30:59Z

It would be good to have a fix for this in 4.10, but I'm afraid I'm not going to have time to deal with @stedolan's review comment in #8797. I propose one of these two:

Merge this PR as-is

Include a test for whether wpath is a directory and specify FILE_FLAG_BACKUP_SEMANTICS only for a directory, and not a file

I prefer the second, since it makes utimes behave in the same slightly incorrect way as stat and readlink presently do. I will give some time to some of my outstanding PRs after the 4.10 branch and then the proper fix should be in 4.11.0.

What do you think @nojb?

Since this is a bug fix, wouldn't it be possible to get it into 4.10 even after the freeze ? In any case, 2 sounds OK as well. Unfortunately I am overloaded at the moment so I can't help getting this in shape for merging.

dmbaturin · 2019-10-11T14:46:50Z

@nojb If option 2 is agreed upon, I can do it myself. I haven't abandoned my PR.

nojb · 2019-10-11T14:51:16Z

@nojb If option 2 is agreed upon, I can do it myself. I haven't abandoned my PR.

I didn't mean to imply that :)

Please go ahead and do that if you can, we can merge @dra27's more complete fix in a later PR.

damiendoligez · 2019-10-15T12:23:16Z

This and #8797 are both bug fixes and can be merged after the (imminent) branching of 4.10.

gasche · 2019-11-07T12:46:02Z

We still need people to converge on this PR and carefully review @dra27' #8797 if we want to have them in 4.10.

damiendoligez · 2020-04-03T15:32:37Z

@dmbaturin are you still willing to work on this? #8797 seems to be stalled as well.

dra27 · 2020-05-04T13:16:07Z

@damiendoligez / @Octachron - #8797 is too invasive to risk putting in 4.11 (but I'm gradually working through my backlog so I will be looking for its being merged before 4.12). However, the change here fixes a real bug in the same way as many other language libraries already do, so could we put this into 4.11 and then replace it with the comprehensive version in 4.12?

dmbaturin · 2020-05-04T14:02:34Z

@damiendoligez Sorry, I've neglected this indeed. I'm ready to get back to it if we have a course of action agreed upon. Do we? :)

This proposal is no worse than mainstream, and to me it seems like security issues it may cause are mostly theoretical. However, I'm not a Windows expert by any means so I'd rather leave the final decision to @dra27. If there's anything you want me to change in this PR before it can be merged, I'm ready to do that.

damiendoligez · 2020-06-03T16:35:33Z

It seems to me that we have a course of action: merge this now and delay #8797 to 4.12.

@dmbaturin if you could add a Changes entry, I'll merge this PR.

dmbaturin · 2020-06-03T16:51:01Z

@damiendoligez Done. Could you check?

damiendoligez · 2020-11-03T10:07:03Z

Merged to trunk and cherry-picked to 4.12 (commit f3ff689). I guess #8797 will wait for 4.13.

damiendoligez · 2020-11-03T10:07:57Z

Thank you @dmbaturin for your contribution.

Fix Unix.utimes not working for directories on Win32.

79f0b29

dmbaturin mentioned this pull request Jul 8, 2019

Skip tests that require POSIX semantics on Win32 gildor478/ocaml-fileutils#9

Merged

dra27 mentioned this pull request Jul 10, 2019

Fix use of FILE_FLAG_BACKUP_SEMANTICS in win32unix #8797

Draft

damiendoligez added the bug label Oct 15, 2019

damiendoligez self-assigned this Jun 3, 2020

dmbaturin added 2 commits June 3, 2020 23:43

Merge https://github.com/ocaml/ocaml into trunk

ed26d31

Update changelog for #8796

7a41d21

Merge branch 'trunk' into trunk

50bcdf4

damiendoligez merged commit 0280127 into ocaml:trunk Nov 3, 2020

damiendoligez pushed a commit that referenced this pull request Nov 3, 2020

Fix Unix.utimes not working for directories on Win32. (#8796)

f3ff689

Conversation

dmbaturin commented Jul 8, 2019

Uh oh!

dmbaturin commented Jul 8, 2019

Uh oh!

nojb commented Jul 9, 2019

Uh oh!

dra27 commented Jul 9, 2019

Uh oh!

dra27 commented Oct 11, 2019

Uh oh!

nojb commented Oct 11, 2019

Uh oh!

dmbaturin commented Oct 11, 2019

Uh oh!

nojb commented Oct 11, 2019

Uh oh!

damiendoligez commented Oct 15, 2019

Uh oh!

gasche commented Nov 7, 2019

Uh oh!

damiendoligez commented Apr 3, 2020

Uh oh!

dra27 commented May 4, 2020

Uh oh!

dmbaturin commented May 4, 2020

Uh oh!

damiendoligez commented Jun 3, 2020

Uh oh!

dmbaturin commented Jun 3, 2020

Uh oh!

damiendoligez commented Nov 3, 2020

Uh oh!

damiendoligez commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants