mkstemp(3) and umask

[p5pRT]

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

Searchable as RT127322$

[p5pRT]

From @ntyni

Hi, I believe there's a minor security issue around mkstemp(3) usage in
Perl, mainly 5.21.1 and newer.

With commit v5.21.0-67-g60f7fc1
http​://perl5.git.perl.org/perl.git/commitdiff/60f7fc1ea42054e92f34b4ce9d608efd14357392
perl started setting umask to 0600 before calling mkstemp(3),
and then restoring it afterwards.

This looks like a logical error as it tells open(2) to strip the
owner read and write bits from the given mode before applying it.
I think umask(0177) is what was intended.

The mkstemp(3) function uses mode 0600 on at least modern GNU/Linux
systems, so the resulting file becomes mode 0000. This is not
a security problem, but it might be a usability problem - see
https://bugs.debian.org/810924 which prompted this report.

However, my mkstemp(3) manual page says that "the application should
make sure its file mode creation mask (see umask(2)) is set appropriately
before calling mkstemp()", and mentions ancient versions of glibc using
mode 0666.

Since the above commit, systems using mode 0666 in mkstemp(3) would
get a file with 0066 (---rw-rw-) regardless of the original umask. This
looks like a security problem, although the window of exploitation seems
to be very small, as both relevant code paths in Perl unlink the file
soon afterwards.

The first code path in perl.c seems to be for some very obscure systems
without /dev/null, but the other one in perlio.c is for things like
'open(my $fh, "+>", undef)' which are quite normal usage.

So, it looks to me like the error has potential security implications for
systems whose mkstemp(3) uses something else than mode 0600. I don't know
if those exist, and the impact seems to be very small, but I'm reporting
this as a security bug to be on the safe side. Feel free to turn this
into a normal bug report if you think that's overly cautious.

Many thanks for your work on Perl,
--
Niko Tyni ntyni@​debian.org

[p5pRT]

From @jhi

On Tuesday, January 19, 2016, Niko Tyni <perl5-security-report@​perl.org>
wrote​:

# New Ticket Created by Niko Tyni
# Please include the string​: [perl #127322]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127322 >

Hi, I believe there's a minor security issue around mkstemp(3) usage in
Perl, mainly 5.21.1 and newer.

With commit v5.21.0-67-g60f7fc1

http​://perl5.git.perl.org/perl.git/commitdiff/60f7fc1ea42054e92f34b4ce9d608efd14357392
perl started setting umask to 0600 before calling mkstemp(3),
and then restoring it afterwards.

This looks like a logical error as it tells open(2) to strip the
owner read and write bits from the given mode before applying it.
I think umask(0177) is what was intended.

Duh, yes. My bad. Forgot the NOT taking place.

The mkstemp(3) function uses mode 0600 on at least modern GNU/Linux
systems, so the resulting file becomes mode 0000. This is not
a security problem, but it might be a usability problem - see
https://bugs.debian.org/810924 which prompted this report.

However, my mkstemp(3) manual page says that "the application should
make sure its file mode creation mask (see umask(2)) is set appropriately
before calling mkstemp()", and mentions ancient versions of glibc using
mode 0666.

Since the above commit, systems using mode 0666 in mkstemp(3) would
get a file with 0066 (---rw-rw-) regardless of the original umask. This
looks like a security problem, although the window of exploitation seems
to be very small, as both relevant code paths in Perl unlink the file
soon afterwards.

The first code path in perl.c seems to be for some very obscure systems
without /dev/null, but the other one in perlio.c is for things like
'open(my $fh, "+>", undef)' which are quite normal usage.

So, it looks to me like the error has potential security implications for
systems whose mkstemp(3) uses something else than mode 0600. I don't know
if those exist, and the impact seems to be very small, but I'm reporting
this as a security bug to be on the safe side. Feel free to turn this
into a normal bug report if you think that's overly cautious.

Many thanks for your work on Perl,
--
Niko Tyni ntyni@​debian.org <javascript​:;>

--
There is this special biologist word we use for 'stable'. It is 'dead'. --
Jack Cohen

[p5pRT]

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

[p5pRT]

From @ntyni

On Tue, Jan 19, 2016 at 03​:21​:22PM -0800, Jarkko Hietaniemi via RT wrote​:

On Tuesday, January 19, 2016, Niko Tyni <perl5-security-report@​perl.org>
wrote​:

# New Ticket Created by Niko Tyni
# Please include the string​: [perl #127322]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127322 >

Hi, I believe there's a minor security issue around mkstemp(3) usage in
Perl, mainly 5.21.1 and newer.

With commit v5.21.0-67-g60f7fc1

http​://perl5.git.perl.org/perl.git/commitdiff/60f7fc1ea42054e92f34b4ce9d608efd14357392
perl started setting umask to 0600 before calling mkstemp(3),
and then restoring it afterwards.

This looks like a logical error as it tells open(2) to strip the
owner read and write bits from the given mode before applying it.
I think umask(0177) is what was intended.

Duh, yes. My bad. Forgot the NOT taking place.

Thanks for the confirmation. Trivial proposed patch attached.
--
Niko Tyni ntyni@​debian.org

[p5pRT]

From @ntyni

0001-Fix-umask-for-mkstemp-3-calls.patch

From 8bba0c2f7b8382d83ef4d5e508e81b6b3ea4f705 Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Thu, 21 Jan 2016 18:17:32 +0200
Subject: [PATCH] Fix umask for mkstemp(3) calls

With commit v5.21.0-67-g60f7fc1, perl started setting umask to 0600
before calling mkstemp(3), and then restoring it afterwards. This is
wrong as it tells open(2) to strip the owner read and write bits from
the given mode before applying it, rather than the intended negation of
leaving only those bits in place.

