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

Use Linux O_TMPFILE flag in tempfile.TemporaryFile? #65714

Closed
vstinner opened this issue May 16, 2014 · 28 comments
Closed

Use Linux O_TMPFILE flag in tempfile.TemporaryFile? #65714

vstinner opened this issue May 16, 2014 · 28 comments

Comments

@vstinner
Copy link
Member

BPO 21515
Nosy @birkenfeld, @ncoghlan, @pitrou, @vstinner, @socketpair, @serhiy-storchaka, @MojoVampire
Files
  • tempfile_o_tmpfile.patch
  • tempfile_o_tmpfile3.patch
  • tempfile_comment.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-10-20.22:20:25.342>
    created_at = <Date 2014-05-16.09:17:21.221>
    labels = []
    title = 'Use Linux O_TMPFILE flag in tempfile.TemporaryFile?'
    updated_at = <Date 2021-09-12.23:29:16.133>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-09-12.23:29:16.133>
    actor = 'WGH'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-10-20.22:20:25.342>
    closer = 'vstinner'
    components = []
    creation = <Date 2014-05-16.09:17:21.221>
    creator = 'vstinner'
    dependencies = []
    files = ['35261', '35277', '40824']
    hgrepos = []
    issue_num = 21515
    keywords = ['patch']
    message_count = 28.0
    messages = ['218648', '218649', '218650', '218653', '218660', '218661', '218667', '218670', '218671', '218690', '218738', '218739', '219792', '219803', '219807', '219808', '220037', '220059', '253196', '253213', '253238', '253240', '253245', '253247', '253248', '253265', '253266', '401675']
    nosy_count = 11.0
    nosy_names = ['georg.brandl', 'ncoghlan', 'pitrou', 'vstinner', 'Arfrever', 'neologix', 'socketpair', 'python-dev', 'serhiy.storchaka', 'josh.r', 'WGH']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue21515'
    versions = ['Python 3.5']

    @vstinner
    Copy link
    Member Author

    Linux 3.11 introduced a new file flag "O_TMPFILE". The flag is exposed in Python, see the issue bpo-18673.

    "O_TMPFILE is a new open(2)/openat(2) flag that makes easier the creation of secure temporary files. Files opened with the O_TMPFILE flag are created but they are not visible in the filesystem. And as soon as they are closed, they get deleted - just as a file you would have opened and unlinked."
    http://kernelnewbies.org/Linux_3.11#head-8be09d59438b31c2a724547838f234cb33c40357

    Does it make sense to use this flag in tempfile.TemporaryFile?

    Attached patch is a work-in-progress patch for tempfile.

    if hasattr(_os, 'O_TMPFILE'):
    _O_TMPFILE = _os.O_TMPFILE
    elif _sys.platform == 'linux':
    __O_TMPFILE = 0o20000000
    _O_TMPFILE = (__O_TMPFILE | _os.O_DIRECTORY)

    The second if should be removed. I used it because my Linux kernel (3.14) supports the flag, but the constant is not defined yet in C headers of my C library (glibc 2.18).

    flags = (flags | _O_TMPFILE) & ~_os.O_CREAT

    O_CREAT is incompatible with O_TMPFILE.

    Bonus point of the flag: no need to compute a random name! Just pass the temporary directory.

    To do: test the patch on Linux < 3.11 to see how the flag is interpreted. If the flag is ignored, we open the directory in write mode! That's insafe. If the flag raises an error, we should fallback to the current implementation and remember that the flag is not supported.

    I implemented something similar for O_CLOEXEC and SOCK_CLOEXEC flags (PEP-433).

    @pitrou
    Copy link
    Member

    pitrou commented May 16, 2014

    I don't think we can use this by default, or it will break the expected semantics of temporary files under Unix (visible by other processes).

    @vstinner
    Copy link
    Member Author

    "I don't think we can use this by default, or it will break the expected semantics of temporary files under Unix (visible by other processes)."

    I proposed to change TemporaryFile, not NamedTemporaryFile. Do you mean that other processes are supposed to have access to the temporary file descriptor? Access through /proc/pid/fd/<tmp_fd>?

    O_TMPFILE should increase the security because there is no more race condition between os.open() and os.unlink() (window where an attack can access the file).

    My patch uses O_EXCL. It makes possible to use linkat() to create a path for the temporary file (I didn't try it, but I read that it's possible). I don't know if using O_EXCL should be the default.

    @pitrou
    Copy link
    Member

    pitrou commented May 16, 2014

    I proposed to change TemporaryFile, not NamedTemporaryFile.

    Ah, sorry. Then it sounds ok.
    (I couldn't find any documentation for O_TMPFILE, though)

    @vstinner
    Copy link
    Member Author

    It looks like O_TMPFILE is supported by tmpfs (3.11), ext3 (3.11), ext4 (3.11), XFS (3.15). It looks like BTRFS will also support the O_TMPFILE:
    https://btrfs.wiki.kernel.org/index.php/Project_ideas#Implement_O_TMPFILE_support

    --

    It looks like os.open() fails with OSError(95, 'Operation not supported') if the filesystem of the directory doesn't support TMPFILE. In this case, a fallback to the current implementation should be enough. I don't think that we need to remember that the directory doesn't support TMPFILE. The directory may be on a different filesystem at the next call.

    haypo@smithers$ ~/prog/python/default/python 
    Python 3.5.0a0 (default:5e98a50e0f55, May 16 2014, 10:44:10) 
    >>> import tempfile
    >>> tempfile._O_TMPFILE
    4259840
    >>> f=tempfile.TemporaryFile(dir='.')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/haypo/prog/python/default/Lib/tempfile.py", line 507, in TemporaryFile
        fd = _os.open(dir, flags, 0o600)
    OSError: [Errno 95] Operation not supported: '.'

    haypo@smithers$ df .
    Sys. de fichiers Taille Utilisé Dispo Uti% Monté sur
    192.168.0.42:/test 96G 9,1G 83G 10% /home/haypo/nfs

    @vstinner
    Copy link
    Member Author

    It looks like open() ignores O_TMPFILE (0o20000000) on old Linux kernels. Test on Linux 3.2:

    >>> fd=os.open("/tmp", os.O_RDWR | O_TMPFILE, 0o600)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [Errno 21] Is a directory: '/tmp'
    
    >>> fd=os.open("/tmp", os.O_RDWR | os.O_DIRECTORY, 0o600)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [Errno 21] Is a directory: '/tmp'

    So we should catch OSError(21, "Is a directory: '/tmp'") and fallback to the current implementation (random name, unlink), and remember that the kernel version is too old.

    @pitrou
    Copy link
    Member

    pitrou commented May 16, 2014

    So we should catch OSError(21, "Is a directory: '/tmp'") and fallback
    to the current implementation (random name, unlink), and remember that
    the kernel version is too old.

    Just catch any OSError?

    @vstinner
    Copy link
    Member Author

    Just catch any OSError?

    If possible, I would prefer to not retry O_TMPFILE each time if the kernel version does not support the flag.

    Pseudo-code:

    if o_tmpfile_supported:
    try:
    fd = os.open(dir, os.O_TMPFILE | ...)
    except IsADirectoryError:
    # Linux kernel older than 3.11 ignores O_TMPFILE flag
    o_tmpfile_supported = False
    except OSError:
    # the filesystem doesn't support O_TMPFILE
    pass
    else:
    return io.open(fd, ...)
    # fallback to unsafe but portable implementation

    # current code generating a name and using unlink
    ---

    @pitrou
    Copy link
    Member

    pitrou commented May 16, 2014

    > Just catch any OSError?

    If possible, I would prefer to not retry O_TMPFILE each time if the kernel version does not support the flag.

    How likely it is to have a glibc flag that's not supported by the kernel
    (on a normal setup, not a self-compiled distro)?

    @ncoghlan
    Copy link
    Contributor

    Reasonably common, I believe. For example, Red Hat ships a Developer
    Toolset, so you may be building with an up to date gcc on RHEL 6 or 7, but
    still support deploying against the older kernel in RHEL 5.

    @vstinner
    Copy link
    Member Author

    tempfile_o_tmpfile2.patch: Updated patch to handle OS errors.

    I'm not sure that __O_TMPFILE has the same value on all architectures.

    The O_TMPFILE flag was added to fcntl.h in the glibc 2.19 (released the 8 Feb 2014):
    https://sourceware.org/git/?p=glibc.git;a=commit;h=ffdd31816a67f48697ea4d6b852e58d2886d42ca

    My Linux Fedora 20 uses the glibc 2.18.

    I removed the hardcoded constant from my patch. Add this to Lib/tempfile.py to test manually if you have a glibc older than 2.19:

    # after "if hasattr(_os, 'O_TMPFILE'):"
    elif _sys.platform == 'linux':
    __O_TMPFILE = 0o20000000
    _O_TMPFILE = (__O_TMPFILE | _os.O_DIRECTORY)

    @vstinner
    Copy link
    Member Author

    (Oops, I made a mistake, the hardcoded constant was still present in my patch 2.)

    Patch 3 uses tempfile._O_TMPFILE_WORKS variable to check if the O_TMPFILE flag is avaialble and works.

    Use "os.O_TMPFILE = 0o20000000 | os.O_DIRECTORY" to try my patch with glibc older than 2.19.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jun 5, 2014

    Can someone please review tempfile_o_tmpfile3.patch ?

    @pitrou
    Copy link
    Member

    pitrou commented Jun 5, 2014

    It would be nice if the patch added a pointer to the O_TMPFILE documentation (if that exists) and mentioned that it is Linux-specific.
    Otherwise, it looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 5, 2014

    New changeset 4b51a992cb70 by Victor Stinner in branch 'default':
    Issue bpo-21515: tempfile.TemporaryFile now uses os.O_TMPFILE flag is available
    http://hg.python.org/cpython/rev/4b51a992cb70

    @vstinner
    Copy link
    Member Author

    vstinner commented Jun 5, 2014

    It would be nice if the patch added a pointer to the O_TMPFILE documentation (if that exists) and mentioned that it is Linux-specific.

    I modified TemporaryFile documentation to mention that the O_TMPFILE
    flag is used if available and if the flag "works".

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Jun 8, 2014

    Minor inconsistency in Lib/tempfile.py:
    # Set flag to None to not try again.
    _O_TMPFILE_WORKS = False

    s/None/False/

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 8, 2014

    New changeset 8b93cdccd872 by Victor Stinner in branch 'default':
    Issue bpo-21515: Fix typo in a comment, thanks Arfrever for the report
    http://hg.python.org/cpython/rev/8b93cdccd872

    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented Oct 19, 2015

    Suppose conditions:

    • Old linux kernel ignoring flag
    • malicious hacker force use of PLAIN FILE instead of directory

    On new kernel it will fail
    On old kernel it will just open that file!

    So, we can make a HACK! Just add last slash to directory name. This will not hurt on new kernels, but protect on old kernels.

    tests should also test a case when directory is symlink really.

    @vstinner
    Copy link
    Member Author

    Suppose conditions:

    • Old linux kernel ignoring flag
    • malicious hacker force use of PLAIN FILE instead of directory

    Is it a theorical bug, or are you able to reproduce it?

    Old Linux kernel ignores the 0o20000000 bit but O_TMPFILE is 0o20000000 | os.O_DIRECTORY. So the kernel still ensures that the path is a directory. tempfile.TemporaryFile() tries to open the path with:

       os.open(path, os.O_RDWR |os.O_EXCL | os.O_TMPFILE)
    
    if the 0o20000000 bit is ignored by old kernel, it becomes:
    
       os.open(path, os.O_RDWR |os.O_EXCL | os.O_DIRECTORY)

    You cannot open a regular file with these flags:

    >>> open('x', 'w').close()
    >>> os.open('x', os.O_RDWR |os.O_EXCL | os.O_DIRECTORY)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    NotADirectoryError: [Errno 20] Not a directory: 'x'

    You cannot open a directory with these flags:

    >>> os.open('.', os.O_RDWR |os.O_EXCL | os.O_DIRECTORY)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    IsADirectoryError: [Errno 21] Is a directory: '.'

    Same behaviour for symbolic link to a regular file or to a directory.

    Please open a new issue if you consider that you found a bug, but please write a short script reproducing the bug.

    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented Oct 20, 2015

    Okay, seemes it is not documented that

    os.open('.', os.O_RDWR |os.O_EXCL | os.O_DIRECTORY)

    Should return EISDIR

    I did not found that in Linux manpages. Using undocumented features is bad. Maybe I should report this to Michael Kerrisk to update manpage ?

    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented Oct 20, 2015

    Well, it's not said explicit, that O_DIRECTORY cannot be combined with O_RDWR.

    So, everything is valid now, very hacky, but works without bugs.

    It will be nice, if someone comment that hacks in source code

    @vstinner
    Copy link
    Member Author

    2015-10-20 20:02 GMT+02:00 Марк Коренберг <report@bugs.python.org>:

    Okay, seemes it is not documented that

    os.open('.', os.O_RDWR |os.O_EXCL | os.O_DIRECTORY)

    Should return EISDIR

    You cannot open a directory to write, only to read.

    @vstinner
    Copy link
    Member Author

    It will be nice, if someone comment that hacks in source code

    I don't understand why you keep calling this a hack. It's part of open() contract, and I'm quite sure that it was a deliberate choice to declare O_TMPFILE as O_DIRECTY|new_bit. See for example this comment:
    https://lwn.net/Articles/560834/

    I wrote a patch to explain that it's fine to call open() with O_TMPFILE on old kernels to check if the flag is supported: see attached patch.

    @vstinner vstinner reopened this Oct 20, 2015
    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented Oct 20, 2015

    Huge thanks for that patch. Now things are much cleaner.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 20, 2015

    New changeset dc2deecb2346 by Victor Stinner in branch '3.5':
    Issue bpo-21515: Elaborate tempfile.TemporaryFile() comment
    https://hg.python.org/cpython/rev/dc2deecb2346

    @vstinner
    Copy link
    Member Author

    Huge thanks for that patch. Now things are much cleaner.

    I understand that the patch looks good to you, so I pushed it to Python 3.5 & 3.6. I close again the issue. Thanks for your analasys of tempfile.TemporaryFile() :-)

    @WGH
    Copy link
    Mannequin

    WGH mannequin commented Sep 12, 2021

    My patch uses O_EXCL. It makes possible to use linkat() to create a path for the temporary file (I didn't try it, but I read that it's possible). I don't know if using O_EXCL should be the default.

    I think it is the other way around. From the manual: "If O_EXCL is not specified, then linkat(2) can ..."

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants