Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Zack Weinberg]

On Wed, Oct 18, 2023, at 10:21 AM, KO Myung-Hun wrote:
> Zack Weinberg wrote:
>> I don’t want to have code in Autoconf that is only safe because of
>> non-obvious details of the context it’s used in.  People might
>> reuse the code in a different context where it’s *not* safe, without
>> realizing the danger.  So I suggest we use the appended patch
>> instead of your patch.  I’ve tested this on Unix systems with both
>> Perl 5.6 and Perl 5.38.  Could you please test it on your OS/2 system
>> as well?
>
> It works well here.

Great! I have committed the version below.

zw

>From c4a695510d240491f89d78204a3d5a6fdbe03648 Mon Sep 17 00:00:00 2001
From: Zack Weinberg 
Date: Wed, 18 Oct 2023 13:23:36 -0400
Subject: [PATCH] autom4te: OS/2 compat: Do not attempt to chmod an open file.

On OS/2, chmod(2) cannot be applied to an open file.

Instead set the desired permissions when the file is initially
created, using the PERMS argument to File::Temp::tempfile if
possible, or by manually emulating that feature if the system
perl does not provide a new enough version of File::Temp.

This has the nice side effect that we no longer need to handle
the umask manually.

* autom4te.in (tempfile_with_mode): New function.
  (handle_output): Use tempfile_with_mode instead of directly using
  File::Temp plus chmod.

Co-authored-by: KO Myung-Hun 
---
 bin/autom4te.in | 56 +
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/bin/autom4te.in b/bin/autom4te.in
index 71d7e6a6..41da77b0 100644
--- a/bin/autom4te.in
+++ b/bin/autom4te.in
@@ -222,6 +222,52 @@ Written by Akim Demaille.
 ## Routines.  ##
 ## -- ##
 
+# tempfile_with_mode ($dir, $mode)
+# 
+# Create a temporary file in $dir with access control bits $mode.
+# Returns a list ($fh, $fname) where $fh is a filehandle open for
+# writing to the file, and $fname is the name of the file.
+sub tempfile_with_mode ($$)
+{
+  my ($dir, $mode) = @_;
+
+  require File::Temp;
+  my $template = "actmp." . File::Temp::TEMPXXX;
+
+  # The PERMS argument was added to File::Temp::tempfile in version
+  # 0.2310 of the File::Temp module; it will be silently ignored if
+  # passed to an older version of the function.  This is the simplest
+  # way to do a non-fatal version check without features of Perl 5.10.
+  local $@;
+  if (eval { File::Temp->VERSION("0.2310"); 1 })
+{
+  # Can use PERMS argument to tempfile().
+  return File::Temp::tempfile ($template, DIR => $dir, PERMS => $mode,
+   UNLINK => 0);
+}
+  else
+{
+  # PERMS is not available.
+  # This is functionally equivalent to what it would do.
+  require Fcntl;
+  my $openflags = Fcntl::O_RDWR | Fcntl::O_CREAT | Fcntl::O_EXCL;
+
+  require File::Spec;
+  $template = File::Spec->catfile($dir, $template);
+
+  # 50 = $MAX_GUESS in File::Temp (not an exported constant).
+  for (my $i = 0; $i < 50; $i++)
+{
+  my $filename = File::Temp::mktemp($template);
+  my $fh;
+  my $success = sysopen ($fh, $filename, $openflags, $mode);
+  return ($fh, $filename) if $success;
+  fatal "Could not create temp file $filename: $!"
+unless $!{EEXIST};
+}
+  fatal "Could not create any temp file from $template: $!";
+}
+}
 
 # $OPTION
 # files_to_options (@FILE)
@@ -563,15 +609,7 @@ sub handle_output ($$)
   else
 {
   my (undef, $outdir, undef) = fileparse ($output);
-
-  use File::Temp qw (tempfile);
-  ($out, $scratchfile) = tempfile (UNLINK => 0, DIR => $outdir);
-  fatal "cannot create a file in $outdir: $!"
-unless $out;
-
-  # File::Temp doesn't give us access to 3-arg open(2), unfortunately.
-  chmod (oct ($mode) & ~(umask), $scratchfile)
-or fatal "setting mode of " . $scratchfile . ": $!";
+  ($out, $scratchfile) = tempfile_with_mode ($outdir, oct($mode));
 }
 
   my $in = new Autom4te::XFile ($ocache . $req->id, "<");
