suid perl and 5.8.4

[Nicholas Clark]

We upgraded perl on a machine at work. qmail broke.

Eh? How! qmail is written in C.

It transpires that it was actually qmail-scanner that was failing, because
qmail-scanner-queue was a perl script that was calling sperl directly.
In this case this results in qmail bouncing messages, and for quite a time
as there's little obvious connection between the mail system and perl.
In fact, it would taken quite a lot longer had google not found reports of
similar problems:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=240687

I don't want to ship a stable release that isn't "stable" because it breaks
seemingly unrelated things. However, I can see why the new "must invoke perl,
perl invokes sperl" logic is the right thing to do security wise.

James Bromberger suggested a way round this. Compared with what we have in
5.8.4-RC1, is it any less secure to

a: name the set UI perl binary something new (ie something that should not
currently be in any #! lines)
b: make perl exec this
c: make sperl a copy of regular perl (ie same binary, not set ID)

This keeps the security model the same, but (I think) means that anything
currently invoking /usr/bin/sperl will continue to work as before.

I've applied the appended patch to maint, on the assumption that this is
correct. (dangerous assumption). If this change does deliver compatibility
without breaking backwards compatibility, I propose to make a 5.8.4 RC2 with
it in, and throw that to the lions for (more) testing.

If it does not, then I'm not sure what to do.

Nicholas Clark

Change 22694 by nicholas@entropy on 2004/04/14 13:47:19

For maint, I don't think that we can have sperl go sulky if invoked
from a #! line. So keep .../sperl functional, and have the /dev/fd/...
only set ID exectuable reside under a different name (suidperl)

Affected files ...

... //depot/maint-5.8/perl/installperl#21 edit
... //depot/maint-5.8/perl/perl.c#72 edit

Differences ...

==== //depot/maint-5.8/perl/installperl#21 (xtext) ====

@@ -366,9 +366,18 @@
}

safe_unlink("$installbin/s$perl_verbase$ver$exe_ext");
+safe_unlink("$installbin/suid$perl_verbase$ver$exe_ext");
if ($d_dosuid) {
- copy("suidperl$exe_ext", "$installbin/s$perl_verbase$ver$exe_ext");
- chmod(04711, "$installbin/s$perl_verbase$ver$exe_ext");
+ # now that the set UID perl must be invoked with /dev/fd/... we need to
+ # give it a different name for maintenance releases, as there are too many
+ # entrenched scripts which call sperl directly. So the suid backend is
+ # suidperl
+ copy("suidperl$exe_ext", "$installbin/suid$perl_verbase$ver$exe_ext");
+ chmod(04711, "$installbin/suid$perl_verbase$ver$exe_ext");
+ # and we provide a "sperl" which will keep on working the way these scripts
+ # expect.
+ link("$installbin/$perl_verbase$ver$exe_ext",
+ "$installbin/s$perl_verbase$ver$exe_ext");
}

# Install library files.

==== //depot/maint-5.8/perl/perl.c#72 (text) ====