On modern systems which call open(2) with mode 0600 in mkstemp(3),
this clears all the created temporary file permissions. However,
any systems that use mode 0666 in mkstemp(3) (like ancient versions
of glibc) now create a file with permissions 0066, leaving world
read and write permission regardless of current umask.

Using umask 0177 instead fixes this.

Bug: https://rt.perl.org/Ticket/Display.html?id=127322
---
 perl.c   | 2 +-
 perlio.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/perl.c b/perl.c
index 17de92c..b557d01 100644
--- a/perl.c
+++ b/perl.c
@@ -3785,7 +3785,7 @@ 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 old_umask = umask(0177);
 	    int tmpfd = mkstemp(tmpname);
             umask(old_umask);
 	    if (tmpfd > -1) {
diff --git a/perlio.c b/perlio.c
index 69f3755..11a66d0 100644
--- a/perlio.c
+++ b/perlio.c
@@ -5009,7 +5009,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);
+     int old_umask = umask(0177);
      /*
       * I have no idea how portable mkstemp() is ... NI-S
       */
-- 
2.7.0.rc3

[p5pRT]

From @jhi

I am prepared just to apply Niko's suggested patch (while noting the
obvious need to have this in 5.22.2) and not bother with CVE-ing this
issue. But then again, I'm quite biased against not making a big
fuss about this, for obvious selfish reasons. Anyone less partial
might to weigh in?

On Tuesday-201601-19 15​:02, Niko Tyni (via RT) wrote​:

# New Ticket Created by Niko Tyni
# Please include the string​: [perl #127322]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127322 >

Hi, I believe there's a minor security issue around mkstemp(3) usage in
Perl, mainly 5.21.1 and newer.

With commit v5.21.0-67-g60f7fc1
http​://perl5.git.perl.org/perl.git/commitdiff/60f7fc1ea42054e92f34b4ce9d608efd14357392
perl started setting umask to 0600 before calling mkstemp(3),
and then restoring it afterwards.

This looks like a logical error as it tells open(2) to strip the
owner read and write bits from the given mode before applying it.
I think umask(0177) is what was intended.

The mkstemp(3) function uses mode 0600 on at least modern GNU/Linux
systems, so the resulting file becomes mode 0000. This is not
a security problem, but it might be a usability problem - see
https://bugs.debian.org/810924 which prompted this report.

However, my mkstemp(3) manual page says that "the application should
make sure its file mode creation mask (see umask(2)) is set appropriately
before calling mkstemp()", and mentions ancient versions of glibc using
mode 0666.

Since the above commit, systems using mode 0666 in mkstemp(3) would
get a file with 0066 (---rw-rw-) regardless of the original umask. This
looks like a security problem, although the window of exploitation seems
to be very small, as both relevant code paths in Perl unlink the file
soon afterwards.

The first code path in perl.c seems to be for some very obscure systems
without /dev/null, but the other one in perlio.c is for things like
'open(my $fh, "+>", undef)' which are quite normal usage.

So, it looks to me like the error has potential security implications for
systems whose mkstemp(3) uses something else than mode 0600. I don't know
if those exist, and the impact seems to be very small, but I'm reporting
this as a security bug to be on the safe side. Feel free to turn this
into a normal bug report if you think that's overly cautious.

Many thanks for your work on Perl,

[p5pRT]

From @rjbs

* Jarkko Hietaniemi <jhi@​iki.fi> [2016-01-24T11​:12​:28]

I am prepared just to apply Niko's suggested patch (while noting the obvious
need to have this in 5.22.2) and not bother with CVE-ing this issue. But
then again, I'm quite biased against not making a big
fuss about this, for obvious selfish reasons. Anyone less partial
might to weigh in?

I'm partial, too, because CVEs are a big pain. :-) Nonetheless, I think this
is not CVE-worthy. Unless there are objections in the next few days, go ahead
and push it. Friday the 29th?

--
rjbs

[p5pRT]

From @jhi

On Monday-201601-25 23​:47, Ricardo Signes wrote​:

* Jarkko Hietaniemi <jhi@​iki.fi> [2016-01-24T11​:12​:28]

I am prepared just to apply Niko's suggested patch (while noting the obvious
need to have this in 5.22.2) and not bother with CVE-ing this issue. But
then again, I'm quite biased against not making a big
fuss about this, for obvious selfish reasons. Anyone less partial
might to weigh in?

I'm partial, too, because CVEs are a big pain. :-) Nonetheless, I think this
is not CVE-worthy. Unless there are objections in the next few days, go ahead
and push it. Friday the 29th?

Sounds like a plan to me.

[p5pRT]

From @jhi

On Tue Jan 26 03​:34​:41 2016, jhi wrote​:

On Monday-201601-25 23​:47, Ricardo Signes wrote​:

* Jarkko Hietaniemi <jhi@​iki.fi> [2016-01-24T11​:12​:28]

I am prepared just to apply Niko's suggested patch (while noting the
obvious
need to have this in 5.22.2) and not bother with CVE-ing this issue.
But
then again, I'm quite biased against not making a big
fuss about this, for obvious selfish reasons. Anyone less partial
might to weigh in?

I'm partial, too, because CVEs are a big pain. :-) Nonetheless, I
think this
is not CVE-worthy. Unless there are objections in the next few days,
go ahead
and push it. Friday the 29th?

Sounds like a plan to me.

http​://perl5.git.perl.org/perl.git/commit/e57270be442bfaa9dc23eebd67485e5a806b44e3

[p5pRT]

From @rjbs

Thanks, I have moved this ticket to perl5 queue and marked it pending release.

--
rjbs

[p5pRT]

@rjbs - Status changed from 'open' to 'pending release'

[p5pRT]

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
 
With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

[p5pRT]

@khwilliamson - Status changed from 'pending release' to 'resolved'