-- 
2.41.0

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Zack Weinberg]

On Tue, Oct 17, 2023, at 2:58 PM, Paul Eggert wrote:
> On 10/17/23 11:16, Zack Weinberg wrote:
...
>> you have to be exquisitely careful, or a malicious concurrent process
>> might be able to trick you into overwriting some file elsewhere on
>> the filesystem.
...
> ? If /tmp is sticky, a malicious process can't rename /tmp/foo.

I might be wrong about that specific thing.  It's been long enough that
I no longer remember the exact details, but there was a CVE reported
against GCC ... I want to say circa version 2.95 ... because it would
create temporary files with predictable names in /tmp and it was
*somehow* possible for a malicious process to substitute symlinks
pointing into /etc, and if you were running the compiler as root, which
you shouldn't but it happens all the time, boom, trashed /etc/shadow or
something equally important.

It is possible that this exploit depended on a kernel bug where the
sticky bit didn't do everything it needed to do, but since people
still want to run autoconf proper (not just configure scripts) on
ancient systems, I think we need to be careful anyway.

zw

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[KO Myung-Hun]

Hi/2.

Zack Weinberg wrote:
> On Sun, Oct 15, 2023, at 3:43 AM, KO Myung-Hun wrote:
>> Zack Weinberg wrote:
>>> On Sat, Oct 14, 2023, at 9:19 AM, KO Myung-Hun wrote:
 * bin/autom4te.in (handle_output): Ignore setting mode failure on OS/2.
>>>
>>> Not OK, for two reasons:
> ...
>> How about this ?
>> 1. create and close a temporary file
>> 2. chmod() on it
>> 3. re-open it with O_TRUNC ?
> 
> Multi-user security is probably not a concern for modern-day use of
> OS/2.  Also, the temporary files created by the code we’re talking
> about are not created in a system-wide scratch directory.  So we
> probably *could* do it this way, but I don’t like it, because it’s
> not safe in the general case.
> 
> The trouble is, on a multi-user system, any time you do any operation
> by name on a file whose full pathname includes a world-writable
> directory (such as the system-wide scratch directories), even if that
> directory is “sticky” (chmod +t), you have to be exquisitely careful,
> or a malicious concurrent process might be able to trick you into
> overwriting some file elsewhere on the filesystem.  For example, your
> steps 2 and 3, if executed as root on a file expected to exist in
> /tmp, would give a malicious concurrent process a chance to clobber
> the access control bits and/or the contents of *any file*, by moving
> the temporary file out of the way, and replacing it with a symlink,
> in between steps 1 and 2.  That’s a narrow race window to hit, but
> it has been done successfully in the past.
> 
> I don’t want to have code in Autoconf that is only safe because of
> non-obvious details of the context it’s used in.  People might
> reuse the code in a different context where it’s *not* safe, without
> realizing the danger.  So I suggest we use the appended patch
> instead of your patch.  I’ve tested this on Unix systems with both
> Perl 5.6 and Perl 5.38.  Could you please test it on your OS/2 system
> as well?
> 

It works well here.

Thanks!

-- 
KO Myung-Hun

Korean OS/2 User Community : https://www.os2.kr/

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Paul Eggert]

On 10/17/23 11:16, Zack Weinberg wrote:

On Sun, Oct 15, 2023, at 3:43 AM, KO Myung-Hun wrote:

How about this ?
1. create and close a temporary file
2. chmod() on it
3. re-open it with O_TRUNC ?


The trouble is, on a multi-user system, any time you do any operation
by name on a file whose full pathname includes a world-writable
directory (such as the system-wide scratch directories), even if that
directory is “sticky” (chmod +t), you have to be exquisitely careful,
or a malicious concurrent process might be able to trick you into
overwriting some file elsewhere on the filesystem.  For example, your
steps 2 and 3, if executed as root on a file expected to exist in
/tmp, would give a malicious concurrent process a chance to clobber
the access control bits and/or the contents of *any file*, by moving
the temporary file out of the way


? If /tmp is sticky, a malicious process can't rename /tmp/foo.

The rest of your email and patch look good to me, though admittedly I 
haven't used perl for real in 30 years.

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Zack Weinberg]

On Sun, Oct 15, 2023, at 3:43 AM, KO Myung-Hun wrote:
> Zack Weinberg wrote:
>> On Sat, Oct 14, 2023, at 9:19 AM, KO Myung-Hun wrote:
>>> * bin/autom4te.in (handle_output): Ignore setting mode failure on OS/2.
>>
>> Not OK, for two reasons:
...
> How about this ?
> 1. create and close a temporary file
> 2. chmod() on it
> 3. re-open it with O_TRUNC ?

Multi-user security is probably not a concern for modern-day use of
OS/2.  Also, the temporary files created by the code we’re talking
about are not created in a system-wide scratch directory.  So we
probably *could* do it this way, but I don’t like it, because it’s
not safe in the general case.

The trouble is, on a multi-user system, any time you do any operation
by name on a file whose full pathname includes a world-writable
directory (such as the system-wide scratch directories), even if that
directory is “sticky” (chmod +t), you have to be exquisitely careful,
or a malicious concurrent process might be able to trick you into
overwriting some file elsewhere on the filesystem.  For example, your
steps 2 and 3, if executed as root on a file expected to exist in
/tmp, would give a malicious concurrent process a chance to clobber
the access control bits and/or the contents of *any file*, by moving
the temporary file out of the way, and replacing it with a symlink,
in between steps 1 and 2.  That’s a narrow race window to hit, but
it has been done successfully in the past.

I don’t want to have code in Autoconf that is only safe because of
non-obvious details of the context it’s used in.  People might
reuse the code in a different context where it’s *not* safe, without
realizing the danger.  So I suggest we use the appended patch
instead of your patch.  I’ve tested this on Unix systems with both
Perl 5.6 and Perl 5.38.  Could you please test it on your OS/2 system
as well?

zw

---
diff --git a/bin/autom4te.in b/bin/autom4te.in
index 71d7e6a6..41da77b0 100644
--- a/bin/autom4te.in
+++ b/bin/autom4te.in
@@ -222,6 +222,52 @@ Written by Akim Demaille.
 ## Routines.  ##
 ## -- ##
 
+# tempfile_with_mode ($dir, $mode)
+# 
+# Create a temporary file in $dir with access control bits $mode.
+# Returns a list ($fh, $fname) where $fh is a filehandle open for
+# writing to the file, and $fname is the name of the file.
+sub tempfile_with_mode ($$)
+{
+  my ($dir, $mode) = @_;
+
+  require File::Temp;
+  my $template = "actmp." . File::Temp::TEMPXXX;
+
+  # The PERMS argument was added to File::Temp::tempfile in version
+  # 0.2310 of the File::Temp module; it will be silently ignored if
+  # passed to an older version of the function.  This is the simplest
+  # way to do a non-fatal version check without features of Perl 5.10.
+  local $@;
+  if (eval { File::Temp->VERSION("0.2310"); 1 })
+{
+  # Can use PERMS argument to tempfile().
+  return File::Temp::tempfile ($template, DIR => $dir, PERMS => $mode,
+   UNLINK => 0);
+}
+  else
+{
+  # PERMS is not available.
+  # This is functionally equivalent to what it would do.
+  require Fcntl;
+  my $openflags = Fcntl::O_RDWR | Fcntl::O_CREAT | Fcntl::O_EXCL;
+
+  require File::Spec;
+  $template = File::Spec->catfile($dir, $template);
+
+  # 50 = $MAX_GUESS in File::Temp (not an exported constant).
+  for (my $i = 0; $i < 50; $i++)
+{
+  my $filename = File::Temp::mktemp($template);
+  my $fh;
+  my $success = sysopen ($fh, $filename, $openflags, $mode);
+  return ($fh, $filename) if $success;
+  fatal "Could not create temp file $filename: $!"
+unless $!{EEXIST};
+}
+  fatal "Could not create any temp file from $template: $!";
+}
+}
 
 # $OPTION
 # files_to_options (@FILE)
