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

[PATCH] Coverity: safe umask before mkstemp #13773

Closed
p5pRT opened this issue Apr 26, 2014 · 6 comments
Closed

[PATCH] Coverity: safe umask before mkstemp #13773

p5pRT opened this issue Apr 26, 2014 · 6 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 26, 2014

Migrated from rt.perl.org#121742 (status was 'resolved')

Searchable as RT121742$

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2014

From @jhi

Use safe umask (0600) before mkstemp. Modern glibcs use a safe umask,
but older ones don't, and on non-glibc systems better play safe.

Attached.

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2014

From @jhi

0001-Fix-for-Coverity-perl5-CID-29068.patch
From c9ec3abfc6deeaf1683f26120b9de7f56580d1e5 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Thu, 24 Apr 2014 12:23:18 -0400
Subject: [PATCH] Fix for Coverity perl5 CID 29068: Insecure temporary file
 (SECURE_TEMP) secure_temp: Calling mkstemp() without securely setting umask
 first.

The umask used for mkstemp should be secure, but umask 0600 has been
the required umask only since POSIX.1-2008.  In glibc 2.06 and earlier
the default was 0666, which is not secure.  And no explicit knowledge
of how well non-glibc platforms implement mkstemp.  Better err on the
side security, so set the umask temporarily to 0600, and then restore it.
---
 perl.c   | 2 ++
 perlio.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/perl.c b/perl.c
index 27d0d9e..f5007f7 100644
--- a/perl.c
+++ b/perl.c
@@ -3763,7 +3763,9 @@ S_open_script(pTHX_ const char *scriptname, bool dosearch, bool *suidscript)
 	const char * const err = "Failed to create a fake bit bucket";
 	if (strEQ(scriptname, BIT_BUCKET)) {
 #ifdef HAS_MKSTEMP /* Hopefully mkstemp() is safe here. */
+            int old_umask = umask(0600);
 	    int tmpfd = mkstemp(tmpname);
+            umask(old_umask);
 	    if (tmpfd > -1) {
 		scriptname = tmpname;
 		close(tmpfd);
diff --git a/perlio.c b/perlio.c
index 0ae0a43..9b466b6 100644
--- a/perlio.c
+++ b/perlio.c
@@ -4961,6 +4961,7 @@ PerlIO_tmpfile(void)
      char tempname[] = "/tmp/PerlIO_XXXXXX";
      const char * const tmpdir = TAINTING_get ? NULL : PerlEnv_getenv("TMPDIR");
      SV * sv = NULL;
+     int old_umask = umask(0600);
      /*
       * I have no idea how portable mkstemp() is ... NI-S
       */
@@ -4982,6 +4983,7 @@ PerlIO_tmpfile(void)
          sv_catpv(sv, tempname + 4);
          fd = mkstemp(SvPVX(sv));
      }
+     umask(old_umask);
      if (fd >= 0) {
 	  f = PerlIO_fdopen(fd, "w+");
 	  if (f)
-- 
1.9.2

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

From @tonycoz

On Sat Apr 26 12​:56​:06 2014, jhi wrote​:

Use safe umask (0600) before mkstemp. Modern glibcs use a safe umask,
but older ones don't, and on non-glibc systems better play safe.

Attached.

While I think the patch is good, threaded code that uses C<< open my $fh, "+>", undef >> might see some intermittent unexpected permissions on files created in other threads.

Unfortunately umask() updates the mask for the whole process and there's no thread-local variant, so I don't see a thread-safe solution.

Added to the 5.21.1 list.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

From @tsee

Thanks, applied as 60f7fc1.

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

@tsee - Status changed from 'open' to 'resolved'

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

1 participant