classification
Title: Change os.access to check ACLs under Windows
Type: enhancement Stage: patch review
Components: Library (Lib), Windows Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: tim.golden Nosy List: amaury.forgeotdarc, christian.heimes, eryksun, paul.moore, piotr.dobrogost, serhiy.storchaka, tim.golden
Priority: normal Keywords: patch

Created on 2008-04-01 14:19 by tim.golden, last changed 2021-03-12 02:11 by eryksun.

Files
File name Uploaded Description Edit
issue2528.2.patch tim.golden, 2013-08-05 15:58 review
Messages (12)
msg64811 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2008-04-01 14:19
At present, os.access under Windows simply calls GetFileAttributes to
determine the readonly attribute (ignoring directories). The patch
attached combines this with the use of the AccessCheck API to compare
the user's permissions with those required for the path.

I'm assuming that ATTRIB and CACLS will be available for use in the unit
tests included.

I haven't altered the structure of the posix_access function at all
although I suspect that it could now be simplified now that we're not
supporting Win9x.
msg109858 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-10 12:12
A quick look tells me that the patch seems clean.  However it involves changes to posixmodule.c and I don't (yet) want to get involved with doing builds, so could someone else please give this a try.
msg109871 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-07-10 14:23
But what are the benefits of this change?
msg109907 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-07-10 18:48
Although I'm the implementer of the patch (the concept
was discussed way back on c.l.py after a naive poster's
original request) I'm probably +0 myself. It's an attempt
to replace os.access' next-to-useless behaviour on Windows
with something which at least uses current security mechanisms
to determine its answer.

However, it's just one function among the many Posixesque
functions which don't quite map to Windows. And one which
you might well be advised to avoid in any case since there's
an race condition inherent in checking for a file's accessibility
and then making use of that fact.  Better to try and fall back.
msg110810 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-19 21:06
Tim, do you want more time to think about this, could we close as "won't fix" or what?
msg192729 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-09 08:37
Do you want to provide an updated patch for 3.4?
msg194489 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2013-08-05 15:58
Here's an updated patch against trunk with tests & doc changes
msg194490 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2013-08-05 16:00
... and to answer Amaury's question in msg109871 it creates a reasonable consistency between the results of os.access and the user's actual ability to read / write a file. eg, you might have no permissions whatsoever on the file but as long as it wasn't read-only, os.access would return True for reading, writing and executing.
msg236144 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-02-17 18:52
The solution proposed here could help resolve #22107.
msg243892 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2015-05-23 07:25
In msg243815 you asked me to look over this patch. I hope this helps.

For GetFileSecurity you need to also request LABEL_SECURITY_INFORMATION. To test this in Vista+, use a file in the root directory of the system drive. This will inherit a high integrity level w/ no write up, which denies write access to a medium integrity (non-elevated) process. If you don't include the label information, the check will erroneously claim a non-elevated token has write access. 

You can set a file's integrity level to high using `icacls filename /setintegritylevel High`. This prevents write up. You can prevent read up and execute up by using the Windows API directly, or with other administration tools such as chml.

As a performance tweak you could get the file security with an initial buffer of 512 bytes. That should be big enough for most file security descriptors. I'd use PyMem_RawRealloc in a do loop (i.e. starting with pSD == NULL and cbSD == 512). It should set PyExc_MemoryError on failure.

Regarding ImpersonateSelf, what if the thread is already impersonating? That's the token the kernel will actually use when checking access. Given that, I'd try OpenThreadToken, and only if it fails call ImpersonateSelf. Then RevertToSelf immediately after the 2nd OpenThreadToken call. Reverting doesn't have to be delayed until after the check. An alternative to ImpersonateSelf is to manually duplicate the process token as an impersonation token (i.e. OpenProcessToken; DuplicateToken). The benefit of this approach is that the duplicated token can be cached in a static variable.

For the access check itself, use the FILE_GENERIC_* / FILE_ALL_ACCESS constants, which are the standard and specific rights for the corresponding GENERIC_* rights. These include the standard READ_CONTROL and SYNCHRONIZE rights that are required for accessing files.

Also, there's no need to call MapGenericMask here, since you know that access_desired doesn't contain any generic rights.

Finally, for backward compatibility this new implementation should default to using only the old file-attribute check. Chaining to the ACL-based check should require a keyword-only argument such as "use_acl".
msg243897 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2015-05-23 08:22
Thanks for the very thorough review. This isn't going to make it into 
3.5, but I'll rework it in the light of your comments and see if people 
are happy with it in the optional argument variation.
msg388534 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-03-12 02:11
With increasing use of os.access() in shutil and tempfile, it would be nice to have a real implementation of os.access() for Windows. 

Instead of manually evaluating the security of the file/directory, as issue2528.2.patch attempts to do, I'd rather just open the file with the desired access (e.g. GENERIC_READ, GENERIC_WRITE, GENERIC_EXECUTE). An open-based check supports checking for sharing violations, filesystem policy (e.g. FILE_READ_ATTRIBUTES granted by the parent directory), non-filesystem devices, and access policy implemented by filter drivers in the device stack. 

The code to open the file/directory can be factored out and generalized from the stat() implementation. The common open function can implement the flags AT_SYMLINK_NOFOLLOW and AT_EACCESS (without which it should temporarily revert to the process access token). Also, when a directory is opened with GENERIC_WRITE access, it can re-try the open with FILE_DELETE_CHILD access, which POSIX includes in write access for a directory.

An S_OK flag could also be supported to ignore a sharing violation in Windows. [MS-FSA] section 2.1.5.1.2 (Open of an Existing File) specifies that access sharing is checked after the readonly attribute and file security access check. So if an open fails with a sharing violation, the caller knows that access was otherwise granted.
History
Date User Action Args
2021-03-12 02:11:01eryksunsetmessages: + msg388534
versions: + Python 3.8, Python 3.9, Python 3.10, - Python 3.5
2016-02-09 10:31:42BreamoreBoysetnosy: - BreamoreBoy
2016-02-09 10:17:20piotr.dobrogostsetnosy: + piotr.dobrogost
2015-09-20 11:32:10eryksununlinkissue25189 superseder
2015-09-20 06:05:56serhiy.storchakalinkissue25189 superseder
2015-05-23 20:27:20paul.mooresetnosy: + paul.moore
2015-05-23 08:22:53tim.goldensetmessages: + msg243897
2015-05-23 07:25:43eryksunsetnosy: + eryksun

messages: + msg243892
versions: + Python 3.5, - Python 3.4
2015-02-17 19:23:08serhiy.storchakasetnosy: + serhiy.storchaka
2015-02-17 18:52:52BreamoreBoysetnosy: + BreamoreBoy
messages: + msg236144
2014-02-03 17:03:39BreamoreBoysetnosy: - BreamoreBoy
2013-08-05 18:32:15tim.goldensetfiles: - os_access-r62091.patch
2013-08-05 16:00:48tim.goldensetmessages: + msg194490
2013-08-05 15:58:18tim.goldensetstatus: languishing -> open
files: + issue2528.2.patch
messages: + msg194489
2013-07-09 08:37:42christian.heimessetstatus: open -> languishing
versions: + Python 3.4, - Python 3.2
nosy: + christian.heimes

messages: + msg192729
2010-08-25 08:49:58tim.goldensetassignee: tim.golden
2010-07-19 21:06:25BreamoreBoysetmessages: + msg110810
2010-07-10 18:48:49tim.goldensetmessages: + msg109907
2010-07-10 14:23:53amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg109871
2010-07-10 12:12:11BreamoreBoysetnosy: + BreamoreBoy

messages: + msg109858
versions: - Python 2.7
2009-05-16 19:38:53ajaksu2setpriority: normal
versions: + Python 2.7, Python 3.2, - Python 2.6
type: enhancement
components: + Windows
stage: patch review
2008-04-01 14:19:18tim.goldencreate