@@ -563,15 +609,7 @@ sub handle_output ($$)
   else
 {
   my (undef, $outdir, undef) = fileparse ($output);
-
-  use File::Temp qw (tempfile);
-  ($out, $scratchfile) = tempfile (UNLINK => 0, DIR => $outdir);
-  fatal "cannot create a file in $outdir: $!"
-unless $out;
-
-  # File::Temp doesn't give us access to 3-arg open(2), unfortunately.
-  chmod (oct ($mode) & ~(umask), $scratchfile)
-or fatal "setting mode of " . $scratchfile . ": $!";
+  ($out, $scratchfile) = tempfile_with_mode ($outdir, oct($mode));
 }
 
   my $in = new Autom4te::XFile ($ocache . $req->id, "<");
---

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Zack Weinberg]

On Sat, Oct 14, 2023, at 1:27 PM, Russ Allbery wrote:
> The standard Perl command corelist -a  tells you the versions of
> that module that shipped with each version of Perl.  The first version of
> Perl that included a version of File::Temp later than 0.2310 was Perl
> 5.33.3 (which included 0.2311).

Thank you; I had no idea that command existed.

zw

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[KO Myung-Hun]

Hi/2.

Zack Weinberg wrote:
> On Sat, Oct 14, 2023, at 9:19 AM, KO Myung-Hun wrote:
>> OS/2 does not allow chmod() on an opened file.
>>
>> * bin/autom4te.in (handle_output): Ignore setting mode failure on OS/2.
> 
> Not OK, for two reasons: (1) IIRC this is used to create scripts with the
> execute bit set in at least one place.  Ignoring the chmod failure will
> cause confusing errors downstream of where the problem actually was.  (2)
> Having a temporary file that's potentially writable by other user ids,
> even briefly, risks a variety of security problems.
> 
> Also, for maintainability's sake, no checks of $^O anywhere in autoconf's
> Perl code unless there is absolutely no alternative.
> 

How about this ?

1. create and close a temporary file
2. chmod() on it
3. re-open it with O_TRUNC ?

-- 
KO Myung-Hun

Korean OS/2 User Community : https://www.os2.kr/

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Russ Allbery]

"Zack Weinberg"  writes:

> Really what we need here is for File::Temp to allow us to supply the
> third argument to the open() system call.  That feature was added in
> version 0.2310 of the File::Temp module.  Does anyone have time right
> now to do the archaeology on what version of File::Temp shipped with the
> oldest version of Perl we currently support?

The standard Perl command corelist -a  tells you the versions of
that module that shipped with each version of Perl.  The first version of
Perl that included a version of File::Temp later than 0.2310 was Perl
5.33.3 (which included 0.2311).

-- 
Russ Allbery (ea...@eyrie.org) 

Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[Zack Weinberg]

On Sat, Oct 14, 2023, at 9:19 AM, KO Myung-Hun wrote:
> OS/2 does not allow chmod() on an opened file.
>
> * bin/autom4te.in (handle_output): Ignore setting mode failure on OS/2.

Not OK, for two reasons: (1) IIRC this is used to create scripts with the
execute bit set in at least one place.  Ignoring the chmod failure will
cause confusing errors downstream of where the problem actually was.  (2)
Having a temporary file that's potentially writable by other user ids,
even briefly, risks a variety of security problems.

Also, for maintainability's sake, no checks of $^O anywhere in autoconf's
Perl code unless there is absolutely no alternative.

Really what we need here is for File::Temp to allow us to supply the third
argument to the open() system call.  That feature was added in version
0.2310 of the File::Temp module.  Does anyone have time right now to do the
archaeology on what version of File::Temp shipped with the oldest version of
Perl we currently support?

zw

[PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2

[KO Myung-Hun]

OS/2 does not allow chmod() on an opened file.

* bin/autom4te.in (handle_output): Ignore setting mode failure on OS/2.
---
 bin/autom4te.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/bin/autom4te.in b/bin/autom4te.in
index 71d7e6a6..ac5fe18b 100644
--- a/bin/autom4te.in
+++ b/bin/autom4te.in
@@ -571,7 +571,8 @@ sub handle_output ($$)
 
   # File::Temp doesn't give us access to 3-arg open(2), unfortunately.
   chmod (oct ($mode) & ~(umask), $scratchfile)
-or fatal "setting mode of " . $scratchfile . ": $!";
+or fatal "setting mode of " . $scratchfile . ": $!"
+ unless $^O eq 'os2';
 }
 
   my $in = new Autom4te::XFile ($ocache . $req->id, "<");
-- 
2.42.0