@@ -3111,7 +3111,9 @@
}
#ifdef IAMSUID
else {
- Perl_croak(aTHX_ "suidperl needs fd script\n");
+ Perl_croak(aTHX_ "suidperl needs fd script\n"
+ "You should not call suidperl directly; do you need to "
+ "change a #! line\nfrom suidperl to perl?\n");
/* PSz 11 Nov 03
* Do not open (or do other fancy stuff) while setuid.
* Perl does the open, and hands script to suidperl on a fd;
@@ -3357,7 +3359,7 @@
* in perl will not fix that problem, but if you have disabled setuid
* scripts in the kernel, this will attempt to emulate setuid and setgid
* on scripts that have those now-otherwise-useless bits set. The setuid
- * root version must be called suidperl or sperlN.NNN. If regular perl
+ * root version must be called suidperl or suidperlN.NNN. If regular perl
* discovers that it has opened a setuid script, it calls suidperl with
* the same argv that it had. If suidperl finds that the script it has
* just opened is NOT setuid root, it sets the effective uid back to the
@@ -3366,7 +3368,7 @@
* uid.
* PSz 27 Feb 04
* Description/comments above do not match current workings:
- * suidperl must be hardlinked to sperlN.NNN (that is what we exec);
+ * suidperl must be hardlinked to suidperlN.NNN (that is what we exec);
* suidperl called with script open and name changed to /dev/fd/N/X;
* suidperl croaks if script is not setuid;
* making perl setuid would be a huge security risk (and yes, that
@@ -3387,8 +3389,10 @@
STRLEN n_a;

#ifdef IAMSUID
- if (PL_fdscript < 0 || PL_suidscript != 1)
- Perl_croak(aTHX_ "Need (suid) fdscript in suidperl\n"); /* We already checked this */
+ if (PL_fdscript < 0 || PL_suidscript != 1) {
+ Perl_croak(aTHX_ "Need (suid) fdscript in suidperl\n");
+ /* We already checked this */
+ }
/* PSz 11 Nov 03
* Since the script is opened by perl, not suidperl, some of these
* checks are superfluous. Leaving them in probably does not lower
@@ -3542,7 +3546,7 @@
PL_euid) { /* oops, we're not the setuid root perl */
/* PSz 18 Feb 04
* When root runs a setuid script, we do not go through the same
- * steps of execing sperl and then perl with fd scripts, but
+ * steps of execing suidperl and then perl with fd scripts, but
* simply set up UIDs within the same perl invocation; so do
* not have the same checks (on options, whatever) that we have
* for plain users. No problem really: would have to be a script
@@ -3586,12 +3590,13 @@
fcntl(PerlIO_fileno(PL_rsfp),F_SETFD,0); /* ensure no close-on-exec */
#endif
PERL_FPU_PRE_EXEC
- PerlProc_execv(Perl_form(aTHX_ "%s/sperl"PERL_FS_VER_FMT, BIN_EXP,
- (int)PERL_REVISION, (int)PERL_VERSION,
- (int)PERL_SUBVERSION), PL_origargv);
+ PerlProc_execv(Perl_form(aTHX_ "%s/suidperl"PERL_FS_VER_FMT,
+ BIN_EXP, (int)PERL_REVISION,
+ (int)PERL_VERSION, (int)PERL_SUBVERSION),
+ PL_origargv);
PERL_FPU_POST_EXEC
#endif /* IAMSUID */
- Perl_croak(aTHX_ "Can't do setuid (cannot exec sperl)\n");
+ Perl_croak(aTHX_ "Can't do setuid (cannot exec suidperl)\n");
}

if (PL_statbuf.st_mode & S_ISGID && PL_statbuf.st_gid != PL_egid) {

[Paul Szabo]

Dear Nick,

> We upgraded perl on a machine at work. qmail broke.

> ...
> ... qmail-scanner ... was calling sperl directly.
> ... reports of similar problems:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=240687
> ...
> James Bromberger suggested ...

> a: name the set UI perl binary something new (ie something that should not
> currently be in any #! lines)
> b: make perl exec this
> c: make sperl a copy of regular perl (ie same binary, not set ID)
> This keeps the security model the same, but (I think) means that anything
> currently invoking /usr/bin/sperl will continue to work as before.

Yes, that solution is safe, and would allow scripts with #!sperl to
continue working. (Never mind that people were never meant to use sperl or
suidperl directly...)

Note however that you seem to have chosen "suidperl" as the "new" name, and
the above referenced bug report indicates that qmail used precisely that.
For backwards compatibility with such broken software, need a different,
"really new" name: I suggest setuidperl; and make both sperl and suidperl
copies of (or hardlinks or symlinks to) perl.

Cheers,

Paul Szabo - p...@maths.usyd.edu.au http://www.maths.usyd.edu.au:8000/u/psz/
School of Mathematics and Statistics University of Sydney 2006 Australia

[Nicholas Clark]

On Thu, Apr 15, 2004 at 05:44:34AM +1000, Paul Szabo wrote:

> Note however that you seem to have chosen "suidperl" as the "new" name, and
> the above referenced bug report indicates that qmail used precisely that.
> For backwards compatibility with such broken software, need a different,
> "really new" name: I suggest setuidperl; and make both sperl and suidperl
> copies of (or hardlinks or symlinks to) perl.

Well, I didn't think that it did, but I checked and it does. There seems to be
a lot of confusion here - if perl is configured to make the set uid perl
then it compiles it as suidperl in the build tree. But the installer copies
that into the install tree as sperl. (and this has been so since the start
of perforce - 5.003). The configuration system for qmail-scanner also
goes hunting explicitly for "suidperl" but not for "sperl", so it seems that
out in the real world the former name is more common. Or maybe there are
only two operating systems - Debian and FreeBSD :-)

However, it seems that both Debian and FreeBSD have chosen to override that
behaviour (or roll their own installing) and install the set uid perl with
the name suidperl.

So you're right. "setuidperl" is also better than anything I can think of.
However, I don't think I should to change the install script further than
this - if someone is installing over a 5.8.0 or later built from source they
won't have a suidperl anywhere. And the vendors' maintainers will know what
to do to keep their systems consistent.

Thanks.

Nicholas Clark

[Nicholas Clark]

On Thu, Apr 15, 2004 at 03:52:43PM +0100, Nicholas Clark wrote:
> However, it seems that both Debian and FreeBSD have chosen to override that
> behaviour (or roll their own installing) and install the set uid perl with
> the name suidperl.

No, I'm just being stupid. I was searching for suidperl in the install
script. Therefore I failed to find suid$perl
suidperl is created by the perl install script. Silly me. As a hardlink to
sperlN.NN
So now it's a hardlink to perl, which works.

Nicholas Clark

[Brendan O'Dea]

Agreed, that's all that's required--change installperl:

- link("$installbin/s$perl_verbase$ver$exe_ext",
+ link("$installbin/$perl_verbase$ver$exe_ext",
"$installbin/suid$perl$exe_ext")
if $d_dosuid;

I can't quite see what the s/s(?=perl)/setuid/ change buys other than a
bigger patch and would echo your own statement from the RC2 announce:
"All this suidperl changing makes me uneasy".

The only bug reports I've seen relate to uses of #!/usr/bin/suidperl, not
#!/usr/bin/sperl5.8.3 which will still break whether you prefix it with
sperl or setuidperl.

I would prefer to see #22700 rolled back, in favour of the simple change
above.

--bod

[Paul Szabo]

Bod wrote:

> I can't quite see what the s/s(?=perl)/setuid/ change buys other than a
> bigger patch and would echo your own statement from the RC2 announce:
> "All this suidperl changing makes me uneasy".
>
> The only bug reports I've seen relate to uses of #!/usr/bin/suidperl, not
> #!/usr/bin/sperl5.8.3 which will still break whether you prefix it with
> sperl or setuidperl.

I am confused: why should #!/usr/bin/sperl5.8.3 break in any way? (When
you say prefixed: do you mean linked to?)

The way I see things: there is a perl binary, and a s(et)uidperl binary.
There are (sym? hard?) links to (or copies of) the perl binary under
various names; similarly there may be names for the s(et)uidperl binary.
Calling any of the perl names will work fine, while calling directly
(e.g. in #! constructs) any of the s(et)uidperl names will "break".
Were not sperl and sperl5.8.3 both going to be names for perl?

[Brendan O'Dea]

On Sun, Apr 18, 2004 at 05:26:01PM +1000, Paul Szabo wrote:
>Bod wrote:
>> The only bug reports I've seen relate to uses of #!/usr/bin/suidperl, not
>> #!/usr/bin/sperl5.8.3 which will still break whether you prefix it with
>> sperl or setuidperl.
>
>I am confused: why should #!/usr/bin/sperl5.8.3 break in any way? (When
>you say prefixed: do you mean linked to?)

Breaks in that you can't run a script with that as the first line--it
dies with "suidperl needs fd script", and will do so whether the setuid
binary name starts with (prefixed) "sperl" or "setuidperl".

>The way I see things: there is a perl binary, and a s(et)uidperl binary.
>There are (sym? hard?) links to (or copies of) the perl binary under
>various names; similarly there may be names for the s(et)uidperl binary.
>Calling any of the perl names will work fine, while calling directly
>(e.g. in #! constructs) any of the s(et)uidperl names will "break".
>Were not sperl and sperl5.8.3 both going to be names for perl?

Currently we have:

perl5.8.3 -> perl
sperl5.8.3 -> suidperl

i.e. there is are two basic binaries, fully qualified with the version
number: "perl5.8.3" and "sperl5.8.3", the latter of which is set uid.

There are also hard links to the canonical names "perl" and "suidperl".

Since #!/usr/bin/suidperl does not work when set uid, Nick has changed
the link in 5.8.4-RC2 so that such usage is equivalent to #!/usr/bin/perl
which should preserve existing behaviour:

perl5.8.4 -> perl, suidperl
setuidperl5.8.4

Note however that the name of the set uid binary has also been changed
from "sperlN.N.N" to "setuidperlN.N.N", a change which I don't believe
is required or desirable.

--bod

[Paul Szabo]

Dear bod,

>>I am confused ...

>
> Currently we have:
>
> perl5.8.3 -> perl
> sperl5.8.3 -> suidperl
>

> ... 5.8.4-RC2 [will have]:

>
> perl5.8.4 -> perl, suidperl
> setuidperl5.8.4
>
> Note however that the name of the set uid binary has also been changed
> from "sperlN.N.N" to "setuidperlN.N.N", a change which I don't believe
> is required or desirable.

Thanks for clearing that up.

You expect that the name /usr/bin/sperl is not used anywhere; but e.g.

http://rpm.pbone.net/index.php3/stat/6/idpl/1088583

seems to suggest otherwise. It would be confusing if they had sperlX.Y.Z
refer to the s(et)uidperl binary and sperl refer to perl. Maybe any such
(broken?) distributions could rename things appropriately. It seems cleaner
to give the s(et)uidperl binary a name that has not been used previously
(wrongly, traditionally).

[Nicholas Clark]

On Mon, Apr 19, 2004 at 10:10:41AM +1000, Paul Szabo wrote:
> Dear bod,
>
> >>I am confused ...
> >
> > Currently we have:
> >
> > perl5.8.3 -> perl
> > sperl5.8.3 -> suidperl
> >
> > ... 5.8.4-RC2 [will have]:
> >
> > perl5.8.4 -> perl, suidperl
> > setuidperl5.8.4
> >
> > Note however that the name of the set uid binary has also been changed
> > from "sperlN.N.N" to "setuidperlN.N.N", a change which I don't believe
> > is required or desirable.
>
> Thanks for clearing that up.

Thanks also. I hadn't grasped this until your explanation.
I agree that it's not required, given that the only problem we see is
scripts using and probing for "suidperl"
(and that there is no existing script with sperl5.8.4 in its #! line,
and we're not changing existing sperl5.8.3 binary so not breaking anything
existing with that in its #! line)

> You expect that the name /usr/bin/sperl is not used anywhere; but e.g.
>
> http://rpm.pbone.net/index.php3/stat/6/idpl/1088583
>
> seems to suggest otherwise. It would be confusing if they had sperlX.Y.Z
> refer to the s(et)uidperl binary and sperl refer to perl. Maybe any such
> (broken?) distributions could rename things appropriately. It seems cleaner
> to give the s(et)uidperl binary a name that has not been used previously
> (wrongly, traditionally).

I'm not sure. Now that I've understood Brendan's explanation, I can see
avoiding a new name as saving pollution of /usr/bin
I don't think that perl has installed bare sperl (but I got the suidperl
wrong), so I'd see it as up to any distribution to chose what to do when
it integrates 5.8.4 (or later)

So reverting the change that brought "setuidperl", and doing as Brendan
proposes seems a simpler solution that will work.

(where by work I mean not break existing #! lines, not break existing software
that probes for "suidperl", and provides the security of your rewrite)

Nicholas Clark

[Rafael Garcia-Suarez]

Nicholas Clark wrote:
>
> I'm not sure. Now that I've understood Brendan's explanation, I can see
> avoiding a new name as saving pollution of /usr/bin
> I don't think that perl has installed bare sperl (but I got the suidperl
> wrong), so I'd see it as up to any distribution to chose what to do when
> it integrates 5.8.4 (or later)

In any case, the new binary layout must be explained clearly and in big
bold letters in perldelta; as the distribution packager will sometimes
need to restore the desired permissions by hand accordingly. (That's at
least the case with RPMs, which are not typically build as root, but
which are installed as root -- meaning that the permissions and owners
of files are adjusted by hand when the files are installed.)

> So reverting the change that brought "setuidperl", and doing as Brendan
> proposes seems a simpler solution that will work.
>
> (where by work I mean not break existing #! lines, not break existing software
> that probes for "suidperl", and provides the security of your rewrite)

I believe that's a safe enough solution too.