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

suggestion: deprecate/replace Filename.temp_file #3169

Closed
vicuna opened this issue Jan 23, 2002 · 2 comments
Closed

suggestion: deprecate/replace Filename.temp_file #3169

vicuna opened this issue Jan 23, 2002 · 2 comments

Comments

@vicuna
Copy link

vicuna commented Jan 23, 2002

Original bug ID: 829
Reporter: administrator
Status: closed
Resolution: fixed
Priority: normal
Severity: feature
Category: ~DO NOT USE (was: OCaml general)

Bug description

Hello,

Since temporary file name vulnerabilities have been well-known by
security people and hackers alike, they make easy targets, so it's
probably best to avoid the possible problems by providing secure means
of obtaining temp files. It looks to like Filename.temp_file is
pretty unsecure, which makes me feel insecure, or something :-)

The problems with it are:

  • it uses easily guessed filenames based on a sequential counter from
    0 to 999.
  • because of the linear scan, if someone used hundreds of tempfiles,
    they'd be doing lots of unecessary filesystem work.
  • and it also leads to a possible denial of service attack if the
    attacker pre-creates all the 1000 files
  • it returns the filename, not a safely opened file descriptor, so
    there is a race condition before the user opens the tempfile
  • it opens with permissions of 0o666, not 0o600, so if a user has a
    silly umask, their tempfile can even be overwritten. Even with a
    normal umask like 022, their temp file will be world readable, so
    private information could possibly be read.

I recommend that Filename.temp_file be deprecated and that 1 or 2 new
functions be created in Sys that correspond to the (safer and more
secure) glibc functions mkstemp(3)/tmpfile(3).

I may try implementing versions of these in pure Ocaml, just for
practice. If they could be of help, please let me know, but it might
be best to directly use the glibc functions.

cheers,
doug

@vicuna
Copy link
Author

vicuna commented Apr 12, 2002

Comment author: administrator

Dear Doug,

Since temporary file name vulnerabilities have been well-known by
security people and hackers alike, they make easy targets, so it's
probably best to avoid the possible problems by providing secure means
of obtaining temp files. It looks to like Filename.temp_file is
pretty unsecure, which makes me feel insecure, or something :-)

The problems with it are:

  • it uses easily guessed filenames based on a sequential counter from
    0 to 999.

Correct.

  • because of the linear scan, if someone used hundreds of tempfiles,
    they'd be doing lots of unecessary filesystem work.

Right.

  • and it also leads to a possible denial of service attack if the
    attacker pre-creates all the 1000 files

Right. The reason for limiting the number of file names tried is that
the temporary directory might not exist, or have wrong permissions
(this is unlikely under Unix, but happens under Windows), and the
OS-independent primitives provided by the OCaml standard library
cannot test for the existence of a directory. Without this
limitation, Filename.temp_file would just loop in this case...

  • it returns the filename, not a safely opened file descriptor, so
    there is a race condition before the user opens the tempfile

The race condition is not as bad as that of mktemp() under Unix,
because Filename.temp_file creates the file atomically (using O_EXCL).
So, this avoids the classic race where two processes both test for the
non-existence of the same temp file, then both open it.

A race remains, though: Filename.temp_file creates the file, then some
other process removes that file and puts instead e.g. a symlink to
another file; then, the Caml process opens the file for writing, thus
truncating the destination of the link. However, most Unix installations
set the "t" bit on /tmp, which I believe prevent the removal of a
temporary file that you don't own. Still, this is a bit of a concern.

  • it opens with permissions of 0o666, not 0o600, so if a user has a
    silly umask, their tempfile can even be overwritten. Even with a
    normal umask like 022, their temp file will be world readable, so
    private information could possibly be read.

Right. I agree the file should be created with permission 0o600.

I recommend that Filename.temp_file be deprecated and that 1 or 2 new
functions be created in Sys that correspond to the (safer and more
secure) glibc functions mkstemp(3)/tmpfile(3).

OK, here is what I'll implement shortly:

  • Use a pseudo-random starting point for the counter, and remember it
    from call to call so that we don't re-try a counter value already used.
  • Keep the 1000 attemps limit for one call to Filename.temp_file,
    but since new names are tried each time, this won't limit the
    number of temp files created by successive calls to Filename.temp_file.
  • Create with mode 0o600.
  • Add Filename.open_temp_file that returns both the name and
    an out_channel opened (atomically) on that name.

I hope this will address your concerns.

Thanks for the feedback,

  • Xavier Leroy

@vicuna
Copy link
Author

vicuna commented Jun 11, 2002

Comment author: administrator

Implemented better temp_file and open_temp_file 2002-04-15, XL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant