Flawed environmental variable handling in taint mode.

[p5pRT]

Migrated from rt.perl.org#127380 (status was 'open')

Searchable as RT127380$

[p5pRT]

From @lightsey

Hi there\,

While investigating taint mode as a possible solution to other flaws\, we came across a flaw in the implementation of taint mode itself.

The "perlrun" documentation states "The PERLIO environment variable is completely ignored when Perl is run in taint mode" and "This (PERLIO_DEBUG) functionality is disabled for setuid scripts and for scripts run with -T."

When Perl is running in taint mode\, it still honors the PERL_HASH_SEED and PERL_HASH_SEED_DEBUG environmental variables. Honoring these variables is a bit dubious on its own\, but it also allows bypassing the restrictions on the PERIO* environmental variables.

Example without taint mode​:

root@​jdvm​:/# rm -f /tmp/perlio.log root@​jdvm​:/# PERLIO="​:raw" PERLIO_DEBUG=/tmp/perlio.log perl -e 'print "hello\n";' hello root@​jdvm​:/# ls -alh /tmp/perlio.log -rw-r--r--. 1 root root 1.8K Jan 7 12​:59 /tmp/perlio.log

Example of correct behavior with taint mode​:

root@​jdvm​:/# rm -f /tmp/perlio.log root@​jdvm​:/# PERLIO="​:raw" PERLIO_DEBUG=/tmp/perlio.log perl -T -e 'print "hello\n";' hello root@​jdvm​:/# ls -alh /tmp/perlio.log ls​: cannot access /tmp/perlio.log​: No such file or directory

Example of vulnerable behavior​:

root@​jdvm​:/# rm -f /tmp/perlio.log root@​jdvm​:/# PERLIO="​:raw" PERLIO_DEBUG=/tmp/perlio.log PERL_HASH_SEED_DEBUG=1 perl -T -e 'print "hello\n";' HASH_SEED = 10642741970855894112 hello root@​jdvm​:/# ls -alh /tmp/perlio.log -rw-r--r--. 1 root root 1.9K Jan 7 13​:02 /tmp/perlio.log

If you agree that this behavior is a vulnerability in the Perl core\, please credit the cPanel Security Team for its discovery.

John Lightsey

[p5pRT]

From @craigberry

On Tue\, Jan 26\, 2016 at 11​:55 AM\, John Lightsey \perl5\-security\-report@​perl\.org wrote​:

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

Hi there\,

While investigating taint mode as a possible solution to other flaws\, we came across a flaw in the implementation of taint mode itself.

The "perlrun" documentation states "The PERLIO environment variable is completely ignored when Perl is run in taint mode" and "This (PERLIO_DEBUG) functionality is disabled for setuid scripts and for scripts run with -T."

When Perl is running in taint mode\, it still honors the PERL_HASH_SEED and PERL_HASH_SEED_DEBUG environmental variables. Honoring these variables is a bit dubious on its own\, but it also allows bypassing the restrictions on the PERIO* environmental variables.

Example without taint mode​:

root@​jdvm​:/# rm -f /tmp/perlio.log root@​jdvm​:/# PERLIO="​:raw" PERLIO_DEBUG=/tmp/perlio.log perl -e 'print "hello\n";' hello root@​jdvm​:/# ls -alh /tmp/perlio.log -rw-r--r--. 1 root root 1.8K Jan 7 12​:59 /tmp/perlio.log

Example of correct behavior with taint mode​:

root@​jdvm​:/# rm -f /tmp/perlio.log root@​jdvm​:/# PERLIO="​:raw" PERLIO_DEBUG=/tmp/perlio.log perl -T -e 'print "hello\n";' hello root@​jdvm​:/# ls -alh /tmp/perlio.log ls​: cannot access /tmp/perlio.log​: No such file or directory

Example of vulnerable behavior​:

root@​jdvm​:/# rm -f /tmp/perlio.log root@​jdvm​:/# PERLIO="​:raw" PERLIO_DEBUG=/tmp/perlio.log PERL_HASH_SEED_DEBUG=1 perl -T -e 'print "hello\n";' HASH_SEED = 10642741970855894112 hello root@​jdvm​:/# ls -alh /tmp/perlio.log -rw-r--r--. 1 root root 1.9K Jan 7 13​:02 /tmp/perlio.log

If you agree that this behavior is a vulnerability in the Perl core\, please credit the cPanel Security Team for its discovery.

It seems to me anything that bypasses a tainting check is problematic.

Here's what I think is happening after looking at the code for a few minutes. The check for PERLIO_HASH_SEED_DEBUG happens very early in perl.c​:perl_parse before PL_tainting has been initialized. If we find PERL_HASH_SEED_DEBUG in the environment\, we call PerlIO_printf(Perl_debug_log\, ...) to report its value. Perl_debug_log is a macro that translates to PerlIO_stderr()\, which calls the following​:

PerlIO_stdstreams   PerlIO_fdopen   PerlIO_openn   PerlIO_debug

In PerlIO_debug we check for tainting\, which hasn't been enabled yet\, so we then check for PERLIO_DEBUG and open the log if it's set. The open fd records that we've already been here and subsequent calls will write to the open log without any further taint checks. This all needs to be verified by stepping through gdb or equivalent\, but I'm pretty sure that's it.

So the problem seems to be very specific to the intersection of PERL_DEBUG_LOG and PERL_HASH_SEED_DEBUG and I'm not seeing any general hole in tainting\, though we should probably check for other uses of PerlIO early in start-up. I guess the fix would be to move the check for PERL_HASH_SEED_DEBUG later\, or to have it not use PerlIO.

And then draw a big black line somewhere in the middle of perl.c saying "Don't use PerlIO until after this!". Or have some way of making a taint check assert that tainting has been initialized and we aren't just saying there's no tainting because we don't know yet.

[p5pRT]

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

[p5pRT]

From @rjbs

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-01-26T17​:47​:34]

And then draw a big black line somewhere in the middle of perl.c saying "Don't use PerlIO until after this!". Or have some way of making a taint check assert that tainting has been initialized and we aren't just saying there's no tainting because we don't know yet.

That assert sounds like a win\, at least in theory.

It would be nice to release a fix for this along with 127322's.

Any takers on patching?

-- rjbs

[p5pRT]

From @craigberry

On Tue\, Jan 26\, 2016 at 10​:07 PM\, Ricardo Signes \perl\.security@&#8203;rjbs\.manxome\.org wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-01-26T17​:47​:34]

And then draw a big black line somewhere in the middle of perl.c saying "Don't use PerlIO until after this!". Or have some way of making a taint check assert that tainting has been initialized and we aren't just saying there's no tainting because we don't know yet.

That assert sounds like a win\, at least in theory.

It would be nice to release a fix for this along with 127322's.

Any takers on patching?

The following (with -DDEBUGGING) will catch us checking whether tainting is enabled before we have processed command-line switches\, -T included. Unfortunately the most trivial run of miniperl will trigger it because Perl_sv_setpvn checks tainting and that gets called (via Perl_newSVpvn) from perl_construct way before we get to command switches.

While I'm not the one doing the CVE paperwork\, I'm thinking this may not have anything to do with the environment per se. I think the topic is more "Operations during the CONSTRUCT phase of start-up can bypass taint."

Inline Patch

```diff diff --git a/perl.h b/perl.h index 960a8a5..bfcdd48 100644 --- a/perl.h +++ b/perl.h @@ -614,7 +614,7 @@ # define TAINT_PROPER(s) if (UNLIKELY(PL_tainting)) { ```

taintproper(NULL, s); } # define TAINT_set(s) (PL_tainted = (s)) # define TAINT_get (PL_tainted) -# define TAINTING_get (PL_tainting) +# define TAINTING_get (__ASSERT\(PL_phase >= PERL_PHASE_START) PL_tainting) # define TAINTING_set(s) (PL_tainting = (s)) # define TAINT_WARN_get (PL_taint_warn) # define TAINT_WARN_set(s) (PL_taint_warn = (s))

[p5pRT]

From @tonycoz

On Tue\, Jan 26\, 2016 at 04​:47​:34PM -0600\, Craig A. Berry wrote​:

It seems to me anything that bypasses a tainting check is problematic.

Here's what I think is happening after looking at the code for a few minutes. The check for PERLIO_HASH_SEED_DEBUG happens very early in perl.c​:perl_parse before PL_tainting has been initialized. If we find PERL_HASH_SEED_DEBUG in the environment\, we call PerlIO_printf(Perl_debug_log\, ...) to report its value. Perl_debug_log is a macro that translates to PerlIO_stderr()\, which calls the following​:

PerlIO_stdstreams PerlIO_fdopen PerlIO_openn PerlIO_debug

In PerlIO_debug we check for tainting\, which hasn't been enabled yet\, so we then check for PERLIO_DEBUG and open the log if it's set. The open fd records that we've already been here and subsequent calls will write to the open log without any further taint checks. This all needs to be verified by stepping through gdb or equivalent\, but I'm pretty sure that's it.

So the problem seems to be very specific to the intersection of PERL_DEBUG_LOG and PERL_HASH_SEED_DEBUG and I'm not seeing any general hole in tainting\, though we should probably check for other uses of PerlIO early in start-up. I guess the fix would be to move the check for PERL_HASH_SEED_DEBUG later\, or to have it not use PerlIO.

And then draw a big black line somewhere in the middle of perl.c saying "Don't use PerlIO until after this!". Or have some way of making a taint check assert that tainting has been initialized and we aren't just saying there's no tainting because we don't know yet.

I'm not sure what this means in concrete terms - some other output can occur very early​:

tony@​mars​:.../git/perl$ ls -l blargle.log ls​: cannot access blargle.log​: No such file or directory tony@​mars​:.../git/perl$ LC_ALL=unknown PERLIO_DEBUG=blargle.log ./perl -Te1 perl​: warning​: Setting locale failed. perl​: warning​: Please check that your locale settings​:   LANGUAGE = (unset)\,   LC_ALL = "unknown"\,   LANG = "en_AU.UTF-8"   are supported and installed on your system. perl​: warning​: Falling back to a fallback locale ("en_AU.UTF-8"). tony@​mars​:.../git/perl$ ls -l blargle.log -rw-r--r-- 1 tony tony 1905 Jan 28 10​:05 blargle.log

The safest way I can see to do it would be to scan for the -T switch at the top of perl_construct()\, but this has two problems​:

a) perl_construct() doesn't have access to argv\, we could add it at the cost of breaking back-compat for embedding.

b) perl's switch parsing is complex\, and we'd have to reproduce a lot of that complexity to reliably parse out the -T switch.

To fix just the hash seed debug case we could move the hash seed debug code from the beginning of perl_parse() to S_parse_body() just after the switch parsing.

Tony

[p5pRT]

From @craigberry

On Wed\, Jan 27\, 2016 at 5​:07 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Tue\, Jan 26\, 2016 at 04​:47​:34PM -0600\, Craig A. Berry wrote​:

And then draw a big black line somewhere in the middle of perl.c saying "Don't use PerlIO until after this!". Or have some way of making a taint check assert that tainting has been initialized and we aren't just saying there's no tainting because we don't know yet.

I'm not sure what this means in concrete terms - some other output can occur very early​:

tony@​mars​:.../git/perl$ ls -l blargle.log ls​: cannot access blargle.log​: No such file or directory tony@​mars​:.../git/perl$ LC_ALL=unknown PERLIO_DEBUG=blargle.log ./perl -Te1 perl​: warning​: Setting locale failed. perl​: warning​: Please check that your locale settings​: LANGUAGE = (unset)\, LC_ALL = "unknown"\, LANG = "en_AU.UTF-8" are supported and installed on your system. perl​: warning​: Falling back to a fallback locale ("en_AU.UTF-8"). tony@​mars​:.../git/perl$ ls -l blargle.log -rw-r--r-- 1 tony tony 1905 Jan 28 10​:05 blargle.log

The safest way I can see to do it would be to scan for the -T switch at the top of perl_construct()\, but this has two problems​:

a) perl_construct() doesn't have access to argv\, we could add it at the cost of breaking back-compat for embedding.

b) perl's switch parsing is complex\, and we'd have to reproduce a lot of that complexity to reliably parse out the -T switch.

To fix just the hash seed debug case we could move the hash seed debug code from the beginning of perl_parse() to S_parse_body() just after the switch parsing.

It's certainly tempting to do that move\, but I'm not sure why Yves wanted it so early in start-up.

I found it surprising\, though\, how many things in the construct phase are bypassing taint checking because PL_tainting hasn't been set yet\, but maybe that's not a problem at that phase since we aren't executing the user's script or any other user input yet?

[p5pRT]

From @tonycoz

On Wed\, Jan 27\, 2016 at 05​:45​:24PM -0600\, Craig A. Berry wrote​:

On Wed\, Jan 27\, 2016 at 5​:07 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Tue\, Jan 26\, 2016 at 04​:47​:34PM -0600\, Craig A. Berry wrote​:

And then draw a big black line somewhere in the middle of perl.c saying "Don't use PerlIO until after this!". Or have some way of making a taint check assert that tainting has been initialized and we aren't just saying there's no tainting because we don't know yet.

I'm not sure what this means in concrete terms - some other output can occur very early​:

tony@​mars​:.../git/perl$ ls -l blargle.log ls​: cannot access blargle.log​: No such file or directory tony@​mars​:.../git/perl$ LC_ALL=unknown PERLIO_DEBUG=blargle.log ./perl -Te1 perl​: warning​: Setting locale failed. perl​: warning​: Please check that your locale settings​: LANGUAGE = (unset)\, LC_ALL = "unknown"\, LANG = "en_AU.UTF-8" are supported and installed on your system. perl​: warning​: Falling back to a fallback locale ("en_AU.UTF-8"). tony@​mars​:.../git/perl$ ls -l blargle.log -rw-r--r-- 1 tony tony 1905 Jan 28 10​:05 blargle.log

The safest way I can see to do it would be to scan for the -T switch at the top of perl_construct()\, but this has two problems​:

a) perl_construct() doesn't have access to argv\, we could add it at the cost of breaking back-compat for embedding.

b) perl's switch parsing is complex\, and we'd have to reproduce a lot of that complexity to reliably parse out the -T switch.

To fix just the hash seed debug case we could move the hash seed debug code from the beginning of perl_parse() to S_parse_body() just after the switch parsing.

It's certainly tempting to do that move\, but I'm not sure why Yves wanted it so early in start-up.

I found it surprising\, though\, how many things in the construct phase are bypassing taint checking because PL_tainting hasn't been set yet\, but maybe that's not a problem at that phase since we aren't executing the user's script or any other user input yet?

It's a serious problem for PERLIO_DEBUG\, if perl is running as a different user to whoever set PERLIO_DEBUG (the author of a wrapper might have believed us about ignoring it\,) then it could be used as an attack on files the original user didn't have access to.

They can't control the contents of the file\, but it could be used to overwrite /etc/sudoers for example.

(This isn't a problem if the wrapper preserved euid != uid\, since perl will enable tainting much earlier.)

Fixing the hash seed debug case with the move won't fix the similar locale issue.

Tony

[p5pRT]

From @craigberry

On Wed\, Jan 27\, 2016 at 6​:43 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Wed\, Jan 27\, 2016 at 05​:45​:24PM -0600\, Craig A. Berry wrote​:

I found it surprising\, though\, how many things in the construct phase are bypassing taint checking because PL_tainting hasn't been set yet\, but maybe that's not a problem at that phase since we aren't executing the user's script or any other user input yet?

It's a serious problem for PERLIO_DEBUG\, if perl is running as a different user to whoever set PERLIO_DEBUG (the author of a wrapper might have believed us about ignoring it\,) then it could be used as an attack on files the original user didn't have access to.

They can't control the contents of the file\, but it could be used to overwrite /etc/sudoers for example.

(This isn't a problem if the wrapper preserved euid != uid\, since perl will enable tainting much earlier.)

Fixing the hash seed debug case with the move won't fix the similar locale issue.

Yeah\, that's why I was looking for a bigger hammer than just moving PERL_HASH_SEED_DEBUG. You easily found the locale issue\, but how many other things might there be that we don't know about or could easily and unwittingly add in the future? The assert was too big a hammer because you can't even create an SVPV without triggering taint checks\, and there are a number of those in perl_construct.

The patch below (inline and attached) is a proof of concept for a different -- but still quite large -- hammer. It might still be too big but illustrates where the problem is. It does pass all tests for me\, but I don't have a very good feel for what might break (embedding?). The commit message should be mostly self-explanatory\, but note that it renders PERL_HASH_SEED_DEBUG safe without moving it by making tainting always on throughout perl_construct.

From 1584d2fa54886e54f8e3a21df2c37a5620e1a263 Mon Sep 17 00​:00​:00 2001 From​: "Craig A. Berry" \craigberry@&#8203;mac\.com Date​: Thu\, 28 Jan 2016 09​:36​:31 -0600 Subject​: [PATCH] Make tainting always on during perl_construct.

Previously tainting only became enabled during the CONSTRUCT phase if euid != uid checks caused it to be so\, and not until the START phase would processing of command-line switches detect that tainting had been selected via -T or -t. This meant that during the construction phase\, "tainting is not enabled" was treated as equivalent to "don't know yet whether tainting is enabled."

This commit closes the "don't know yet" window. Perl is now compiled with tainting on by default and it remains on through the CONSTRUCT phase of interpreter start-up. Then\, right as we begin the START phase\, we turn it off\, do our uid checks\, and then process command-line switches\, re-enabling tainting if we find -T or -t. This prevents anything running during construction from bypassing taint checks\, while leaving anything running from switch processing and later as-is.

---

intrpvar.h | 2 +- perl.c | 8 ++++++-- 2 files changed\, 7 insertions(+)\, 3 deletions(-)

Inline Patch

```diff diff --git a/intrpvar.h b/intrpvar.h index e54f53e..8c3d27a 100644 --- a/intrpvar.h +++ b/intrpvar.h @@ -74,7 +74,7 @@ PERLVAR(I, multideref_pc, UNOP_AUX_item *) /* Fields used by magic variables such as $@, $/ and so on */ PERLVAR(I, curpm, PMOP *) /* what to do \ interps in REs from */ -PERLVAR(I, tainting, bool) /* doing taint checks */ +PERLVARI(I, tainting, bool, TRUE) /* doing taint checks */ PERLVARI(I, tainted, bool, FALSE) /* using variables controlled by $< */ /* PL_delaymagic is currently used for two purposes: to assure simultaneous diff --git a/perl.c b/perl.c index c7c1fe6..a3ef760 100644 --- a/perl.c +++ b/perl.c @@ -248,8 +248,6 @@ perl_construct(pTHXx) init_stacks(); - init_ids(); - JMPENV_BOOTSTRAP; STATUS_ALL_SUCCESS; @@ -1864,6 +1862,12 @@ S_parse_body(pTHX_ char **env, XSINIT_t xsinit) PERL_SET_PHASE(PERL_PHASE_START); + /* We're born with taint, but only stay that way if we choose to via + * -t or -T below, or if our effective ids don't match our actual ids. + */ + TAINTING_set(FALSE); + init_ids(); + init_main_stash(); { -- 2.2.1 ```

[p5pRT]

From @craigberry

0001-Make-tainting-always-on-during-perl_construct.patch

```diff From 1584d2fa54886e54f8e3a21df2c37a5620e1a263 Mon Sep 17 00:00:00 2001 From: "Craig A. Berry" Date: Thu, 28 Jan 2016 09:36:31 -0600 Subject: [PATCH] Make tainting always on during perl_construct. Previously tainting only became enabled during the CONSTRUCT phase if euid != uid checks caused it to be so, and not until the START phase would processing of command-line switches detect that tainting had been selected via -T or -t. This meant that during the construction phase, "tainting is not enabled" was treated as equivalent to "don't know yet whether tainting is enabled." This commit closes the "don't know yet" window. Perl is now compiled with tainting on by default and it remains on through the CONSTRUCT phase of interpreter start-up. Then, right as we begin the START phase, we turn it off, do our uid checks, and then process command-line switches, re-enabling tainting if we find -T or -t. This prevents anything running during construction from bypassing taint checks, while leaving anything running from switch processing and later as-is. --- intrpvar.h | 2 +- perl.c | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/intrpvar.h b/intrpvar.h index e54f53e..8c3d27a 100644 --- a/intrpvar.h +++ b/intrpvar.h @@ -74,7 +74,7 @@ PERLVAR(I, multideref_pc, UNOP_AUX_item *) /* Fields used by magic variables such as $@, $/ and so on */ PERLVAR(I, curpm, PMOP *) /* what to do \ interps in REs from */ -PERLVAR(I, tainting, bool) /* doing taint checks */ +PERLVARI(I, tainting, bool, TRUE) /* doing taint checks */ PERLVARI(I, tainted, bool, FALSE) /* using variables controlled by $< */ /* PL_delaymagic is currently used for two purposes: to assure simultaneous diff --git a/perl.c b/perl.c index c7c1fe6..a3ef760 100644 --- a/perl.c +++ b/perl.c @@ -248,8 +248,6 @@ perl_construct(pTHXx) init_stacks(); - init_ids(); - JMPENV_BOOTSTRAP; STATUS_ALL_SUCCESS; @@ -1864,6 +1862,12 @@ S_parse_body(pTHX_ char **env, XSINIT_t xsinit) PERL_SET_PHASE(PERL_PHASE_START); + /* We're born with taint, but only stay that way if we choose to via + * -t or -T below, or if our effective ids don't match our actual ids. + */ + TAINTING_set(FALSE); + init_ids(); + init_main_stash(); { -- 2.2.1 ```

[p5pRT]

From @tonycoz

On Thu Jan 28 08​:49​:28 2016\, craig.a.berry@​gmail.com wrote​:

The patch below (inline and attached) is a proof of concept for a different -- but still quite large -- hammer. It might still be too big but illustrates where the problem is. It does pass all tests for me\, but I don't have a very good feel for what might break (embedding?). The commit message should be mostly self-explanatory\, but note that it renders PERL_HASH_SEED_DEBUG safe without moving it by making tainting always on throughout perl_construct.

I couldn't see a way to make perl with this patch honor PERLIO_DEBUG with -T\, but I did make it ignore PERLIO_DEBUG without -T​:

tony@​mars​:.../git/perl$ PERLIO_DEBUG=blargle.txt LC_ALL=unknown ./perl -e 'print "foo\n"' perl​: warning​: Setting locale failed. perl​: warning​: Please check that your locale settings​:   LANGUAGE = (unset)\,   LC_ALL = "unknown"\,   LANG = "en_AU.UTF-8"   are supported and installed on your system. perl​: warning​: Falling back to a fallback locale ("en_AU.UTF-8"). foo You have new mail in /var/mail/tony tony@​mars​:.../git/perl$ ls -l blargle.txt ls​: cannot access blargle.txt​: No such file or directory

We're also still honoring PERL_HASH_SEED and PERL_HASH_SEED_DEBUG with -T (and with euid != uid.)

Tony

[p5pRT]

From @craigberry

On Thu\, Jan 28\, 2016 at 5​:11 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu Jan 28 08​:49​:28 2016\, craig.a.berry@​gmail.com wrote​:

The patch below (inline and attached) \ renders PERL_HASH_SEED_DEBUG safe without moving it by making tainting always on throughout perl_construct.

I couldn't see a way to make perl with this patch honor PERLIO_DEBUG with -T\,

Thanks for the test drive.

but I did make it ignore PERLIO_DEBUG without -T​:

That's expected\, and I probably should have said so in the commit message. Because all of perl_construct has tainting enabled\, anything PerlIO that happens in that phase while PERLIO_DEBUG is defined will trigger the tainting clause of what's documented for PERLIO_DEBUG\, i.e.\, no log. That certainly could surprise people who set PERLIO_DEBUG and didn't know they were doing anything that would cause I/O that early in start-up. It seemed preferable to letting the log get created when it shoudn't. I think we could mitigate this by having PerlIO_debug do nothing unless PL_phase >= PERL_PHASE_START. That way debugging messages during PERL_PHASE_CONSTRUCT would not get logged\, but everything later would (assuming no -T).

We're also still honoring PERL_HASH_SEED and PERL_HASH_SEED_DEBUG with -T (and with euid != uid.)

And I'm thinking this is as it should be\, but please let me know if I'm missing something.

[p5pRT]

From @tonycoz

On Thu Jan 28 17​:15​:54 2016\, craig.a.berry@​gmail.com wrote​:

On Thu\, Jan 28\, 2016 at 5​:11 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

We're also still honoring PERL_HASH_SEED and PERL_HASH_SEED_DEBUG with -T (and with euid != uid.)

And I'm thinking this is as it should be\, but please let me know if I'm missing something.

It's an issue because an attacker can set PERL_HASH_SEED to a known bad seed and cause the child process to build a linear HV and use much more CPU than expected.

Tony

[p5pRT]

From @craigberry

On Mon\, Feb 1\, 2016 at 6​:42 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu Jan 28 17​:15​:54 2016\, craig.a.berry@​gmail.com wrote​:

On Thu\, Jan 28\, 2016 at 5​:11 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

We're also still honoring PERL_HASH_SEED and PERL_HASH_SEED_DEBUG with -T (and with euid != uid.)

And I'm thinking this is as it should be\, but please let me know if I'm missing something.

It's an issue because an attacker can set PERL_HASH_SEED to a known bad seed and cause the child process to build a linear HV and use much more CPU than expected.

Ah\, ok. Then we have a chicken-and-egg problem because you must initialize the seed before creating any HVs\, and we start creating HVs very early in perl_construct. We don't know whether tainting is enabled at the command line until well into perl_parse. So I don't see a way to disable PERL_HASH_SEED under -T\, though we could do so under euid != uid because that check is early enough.

[p5pRT]

From @craigberry

The following little hack was the quickest way I could instrument Perl to tell us what environment variables we are looking up at what phase of start-up (1st column) and whether we think tainting is enabled at the time that we check (middle column). You can't build extensions with this patch but you get a working miniperl that tells us a lot.

Inline Patch

```diff diff --git a/iperlsys.h b/iperlsys.h index 86ab687..87544f7 100644 --- a/iperlsys.h +++ b/iperlsys.h @@ -562,7 +562,11 @@ struct IPerlEnvInfo #else /* PERL_IMPLICIT_SYS */ #define PerlEnv_putenv(str) putenv((str)) -#define PerlEnv_getenv(str) getenv((str)) +#undef printf +#include + +#define PerlEnv_getenv(str) (((void)printf("%s\t%d\t%s\n", ```

PL_phase_names[PL_phase],PL_tainting,str)),getenv((str))) + #define PerlEnv_getenv_len(str\,l) getenv_len((str)\, (l)) #ifdef HAS_ENVGETENV # define PerlEnv_ENVgetenv(str) ENVgetenv((str)) [end]

Here\, with the simplest thing possible under -T\, we look up 8 environment variables before we get to the start phase and actually realize tainting is enabled. There are likely others based on platform or configuration​:

$ ./miniperl -T -e '1;' CONSTRUCT 0 PERL_SKIP_LOCALE_INIT CONSTRUCT 0 LC_ALL CONSTRUCT 0 LANG CONSTRUCT 0 PERL_BADLANG CONSTRUCT 0 PERL_UNICODE CONSTRUCT 0 PERL_HASH_SEED CONSTRUCT 0 PERL_PERTURB_KEYS CONSTRUCT 0 PERL_HASH_SEED_DEBUG START 1 PERL_SIGNALS

In the following case\, we can trigger checks of PERLIO and PERLIO_DEBUG before we know that tainting is enabled. There are likely several bad combinations possible\, such as doing something that causes PERL_BADLANG to print a warning\, which of course uses PerlIO to output the message. I have not checked all these environment variables to see which ones are documented to disarm themselves under taint\, but even if they don't say they will\, perhaps they should\, and they currently can't since we don't know tainting has been enabled yet when they are checked.

$ export PERLIO=perlio $ export PERLIO_DEBUG=pio.log $ export PERL_HASH_SEED_DEBUG=1 $ ./miniperl -T -e '1;' CONSTRUCT 0 PERL_SKIP_LOCALE_INIT CONSTRUCT 0 LC_ALL CONSTRUCT 0 LANG CONSTRUCT 0 PERL_BADLANG CONSTRUCT 0 PERL_UNICODE CONSTRUCT 0 PERL_HASH_SEED CONSTRUCT 0 PERL_PERTURB_KEYS CONSTRUCT 0 PERL_HASH_SEED_DEBUG CONSTRUCT 0 PERLIO CONSTRUCT 0 PERLIO_DEBUG HASH_FUNCTION = ONE_AT_A_TIME_HARD HASH_SEED = 0xe7c36c03163165e4 PERTURB_KEYS = 1 (RANDOM) START 1 PERL_SIGNALS

[p5pRT]

From @craigberry

I've attached (and inlined) an alternative patch for this ticket. It's safer than anything thus-far discussed because it only tweaks how we handle environ vars in perlio.c rather than trying to address everything taint-related during the earliest phase of start-up. It could be considered a bad precedent in that it sort of implies every time you check for taint you also have to check the phase of the Perl interpreter. But\, unless we want to scan for -T earlier than we do or consider taint to be on throughout the construct phase as in my earlier patch\, this may be the only way forward. At least I haven't thought of any others.

From b71ab34ae48db0d679484be28259f74ead8bd88a Mon Sep 17 00​:00​:00 2001 From​: "Craig A. Berry" \craigberry@&#8203;mac\.com Date​: Thu\, 11 Feb 2016 19​:02​:41 -0600 Subject​: [PATCH] Make PerlIO environ vars wait for taint.

The environment variables PERLIO\, PERLIO_DEBUG\, and TMPDIR are not honored under taint. However\, anything that triggers I/O very early in start-up could have bypassed these taint checks because the command line (including -T) had not been processed yet. So don't honor them unless far enough along in start-up to have seen -T.

In the case of PERLIO_DEBUG\, this means debug messages very early in start-up (during the CONSTRUCT phase) will not be written to the log\, regardless of whether -T is present on the command line.

In the case of PERLIO\, anything that triggers I/O very early in start-up will cause a default layer to be selected without the environment's PERLIO setting being consulted\, regardless of whether -T is present on the command line.

---

perlio.c | 10 +++++++--- 1 file changed\, 7 insertions(+)\, 3 deletions(-)

Inline Patch

```diff diff --git a/perlio.c b/perlio.c index 11a66d0..5e32fd4 100644 --- a/perlio.c +++ b/perlio.c @@ -351,7 +351,7 @@ PerlIO_debug(const char *fmt, ...) va_list ap; dSYS; va_start(ap, fmt); - if (!PL_perlio_debug_fd) { + if (!PL_perlio_debug_fd && PL_phase >= PERL_PHASE_START) { if (!TAINTING_get && PerlProc_getuid() == PerlProc_geteuid() && PerlProc_getgid() == PerlProc_getegid()) { @@ -1051,7 +1051,9 @@ PerlIO_list_t * PerlIO_default_layers(pTHX) { if (!PL_def_layerlist) { - const char * const s = TAINTING_get ? NULL : PerlEnv_getenv("PERLIO"); + const char * const s = TAINTING_get || PL_phase < PERL_PHASE_START + ? NULL + : PerlEnv_getenv("PERLIO"); PERLIO_FUNCS_DECL(*osLayer) = &PerlIO_unix; PL_def_layerlist = PerlIO_list_alloc(aTHX); PerlIO_define_layer(aTHX_ PERLIO_FUNCS_CAST(&PerlIO_unix)); @@ -5007,7 +5009,9 @@ PerlIO_tmpfile(void) # if defined(HAS_MKSTEMP) && ! defined(VMS) && ! defined(OS2) int fd = -1; char tempname[] = "/tmp/PerlIO_XXXXXX"; - const char * const tmpdir = TAINTING_get ? NULL : ```

PerlEnv_getenv("TMPDIR"); + const char * const tmpdir = TAINTING_get || PL_phase \< PERL_PHASE_START + ? NULL + : PerlEnv_getenv("TMPDIR");   SV * sv = NULL;   int old_umask = umask(0177);   /* -- 2.2.1

[p5pRT]

From @craigberry

0001-Make-PerlIO-environ-vars-wait-for-taint.patch

```diff From b71ab34ae48db0d679484be28259f74ead8bd88a Mon Sep 17 00:00:00 2001 From: "Craig A. Berry" Date: Thu, 11 Feb 2016 19:02:41 -0600 Subject: [PATCH] Make PerlIO environ vars wait for taint. The environment variables PERLIO, PERLIO_DEBUG, and TMPDIR are not honored under taint. However, anything that triggers I/O very early in start-up could have bypassed these taint checks because the command line (including -T) had not been processed yet. So don't honor them unless far enough along in start-up to have seen -T. In the case of PERLIO_DEBUG, this means debug messages very early in start-up (during the CONSTRUCT phase) will not be written to the log, regardless of whether -T is present on the command line. In the case of PERLIO, anything that triggers I/O very early in start-up will cause a default layer to be selected without the environment's PERLIO setting being consulted, regardless of whether -T is present on the command line. --- perlio.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/perlio.c b/perlio.c index 11a66d0..5e32fd4 100644 --- a/perlio.c +++ b/perlio.c @@ -351,7 +351,7 @@ PerlIO_debug(const char *fmt, ...) va_list ap; dSYS; va_start(ap, fmt); - if (!PL_perlio_debug_fd) { + if (!PL_perlio_debug_fd && PL_phase >= PERL_PHASE_START) { if (!TAINTING_get && PerlProc_getuid() == PerlProc_geteuid() && PerlProc_getgid() == PerlProc_getegid()) { @@ -1051,7 +1051,9 @@ PerlIO_list_t * PerlIO_default_layers(pTHX) { if (!PL_def_layerlist) { - const char * const s = TAINTING_get ? NULL : PerlEnv_getenv("PERLIO"); + const char * const s = TAINTING_get || PL_phase < PERL_PHASE_START + ? NULL + : PerlEnv_getenv("PERLIO"); PERLIO_FUNCS_DECL(*osLayer) = &PerlIO_unix; PL_def_layerlist = PerlIO_list_alloc(aTHX); PerlIO_define_layer(aTHX_ PERLIO_FUNCS_CAST(&PerlIO_unix)); @@ -5007,7 +5009,9 @@ PerlIO_tmpfile(void) # if defined(HAS_MKSTEMP) && ! defined(VMS) && ! defined(OS2) int fd = -1; char tempname[] = "/tmp/PerlIO_XXXXXX"; - const char * const tmpdir = TAINTING_get ? NULL : PerlEnv_getenv("TMPDIR"); + const char * const tmpdir = TAINTING_get || PL_phase < PERL_PHASE_START + ? NULL + : PerlEnv_getenv("TMPDIR"); SV * sv = NULL; int old_umask = umask(0177); /* -- 2.2.1 ```

[p5pRT]

From @rjbs

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-05T19​:54​:57]

I've attached (and inlined) an alternative patch for this ticket.

Thanks\, Craig.

There have been no follow-ups to this patch. Warnock was here.

Is this a patch we want to apply to blead before 5.24.0? Is this a security problem? It seems like "maybe." My quick scan shows that by getting the PerlIO debug handle opened early\, it stays open and will be used for the rest of the program\, meaning we leak... things. I'm not sure exactly what.

If we're going to fix this problem (as seems reasonable)\, do we want to fix it in 5.22 and 5.20? Presumably we should\, if we're viewing it as a security problem. Does it need a CVE and an embargo? My hunch is "no." If that's the case\, we just apply the patch everywhere and we're done.

If the reset of the security team disagrees\, I'd like to get started ASAP\, because any embargo time is time when this patch isn't on blead\, and so isn't smoking\, and so makes me less confident in blead's ability to become 5.23.0.

-- rjbs

[p5pRT]

From @craigberry

On Sun\, Mar 20\, 2016 at 8​:27 AM\, Ricardo Signes \perl\.security@&#8203;rjbs\.manxome\.org wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-05T19​:54​:57]

I've attached (and inlined) an alternative patch for this ticket.

Thanks\, Craig.

There have been no follow-ups to this patch. Warnock was here.

That bastard has been getting around ;-).

Is this a patch we want to apply to blead before 5.24.0? Is this a security problem? It seems like "maybe." My quick scan shows that by getting the PerlIO debug handle opened early\, it stays open and will be used for the rest of the program\, meaning we leak... things. I'm not sure exactly what.

My understanding is that the problem isn't leakage but nefarious definitions. If you set PERLIO_DEBUG=/etc/passwd and then run a privileged subprocess that depends on -T to prevent bad things from happening\, the safety mechanism of -T fails (and your password file gets clobbered) if anything triggers PerlIO during the earliest phase of start-up before -T has been checked. That's a bit hand-wavey -- I think Tony had some concrete examples up-thread somewhere.

If we're going to fix this problem (as seems reasonable)\, do we want to fix it in 5.22 and 5.20? Presumably we should\, if we're viewing it as a security problem. Does it need a CVE and an embargo? My hunch is "no." If that's the case\, we just apply the patch everywhere and we're done.

If the reset of the security team disagrees\, I'd like to get started ASAP\, because any embargo time is time when this patch isn't on blead\, and so isn't smoking\, and so makes me less confident in blead's ability to become 5.23.0.

I don't know about these things and I don't know how dangerous the bug actually is. This really needs more eyes on it\, preferably belonging to someone who's used taint and setuid scripts in the real world.

[p5pRT]

From @craigberry

On Sun\, Mar 20\, 2016 at 10​:35 PM\, Craig A. Berry \craig\.a\.berry@&#8203;gmail\.com wrote​:

On Sun\, Mar 20\, 2016 at 8​:27 AM\, Ricardo Signes \perl\.security@&#8203;rjbs\.manxome\.org wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-05T19​:54​:57]

I've attached (and inlined) an alternative patch for this ticket.

Thanks\, Craig.

There have been no follow-ups to this patch. Warnock was here.

That bastard has been getting around ;-).

Tony's grant report from 10 March has the following lines on it\, so I suspect we'll here from him again soon.

0.70 #127380 (sec) work on alternate patch 1.12 #127380 (sec) more alternate patch work

[p5pRT]

From @tonycoz

On Mon Mar 21 12​:50​:36 2016\, craig.a.berry@​gmail.com wrote​:

Tony's grant report from 10 March has the following lines on it\, so I suspect we'll here from him again soon.

0.70 #127380 (sec) work on alternate patch 1.12 #127380 (sec) more alternate patch work

Sorry for not replying to your original patch.

I think your patch is fine.

The only way I can see to avoid PERL_HASH_SEED causing problems is to parse out -T earlier\, which isn't backportable\, since we'd need to supply argc/argv to perl_construct() (which is what I was working on.)

Tony

[p5pRT]

From @craigberry

On Mon\, Mar 21\, 2016 at 7​:11 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Mon Mar 21 12​:50​:36 2016\, craig.a.berry@​gmail.com wrote​:

Tony's grant report from 10 March has the following lines on it\, so I suspect we'll here from him again soon.

0.70 #127380 (sec) work on alternate patch 1.12 #127380 (sec) more alternate patch work

Sorry for not replying to your original patch.

I think your patch is fine.

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

The only way I can see to avoid PERL_HASH_SEED causing problems is to parse out -T earlier\, which isn't backportable\, since we'd need to supply argc/argv to perl_construct() (which is what I was working on.)

This is tricky\, not only because it's scary Larry code from days of yore that needs to be duplicated or refactored or whatever\, but also because changing the function signature of perl_construct breaks source compatibility\, not just binary compatibility\, and requires every embedded Perl application to make source changes.

The only alternative to that I could think of would be that PERL_SYS_INIT3 (which already gets argv and argc) could check for -T and set a global variable indicating what it found. It could not be a PL_xxx global that is part of the interpreter struct because that struct does not exist yet. Then this global could be checked in perl_construct and PL_taint set appropriately. The joys of bootstrapping.

[p5pRT]

From @rjbs

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

  https://twitter.com/hackerfantastic/status/707314526107127809

-- rjbs

[p5pRT]

From @tonycoz

On Sun\, Mar 27\, 2016 at 05​:58​:41PM -0400\, Ricardo Signes wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

I'm not sure that's the same issue\, since I'm not sure taint is involved.

Maybe we're approaching this the wrong way.

Right now\, PERLIO_DEBUG is enabled in non--DDEBUGGING perls\, and file creation/output is enabled based on the environment variable.

Perhaps​:

a) it should only be enabled on -DDEBUGGING perls\, and

b) require a -Di flag (which appears to be unused)

This avoids most of the mess from PL_tainting not being available early enough.

It's technically back-incompatible for maint\, but it's a feature that's mostly of used in debugging PerlIO layers\, for which you can use a -DDEBUGGING perl or a debugger.

Tony

[p5pRT]

From @craigberry

On Sun\, Mar 27\, 2016 at 6​:31 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Sun\, Mar 27\, 2016 at 05​:58​:41PM -0400\, Ricardo Signes wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

I'm not sure that's the same issue\, since I'm not sure taint is involved.

More detail here​:

\https://lists.exim.org/lurker/message/20160302.191005.a72d8433.en.html

They don't use the word "taint" anywhere\, but they do say "installations having Exim set-uid root." Doesn't setuid turn on tainting? I think it does so somewhat earlier than the -T switch does (perl_construct->init_ids) but apparently not early enough. Or maybe somehow\, since it's an embedded Perl\, they are disabling tainting\, though I don't see anything like that on first glance. I believe the relevant code is here​:

\https://github.com/Exim/exim/blob/master/src/src/perl.c#L96

Maybe we're approaching this the wrong way.

Right now\, PERLIO_DEBUG is enabled in non--DDEBUGGING perls\, and file creation/output is enabled based on the environment variable.

Perhaps​:

a) it should only be enabled on -DDEBUGGING perls\, and

b) require a -Di flag (which appears to be unused)

This avoids most of the mess from PL_tainting not being available early enough.

It's technically back-incompatible for maint\, but it's a feature that's mostly of used in debugging PerlIO layers\, for which you can use a -DDEBUGGING perl or a debugger.

It's probably a better design. Certainly a safer one. It feels like a pretty big behavior change. Note that this affects anything calling PerlIO_debug\, not just PerlIO as such. So\, for example\, setting PERL_HASH_SEED_DEBUG would do nothing except on debugging builds and when -Di is passed on the command line.

[p5pRT]

From @craigberry

On Sun\, Mar 27\, 2016 at 7​:48 PM\, Craig A. Berry \craig\.a\.berry@&#8203;gmail\.com wrote​:

On Sun\, Mar 27\, 2016 at 6​:31 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Sun\, Mar 27\, 2016 at 05​:58​:41PM -0400\, Ricardo Signes wrote​:

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

I'm not sure that's the same issue\, since I'm not sure taint is involved.

More detail here​:

\https://lists.exim.org/lurker/message/20160302.191005.a72d8433.en.html

The fix to Exim is at \https://github.com/Exim/exim/commit/bc3c7bb7d4aba3e563434e5627fe1f2176aa18c0. That commit changes 850 files! So it's a bit hard to quickly spot Perl's involvement\, if any.

[p5pRT]

From @tonycoz

On Sun\, Mar 27\, 2016 at 07​:48​:28PM -0500\, Craig A. Berry wrote​:

On Sun\, Mar 27\, 2016 at 6​:31 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Sun\, Mar 27\, 2016 at 05​:58​:41PM -0400\, Ricardo Signes wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

I'm not sure that's the same issue\, since I'm not sure taint is involved.

More detail here​:

\https://lists.exim.org/lurker/message/20160302.191005.a72d8433.en.html

They don't use the word "taint" anywhere\, but they do say "installations having Exim set-uid root." Doesn't setuid turn on tainting? I think it does so somewhat earlier than the -T switch does (perl_construct->init_ids) but apparently not early enough. Or maybe somehow\, since it's an embedded Perl\, they are disabling tainting\, though I don't see anything like that on first glance. I believe the relevant code is here​:

\https://github.com/Exim/exim/blob/master/src/src/perl.c#L96

I guess I made a bad assumption there\, I'd assumed they were calling an external process.

Maybe we're approaching this the wrong way.

Right now\, PERLIO_DEBUG is enabled in non--DDEBUGGING perls\, and file creation/output is enabled based on the environment variable.

Perhaps​:

a) it should only be enabled on -DDEBUGGING perls\, and

b) require a -Di flag (which appears to be unused)

This avoids most of the mess from PL_tainting not being available early enough.

It's technically back-incompatible for maint\, but it's a feature that's mostly of used in debugging PerlIO layers\, for which you can use a -DDEBUGGING perl or a debugger.

It's probably a better design. Certainly a safer one. It feels like a pretty big behavior change. Note that this affects anything calling PerlIO_debug\, not just PerlIO as such. So\, for example\, setting PERL_HASH_SEED_DEBUG would do nothing except on debugging builds and when -Di is passed on the command line.

The PERL_HASH_SEED_DEBUG code doesn't call PerlIO_debug().

PerlIO_debug() is purely used for internal debugging by PerlIO - it's a simple output function that doesn't depend on the rest of PerlIO.

Tony

[p5pRT]

From @craigberry

On Sun\, Mar 27\, 2016 at 7​:58 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

The PERL_HASH_SEED_DEBUG code doesn't call PerlIO_debug().

PerlIO_debug() is purely used for internal debugging by PerlIO - it's a simple output function that doesn't depend on the rest of PerlIO.

Ah\, right. How quickly I forget. So PERL_HASH_SEED and other things that do I/O early in start-up should become safe if PERLIO_DEBUG goes away and you have to use -Di rather than an environment variable to get a PerlIO log file open. I guess -Di would have to take a name​: -Di=debug.log.

[p5pRT]

From @craigberry

On Sun\, Mar 27\, 2016 at 7​:58 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Sun\, Mar 27\, 2016 at 07​:48​:28PM -0500\, Craig A. Berry wrote​:

More detail here​:

\https://lists.exim.org/lurker/message/20160302.191005.a72d8433.en.html

They don't use the word "taint" anywhere\, but they do say "installations having Exim set-uid root." Doesn't setuid turn on tainting? I think it does so somewhat earlier than the -T switch does (perl_construct->init_ids) but apparently not early enough. Or maybe somehow\, since it's an embedded Perl\, they are disabling tainting\, though I don't see anything like that on first glance. I believe the relevant code is here​:

\https://github.com/Exim/exim/blob/master/src/src/perl.c#L96

I guess I made a bad assumption there\, I'd assumed they were calling an external process.

And here is a publicly available exploit\, which does not use PERLIO_DEBUG​:

\https://github.com/HackerFantastic/Public/blob/master/exploits/cve-2016-1531.sh

I'm not sure Perl is at fault here. They are allowing the user to load a module from an embedded Perl started from a privileged process.

[p5pRT]

From @tonycoz

On Sun\, Mar 27\, 2016 at 08​:28​:41PM -0500\, Craig A. Berry wrote​:

On Sun\, Mar 27\, 2016 at 7​:58 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

The PERL_HASH_SEED_DEBUG code doesn't call PerlIO_debug().

PerlIO_debug() is purely used for internal debugging by PerlIO - it's a simple output function that doesn't depend on the rest of PerlIO.

Ah\, right. How quickly I forget. So PERL_HASH_SEED and other things that do I/O early in start-up should become safe if PERLIO_DEBUG goes away and you have to use -Di rather than an environment variable to get a PerlIO log file open. I guess -Di would have to take a name​: -Di=debug.log.

I was thinking we'd only look at PERLIO_DEBUG if -Di was present. -Di wouldn't take a filename parameter.

Perhaps -Di without PERLIO_DEBUG would default to stderr.

Tony

[p5pRT]

From @tonycoz

On Sun\, Mar 27\, 2016 at 08​:52​:36PM -0500\, Craig A. Berry wrote​:

And here is a publicly available exploit\, which does not use PERLIO_DEBUG​:

\https://github.com/HackerFantastic/Public/blob/master/exploits/cve-2016-1531.sh

I'm not sure Perl is at fault here. They are allowing the user to load a module from an embedded Perl started from a privileged process.

Yes\, that looks like an exim bug - we don't look at PERL5LIB or PERL5OPT when euid != uid.

Tony

[p5pRT]

From @tonycoz

On Sun Mar 27 16​:31​:59 2016\, tonyc wrote​:

On Sun\, Mar 27\, 2016 at 05​:58​:41PM -0400\, Ricardo Signes wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

I'm not sure that's the same issue\, since I'm not sure taint is involved.

Maybe we're approaching this the wrong way.

Right now\, PERLIO_DEBUG is enabled in non--DDEBUGGING perls\, and file creation/output is enabled based on the environment variable.

Perhaps​:

a) it should only be enabled on -DDEBUGGING perls\, and

b) require a -Di flag (which appears to be unused)

This avoids most of the mess from PL_tainting not being available early enough.

It's technically back-incompatible for maint\, but it's a feature that's mostly of used in debugging PerlIO layers\, for which you can use a -DDEBUGGING perl or a debugger.

Patch.

I didn't remove PerlIO_debug itself under -DDEBUGGING\, since CPAN modules might be using it without a -DDEBUGGING guard (and this might go into maint.)

Tony

Tony

[p5pRT]

From @tonycoz

0001-perl-127380-only-trace-to-PERLIO_DEBUG-if-Di-is-supp.patch

```diff From f285fe5a8f0f3a721c01056cb1020b3356a00c4d Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Mon, 28 Mar 2016 14:58:56 +1100 Subject: [PATCH] (perl #127380) only trace to $PERLIO_DEBUG if -Di is supplied --- perl.c | 3 +- perl.h | 8 ++++- perlio.c | 102 ++++++++++++++++++++++++++++++++------------------------ pod/perlrun.pod | 68 +++++++++++++++++++------------------ 4 files changed, 102 insertions(+), 79 deletions(-) diff --git a/perl.c b/perl.c index 15e9150..2fe0320 100644 --- a/perl.c +++ b/perl.c @@ -3112,6 +3112,7 @@ Perl_get_debug_opts(pTHX_ const char **s, bool givehelp) " M trace smart match resolution\n" " B dump suBroutine definitions, including special Blocks like BEGIN\n", " L trace some locale setting information--for Perl core development\n", + " i trace PerlIO layer processing\n", NULL }; UV uv = 0; @@ -3120,7 +3121,7 @@ Perl_get_debug_opts(pTHX_ const char **s, bool givehelp) if (isALPHA(**s)) { /* if adding extra options, remember to update DEBUG_MASK */ - static const char debopts[] = "psltocPmfrxuUHXDSTRJvCAqMBL"; + static const char debopts[] = "psltocPmfrxuUHXDSTRJvCAqMBLi"; for (; isWORDCHAR(**s); (*s)++) { const char * const d = strchr(debopts,**s); diff --git a/perl.h b/perl.h index 2ee79c4..b45b241 100644 --- a/perl.h +++ b/perl.h @@ -4171,7 +4171,8 @@ Gid_t getegid (void); #define DEBUG_M_FLAG 0x01000000 /*16777216*/ #define DEBUG_B_FLAG 0x02000000 /*33554432*/ #define DEBUG_L_FLAG 0x04000000 /*67108864*/ -#define DEBUG_MASK 0x07FFEFFF /* mask of all the standard flags */ +#define DEBUG_i_FLAG 0x08000000 /*134217728*/ +#define DEBUG_MASK 0x0FFFEFFF /* mask of all the standard flags */ #define DEBUG_DB_RECURSE_FLAG 0x40000000 #define DEBUG_TOP_FLAG 0x80000000 /* -D was given --> PL_debug |= FLAG */ @@ -4203,6 +4204,7 @@ Gid_t getegid (void); # define DEBUG_M_TEST_ (PL_debug & DEBUG_M_FLAG) # define DEBUG_B_TEST_ (PL_debug & DEBUG_B_FLAG) # define DEBUG_L_TEST_ (PL_debug & DEBUG_L_FLAG) +# define DEBUG_i_TEST_ (PL_debug & DEBUG_i_FLAG) # define DEBUG_Xv_TEST_ (DEBUG_X_TEST_ && DEBUG_v_TEST_) # define DEBUG_Uv_TEST_ (DEBUG_U_TEST_ && DEBUG_v_TEST_) # define DEBUG_Pv_TEST_ (DEBUG_P_TEST_ && DEBUG_v_TEST_) @@ -4237,6 +4239,7 @@ Gid_t getegid (void); # define DEBUG_M_TEST DEBUG_M_TEST_ # define DEBUG_B_TEST DEBUG_B_TEST_ # define DEBUG_L_TEST DEBUG_L_TEST_ +# define DEBUG_i_TEST DEBUG_i_TEST_ # define DEBUG_Xv_TEST DEBUG_Xv_TEST_ # define DEBUG_Uv_TEST DEBUG_Uv_TEST_ # define DEBUG_Pv_TEST DEBUG_Pv_TEST_ @@ -4292,6 +4295,7 @@ Gid_t getegid (void); # define DEBUG_M(a) DEBUG__(DEBUG_M_TEST, a) # define DEBUG_B(a) DEBUG__(DEBUG_B_TEST, a) # define DEBUG_L(a) DEBUG__(DEBUG_L_TEST, a) +# define DEBUG_i(a) DEBUG__(DEBUG_i_TEST, a) #else /* DEBUGGING */ @@ -4322,6 +4326,7 @@ Gid_t getegid (void); # define DEBUG_M_TEST (0) # define DEBUG_B_TEST (0) # define DEBUG_L_TEST (0) +# define DEBUG_i_TEST (0) # define DEBUG_Xv_TEST (0) # define DEBUG_Uv_TEST (0) # define DEBUG_Pv_TEST (0) @@ -4356,6 +4361,7 @@ Gid_t getegid (void); # define DEBUG_M(a) # define DEBUG_B(a) # define DEBUG_L(a) +# define DEBUG_i(a) # define DEBUG_Xv(a) # define DEBUG_Uv(a) # define DEBUG_Pv(a) diff --git a/perlio.c b/perlio.c index 11a66d0..20c2fa3 100644 --- a/perlio.c +++ b/perlio.c @@ -351,6 +351,10 @@ PerlIO_debug(const char *fmt, ...) va_list ap; dSYS; va_start(ap, fmt); + + if (!DEBUG_i_TEST) + return; + if (!PL_perlio_debug_fd) { if (!TAINTING_get && PerlProc_getuid() == PerlProc_geteuid() && @@ -477,7 +481,7 @@ PerlIO_fdupopen(pTHX_ PerlIO *f, CLONE_PARAMS *param, int flags) { if (PerlIOValid(f)) { const PerlIO_funcs * const tab = PerlIOBase(f)->tab; - PerlIO_debug("fdupopen f=%p param=%p\n",(void*)f,(void*)param); + DEBUG_i( PerlIO_debug("fdupopen f=%p param=%p\n",(void*)f,(void*)param) ); if (tab && tab->Dup) return (*tab->Dup)(aTHX_ PerlIO_allocate(aTHX), f, param, flags); else { @@ -586,7 +590,7 @@ PerlIO_clone(pTHX_ PerlInterpreter *proto, CLONE_PARAMS *param) PL_known_layers = PerlIO_clone_list(aTHX_ proto->Iknown_layers, param); PL_def_layerlist = PerlIO_clone_list(aTHX_ proto->Idef_layerlist, param); PerlIO_init_table(aTHX); - PerlIO_debug("Clone %p from %p\n",(void*)aTHX,(void*)proto); + DEBUG_i( PerlIO_debug("Clone %p from %p\n",(void*)aTHX,(void*)proto) ); while ((f = *table)) { int i; table = (PerlIOl **) (f++); @@ -610,7 +614,7 @@ PerlIO_destruct(pTHX) PerlIOl **table = &PL_perlio; PerlIOl *f; #ifdef USE_ITHREADS - PerlIO_debug("Destruct %p\n",(void*)aTHX); + DEBUG_i( PerlIO_debug("Destruct %p\n",(void*)aTHX) ); #endif while ((f = *table)) { int i; @@ -620,7 +624,7 @@ PerlIO_destruct(pTHX) const PerlIOl *l; while ((l = *x)) { if (l->tab && l->tab->kind & PERLIO_K_DESTRUCT) { - PerlIO_debug("Destruct popping %s\n", l->tab->name); + DEBUG_i( PerlIO_debug("Destruct popping %s\n", l->tab->name) ); PerlIO_flush(x); PerlIO_pop(aTHX_ x); } @@ -639,8 +643,8 @@ PerlIO_pop(pTHX_ PerlIO *f) const PerlIOl *l = *f; VERIFY_HEAD(f); if (l) { - PerlIO_debug("PerlIO_pop f=%p %s\n", (void*)f, - l->tab ? l->tab->name : "(Null)"); + DEBUG_i( PerlIO_debug("PerlIO_pop f=%p %s\n", (void*)f, + l->tab ? l->tab->name : "(Null)") ); if (l->tab && l->tab->Popped) { /* * If popped returns non-zero do not free its layer structure @@ -713,7 +717,7 @@ PerlIO_find_layer(pTHX_ const char *name, STRLEN len, int load) PerlIO_funcs * const f = PL_known_layers->array[i].funcs; const STRLEN this_len = strlen(f->name); if (this_len == len && memEQ(f->name, name, len)) { - PerlIO_debug("%.*s => %p\n", (int) len, name, (void*)f); + DEBUG_i( PerlIO_debug("%.*s => %p\n", (int) len, name, (void*)f) ); return f; } } @@ -741,7 +745,7 @@ PerlIO_find_layer(pTHX_ const char *name, STRLEN len, int load) return PerlIO_find_layer(aTHX_ name, len, 0); } } - PerlIO_debug("Cannot find %.*s\n", (int) len, name); + DEBUG_i( PerlIO_debug("Cannot find %.*s\n", (int) len, name) ); return NULL; } @@ -844,8 +848,9 @@ XS(XS_PerlIO__Layer__NoWarnings) */ dXSARGS; PERL_UNUSED_ARG(cv); - if (items) - PerlIO_debug("warning:%s\n",SvPV_nolen_const(ST(0))); + DEBUG_i( + if (items) + PerlIO_debug("warning:%s\n",SvPV_nolen_const(ST(0))) ); XSRETURN(0); } @@ -874,7 +879,7 @@ PerlIO_define_layer(pTHX_ PerlIO_funcs *tab) if (!PL_known_layers) PL_known_layers = PerlIO_list_alloc(aTHX); PerlIO_list_push(aTHX_ PL_known_layers, tab, NULL); - PerlIO_debug("define %s %p\n", tab->name, (void*)tab); + DEBUG_i( PerlIO_debug("define %s %p\n", tab->name, (void*)tab) ); } int @@ -979,7 +984,7 @@ PerlIO_default_buffer(pTHX_ PerlIO_list_t *av) if (PerlIO_stdio.Set_ptrcnt) tab = &PerlIO_stdio; #endif - PerlIO_debug("Pushing %s\n", tab->name); + DEBUG_i( PerlIO_debug("Pushing %s\n", tab->name) ); PerlIO_list_push(aTHX_ av, (PerlIO_funcs *)tab, &PL_sv_undef); } @@ -993,8 +998,8 @@ PerlIO_funcs * PerlIO_layer_fetch(pTHX_ PerlIO_list_t *av, IV n, PerlIO_funcs *def) { if (n >= 0 && n < av->cur) { - PerlIO_debug("Layer %" IVdf " is %s\n", n, - av->array[n].funcs->name); + DEBUG_i( PerlIO_debug("Layer %" IVdf " is %s\n", n, + av->array[n].funcs->name) ); return av->array[n].funcs; } if (!def) @@ -1145,9 +1150,9 @@ PerlIO_push(pTHX_ PerlIO *f, PERLIO_FUNCS_DECL(*tab), const char *mode, SV *arg) l->tab = (PerlIO_funcs*) tab; l->head = ((PerlIOl*)f)->head; *f = l; - PerlIO_debug("PerlIO_push f=%p %s %s %p\n", - (void*)f, tab->name, - (mode) ? mode : "(Null)", (void*)arg); + DEBUG_i( PerlIO_debug("PerlIO_push f=%p %s %s %p\n", + (void*)f, tab->name, + (mode) ? mode : "(Null)", (void*)arg) ); if (*l->tab->Pushed && (*l->tab->Pushed) (aTHX_ f, mode, arg, (PerlIO_funcs*) tab) != 0) { @@ -1161,8 +1166,8 @@ PerlIO_push(pTHX_ PerlIO *f, PERLIO_FUNCS_DECL(*tab), const char *mode, SV *arg) } else if (f) { /* Pseudo-layer where push does its own stack adjust */ - PerlIO_debug("PerlIO_push f=%p %s %s %p\n", (void*)f, tab->name, - (mode) ? mode : "(Null)", (void*)arg); + DEBUG_i( PerlIO_debug("PerlIO_push f=%p %s %s %p\n", (void*)f, tab->name, + (mode) ? mode : "(Null)", (void*)arg) ); if (tab->Pushed && (*tab->Pushed) (aTHX_ f, mode, arg, (PerlIO_funcs*) tab) != 0) { return NULL; @@ -1241,8 +1246,8 @@ PerlIORaw_pushed(pTHX_ PerlIO *f, const char *mode, SV *arg, PerlIO_funcs *tab) } } if (PerlIOValid(f)) { - PerlIO_debug(":raw f=%p :%s\n", (void*)f, - PerlIOBase(f)->tab ? PerlIOBase(f)->tab->name : "(Null)"); + DEBUG_i( PerlIO_debug(":raw f=%p :%s\n", (void*)f, + PerlIOBase(f)->tab ? PerlIOBase(f)->tab->name : "(Null)") ); return 0; } } @@ -1294,10 +1299,11 @@ PerlIO_apply_layers(pTHX_ PerlIO *f, const char *mode, const char *names) int PerlIO_binmode(pTHX_ PerlIO *f, int iotype, int mode, const char *names) { - PerlIO_debug("PerlIO_binmode f=%p %s %c %x %s\n", (void*)f, - (PerlIOBase(f) && PerlIOBase(f)->tab) ? - PerlIOBase(f)->tab->name : "(Null)", - iotype, mode, (names) ? names : "(Null)"); + DEBUG_i( + PerlIO_debug("PerlIO_binmode f=%p %s %c %x %s\n", (void*)f, + (PerlIOBase(f) && PerlIOBase(f)->tab) ? + PerlIOBase(f)->tab->name : "(Null)", + iotype, mode, (names) ? names : "(Null)") ); if (names) { /* Do not flush etc. if (e.g.) switching encodings. @@ -1530,9 +1536,9 @@ PerlIO_openn(pTHX_ const char *layers, const char *mode, int fd, if (narg > 1 && !(tab->kind & PERLIO_K_MULTIARG)) { Perl_croak(aTHX_ "More than one argument to open(,':%s')",tab->name); } - PerlIO_debug("openn(%s,'%s','%s',%d,%x,%o,%p,%d,%p)\n", - tab->name, layers ? layers : "(Null)", mode, fd, - imode, perm, (void*)f, narg, (void*)args); + DEBUG_i( PerlIO_debug("openn(%s,'%s','%s',%d,%x,%o,%p,%d,%p)\n", + tab->name, layers ? layers : "(Null)", mode, fd, + imode, perm, (void*)f, narg, (void*)args) ); if (tab->Open) f = (*tab->Open) (aTHX_ tab, layera, n, mode, fd, imode, perm, f, narg, args); @@ -1609,7 +1615,7 @@ Perl_PerlIO_flush(pTHX_ PerlIO *f) return 0; /* If no Flush defined, silently succeed. */ } else { - PerlIO_debug("Cannot flush f=%p\n", (void*)f); + DEBUG_i( PerlIO_debug("Cannot flush f=%p\n", (void*)f) ); SETERRNO(EBADF, SS_IVCHAN); return -1; } @@ -2001,9 +2007,11 @@ PerlIOBase_pushed(pTHX_ PerlIO *f, const char *mode, SV *arg, PerlIO_funcs *tab) } } #if 0 + DEBUG_i( PerlIO_debug("PerlIOBase_pushed f=%p %s %s fl=%08" UVxf " (%s)\n", (void*)f, PerlIOBase(f)->tab->name, (omode) ? omode : "(Null)", l->flags, PerlIO_modestr(f, temp)); + ); #endif return 0; } @@ -2187,9 +2195,9 @@ PerlIOBase_dup(pTHX_ PerlIO *f, PerlIO *o, CLONE_PARAMS *param, int flags) SV *arg = NULL; char buf[8]; assert(self); - PerlIO_debug("PerlIOBase_dup %s f=%p o=%p param=%p\n", - self->name, - (void*)f, (void*)o, (void*)param); + DEBUG_i(PerlIO_debug("PerlIOBase_dup %s f=%p o=%p param=%p\n", + self->name, + (void*)f, (void*)o, (void*)param) ); if (self->Getarg) arg = (*self->Getarg)(aTHX_ o, param, flags); f = PerlIO_push(aTHX_ f, self, PerlIO_modestr(o,buf), arg); @@ -2216,8 +2224,8 @@ S_more_refcounted_fds(pTHX_ const int new_fd) PERL_UNUSED_CONTEXT; #endif - PerlIO_debug("More fds - old=%d, need %d, new=%d\n", - old_max, new_fd, new_max); + DEBUG_i( PerlIO_debug("More fds - old=%d, need %d, new=%d\n", + old_max, new_fd, new_max) ); if (new_fd < old_max) { return; @@ -2239,9 +2247,9 @@ S_more_refcounted_fds(pTHX_ const int new_fd) PL_perlio_fd_refcnt_size = new_max; PL_perlio_fd_refcnt = new_array; - PerlIO_debug("Zeroing %p, %d\n", - (void*)(new_array + old_max), - new_max - old_max); + DEBUG_i( PerlIO_debug("Zeroing %p, %d\n", + (void*)(new_array + old_max), + new_max - old_max) ); Zero(new_array + old_max, new_max - old_max, int); } @@ -2273,8 +2281,8 @@ PerlIOUnix_refcnt_inc(int fd) Perl_croak(aTHX_ "refcnt_inc: fd %d: %d <= 0\n", fd, PL_perlio_fd_refcnt[fd]); } - PerlIO_debug("refcnt_inc: fd %d refcnt=%d\n", - fd, PL_perlio_fd_refcnt[fd]); + DEBUG_i( PerlIO_debug("refcnt_inc: fd %d refcnt=%d\n", + fd, PL_perlio_fd_refcnt[fd]) ); #ifdef USE_ITHREADS MUTEX_UNLOCK(&PL_perlio_mutex); @@ -2290,7 +2298,11 @@ PerlIOUnix_refcnt_dec(int fd) { int cnt = 0; if (fd >= 0) { +#ifdef DEBUGGING + dTHX; +#else dVAR; +#endif #ifdef USE_ITHREADS MUTEX_LOCK(&PL_perlio_mutex); #endif @@ -2305,7 +2317,7 @@ PerlIOUnix_refcnt_dec(int fd) fd, PL_perlio_fd_refcnt[fd]); } cnt = --PL_perlio_fd_refcnt[fd]; - PerlIO_debug("refcnt_dec: fd %d refcnt=%d\n", fd, cnt); + DEBUG_i( PerlIO_debug("refcnt_dec: fd %d refcnt=%d\n", fd, cnt) ); #ifdef USE_ITHREADS MUTEX_UNLOCK(&PL_perlio_mutex); #endif @@ -2352,9 +2364,9 @@ PerlIO_cleanup(pTHX) { int i; #ifdef USE_ITHREADS - PerlIO_debug("Cleanup layers for %p\n",(void*)aTHX); + DEBUG_i( PerlIO_debug("Cleanup layers for %p\n",(void*)aTHX) ); #else - PerlIO_debug("Cleanup layers\n"); + DEBUG_i( PerlIO_debug("Cleanup layers\n") ); #endif /* Raise STDIN..STDERR refcount so we don't close them */ @@ -2557,11 +2569,11 @@ PerlIOUnix_setfd(pTHX_ PerlIO *f, int fd, int imode) Stat_t st; if (PerlLIO_fstat(fd, &st) == 0) { if (!S_ISREG(st.st_mode)) { - PerlIO_debug("%d is not regular file\n",fd); + DEBUG_i( PerlIO_debug("%d is not regular file\n",fd) ); PerlIOBase(f)->flags |= PERLIO_F_NOTREG; } else { - PerlIO_debug("%d _is_ a regular file\n",fd); + DEBUG_i( PerlIO_debug("%d _is_ a regular file\n",fd) ); } } #endif @@ -4493,9 +4505,11 @@ PerlIOCrlf_pushed(pTHX_ PerlIO *f, const char *mode, SV *arg, PerlIO_funcs *tab) PerlIOBase(f)->flags |= PERLIO_F_CRLF; code = PerlIOBuf_pushed(aTHX_ f, mode, arg, tab); #if 0 + DEBUG_i( PerlIO_debug("PerlIOCrlf_pushed f=%p %s %s fl=%08" UVxf "\n", (void*)f, PerlIOBase(f)->tab->name, (mode) ? mode : "(Null)", PerlIOBase(f)->flags); + ); #endif { /* If the old top layer is a CRLF layer, reactivate it (if diff --git a/pod/perlrun.pod b/pod/perlrun.pod index 1ff3ce2..abe1af2 100644 --- a/pod/perlrun.pod +++ b/pod/perlrun.pod @@ -388,39 +388,41 @@ the format of the output is explained in L. As an alternative, specify a number instead of list of letters (e.g., B<-D14> is equivalent to B<-Dtls>): - 1 p Tokenizing and parsing (with v, displays parse stack) - 2 s Stack snapshots (with v, displays all stacks) - 4 l Context (loop) stack processing - 8 t Trace execution - 16 o Method and overloading resolution - 32 c String/numeric conversions - 64 P Print profiling info, source file input state - 128 m Memory and SV allocation - 256 f Format processing - 512 r Regular expression parsing and execution - 1024 x Syntax tree dump - 2048 u Tainting checks - 4096 U Unofficial, User hacking (reserved for private, - unreleased use) - 8192 H Hash dump -- usurps values() - 16384 X Scratchpad allocation - 32768 D Cleaning up - 65536 S Op slab allocation - 131072 T Tokenizing - 262144 R Include reference counts of dumped variables (eg when - using -Ds) - 524288 J show s,t,P-debug (don't Jump over) on opcodes within - package DB - 1048576 v Verbose: use in conjunction with other flags - 2097152 C Copy On Write - 4194304 A Consistency checks on internal structures - 8388608 q quiet - currently only suppresses the "EXECUTING" - message - 16777216 M trace smart match resolution - 33554432 B dump suBroutine definitions, including special Blocks - like BEGIN - 67108864 L trace Locale-related info; what gets output is very - subject to change + 1 p Tokenizing and parsing (with v, displays parse stack) + 2 s Stack snapshots (with v, displays all stacks) + 4 l Context (loop) stack processing + 8 t Trace execution + 16 o Method and overloading resolution + 32 c String/numeric conversions + 64 P Print profiling info, source file input state + 128 m Memory and SV allocation + 256 f Format processing + 512 r Regular expression parsing and execution + 1024 x Syntax tree dump + 2048 u Tainting checks + 4096 U Unofficial, User hacking (reserved for private, + unreleased use) + 8192 H Hash dump -- usurps values() + 16384 X Scratchpad allocation + 32768 D Cleaning up + 65536 S Op slab allocation + 131072 T Tokenizing + 262144 R Include reference counts of dumped variables (eg when + using -Ds) + 524288 J show s,t,P-debug (don't Jump over) on opcodes within + package DB + 1048576 v Verbose: use in conjunction with other flags + 2097152 C Copy On Write + 4194304 A Consistency checks on internal structures + 8388608 q quiet - currently only suppresses the "EXECUTING" + message + 16777216 M trace smart match resolution + 33554432 B dump suBroutine definitions, including special Blocks + like BEGIN + 67108864 L trace Locale-related info; what gets output is very + subject to change + 134217728 i trace PerlIO layer processing. Set PERLIO_DEBUG to + the filename to trace to. All these flags require B<-DDEBUGGING> when you compile the Perl executable (but see C<:opd> in L or L -- 2.1.4 ```

[p5pRT]

From @lightsey

On 3/27/16 4​:59 PM\, Ricardo Signes via RT wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

This is a different issue that cPanel reported to the EXIM devs. They weren't doing any sanitization of the environment before starting the embedded Perl interpreter in a setuid binary.

The flaw in EXIM is what actually led us to look at Perl's taint mode handling as a possible solution since EXIM doesn't use taint mode.

The final fix was to clear the environment completely instead.

[p5pRT]

From @demerphq

On 28 March 2016 at 04​:56\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Sun\, Mar 27\, 2016 at 08​:28​:41PM -0500\, Craig A. Berry wrote​:

On Sun\, Mar 27\, 2016 at 7​:58 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

The PERL_HASH_SEED_DEBUG code doesn't call PerlIO_debug().

PerlIO_debug() is purely used for internal debugging by PerlIO - it's a simple output function that doesn't depend on the rest of PerlIO.

Ah\, right. How quickly I forget. So PERL_HASH_SEED and other things that do I/O early in start-up should become safe if PERLIO_DEBUG goes away and you have to use -Di rather than an environment variable to get a PerlIO log file open. I guess -Di would have to take a name​: -Di=debug.log.

I was thinking we'd only look at PERLIO_DEBUG if -Di was present. -Di wouldn't take a filename parameter.

Perhaps -Di without PERLIO_DEBUG would default to stderr.

You mean when taint is enabled?

Yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[p5pRT]

From @demerphq

On 2 February 2016 at 19​:20\, Craig A. Berry \craig\.a\.berry@&#8203;gmail\.com wrote​:

On Mon\, Feb 1\, 2016 at 6​:42 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu Jan 28 17​:15​:54 2016\, craig.a.berry@​gmail.com wrote​:

On Thu\, Jan 28\, 2016 at 5​:11 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

We're also still honoring PERL_HASH_SEED and PERL_HASH_SEED_DEBUG with -T (and with euid != uid.)

And I'm thinking this is as it should be\, but please let me know if I'm missing something.

It's an issue because an attacker can set PERL_HASH_SEED to a known bad seed and cause the child process to build a linear HV and use much more CPU than expected.

Ah\, ok. Then we have a chicken-and-egg problem because you must initialize the seed before creating any HVs\, and we start creating HVs very early in perl_construct. We don't know whether tainting is enabled at the command line until well into perl_parse. So I don't see a way to disable PERL_HASH_SEED under -T\, though we could do so under euid != uid because that check is early enough.

Wow. I am really surprised how late arguments to perl are parsed\, and how little separation of concerns there are in our setup process.

It really should be possible to parse ours args before we setup the perl interpreter\, but well\, as the old joke goes\, if you want to go /there/\, I wouldn't start from /here/.

I guess Tony's idea of making PERL_HASH_SEED* env vars only work on DEBUGGING builds is the only option. In hindsight this was/is a surprisingly obvious problem that is surprisingly inobvious how to solve with our current code. Which is a pity.

For instance with current code there is no way we could supply the hash seed as an argument to perl. Which just seems wrong to me.

cheers\, Yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[p5pRT]

From @tonycoz

On Mon Mar 28 09​:37​:43 2016\, demerphq wrote​:

On 28 March 2016 at 04​:56\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Sun\, Mar 27\, 2016 at 08​:28​:41PM -0500\, Craig A. Berry wrote​:

On Sun\, Mar 27\, 2016 at 7​:58 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

The PERL_HASH_SEED_DEBUG code doesn't call PerlIO_debug().

PerlIO_debug() is purely used for internal debugging by PerlIO - it's a simple output function that doesn't depend on the rest of PerlIO.

Ah\, right. How quickly I forget. So PERL_HASH_SEED and other things that do I/O early in start-up should become safe if PERLIO_DEBUG goes away and you have to use -Di rather than an environment variable to get a PerlIO log file open. I guess -Di would have to take a name​: -Di=debug.log.

I was thinking we'd only look at PERLIO_DEBUG if -Di was present. -Di wouldn't take a filename parameter.

Perhaps -Di without PERLIO_DEBUG would default to stderr.

You mean when taint is enabled?

No\, I was thinking​:

  perl -Di ... # dump to STDERR   PERLIO_DEBUG=logfile perl -Di ... # dump to logfile

My patch doesn't include that behaviour.

With -Di required\, I think we could ignore taint for PERLIO_DEBUG\, if someone builds a debug perl\, enables taint (via -T or setuid) and adds -Di and then complains I don't think they have much of a case when PERLIO_DEBUG overwrites their files.

Or we could just make -Di always write to STDERR like every other -D switch does.

Tony

[p5pRT]

From @tonycoz

On Mon Mar 28 10​:05​:48 2016\, demerphq wrote​:

On 2 February 2016 at 19​:20\, Craig A. Berry \craig\.a\.berry@&#8203;gmail\.com wrote​:

On Mon\, Feb 1\, 2016 at 6​:42 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu Jan 28 17​:15​:54 2016\, craig.a.berry@​gmail.com wrote​:

On Thu\, Jan 28\, 2016 at 5​:11 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

We're also still honoring PERL_HASH_SEED and PERL_HASH_SEED_DEBUG with -T (and with euid != uid.)

And I'm thinking this is as it should be\, but please let me know if I'm missing something.

It's an issue because an attacker can set PERL_HASH_SEED to a known bad seed and cause the child process to build a linear HV and use much more CPU than expected.

Ah\, ok. Then we have a chicken-and-egg problem because you must initialize the seed before creating any HVs\, and we start creating HVs very early in perl_construct. We don't know whether tainting is enabled at the command line until well into perl_parse. So I don't see a way to disable PERL_HASH_SEED under -T\, though we could do so under euid != uid because that check is early enough.

Wow. I am really surprised how late arguments to perl are parsed\, and how little separation of concerns there are in our setup process.

It really should be possible to parse ours args before we setup the perl interpreter\, but well\, as the old joke goes\, if you want to go /there/\, I wouldn't start from /here/.

One option I considered (with my head buried in the switch parsing code) was requiring -T be the *first* switch. That would simplify parsing -T earlier so code that depends on PL_tainting won't see a false negative.

That won't work for maint\, but it's something to consider for the future.

There's already Perl_doing_taint() which checks for -T in the first argument\, but it seems to only be used with -Dmymalloc

I guess Tony's idea of making PERL_HASH_SEED* env vars only work on DEBUGGING builds is the only option. In hindsight this was/is a surprisingly obvious problem that is surprisingly inobvious how to solve with our current code. Which is a pity.

I was mostly thinking of PERLIO_DEBUG\, since a -D\ switch works well there (and it's more directly dangerous.)

Unfortunately Perl_doing_taint() requires argc and argv\, so we can't use it to make the PERL_HASH_SEED code safer.

Tony

[p5pRT]

From @iabyn

On Tue\, Jan 26\, 2016 at 09​:55​:03AM -0800\, John Lightsey wrote​:

While investigating taint mode as a possible solution to other flaws\, we came across a flaw in the implementation of taint mode itself.

I'd like to make a general comment here. Currently our start-up code is a complete mess\, and virtually unaditable for security correctness.

I'd suggest that post-5.24.0\, we (propbably Tony or I perhaps) move all the code that handles parsing flags\, setting taint\, looking at euid\, getting the values of ENV vars\, etc\, into a separate source file (called secinit.c say - for secure init)\, then rework the hell out of it. For example\, off the top of my head​:

* Coalesce the at least three places which parse perl flags\, into a single function that can if necessary be called multiple times\, possibly with a 'phase' arg that determines what actions are skipped each time. That would remove any requiring for -T to be the first arg\, for example.

* Use #defines etc so that certain functions are only available within this one source file and not the rest of the code. So any other code in the core that tried to mess with uid's\, say\, would see a compile error. Possibly env vars would only be accessible from within the src file too\, plus accessor functions to allow %ENV etc to work?

* Have warn/die functions in secinit.c which can test whether PerlIO has been initialised yet\, and if not try printing directly to FD 2 or the platform equivalent. The normal warn/die functions might then be made inaccessible from within that src file.

The above is all very hand-wavey\, but hopefully I've expressed the general idea.

-- "Emacs isn't a bad OS once you get used to it. It just lacks a decent editor."

[p5pRT]

From @tux

On Fri\, 1 Apr 2016 16​:11​:49 +0100\, Dave Mitchell \davem@&#8203;iabyn\.com wrote​:

On Tue\, Jan 26\, 2016 at 09​:55​:03AM -0800\, John Lightsey wrote​:

While investigating taint mode as a possible solution to other flaws\, we came across a flaw in the implementation of taint mode itself.

I'd like to make a general comment here. Currently our start-up code is a complete mess\, and virtually unaditable for security correctness.

I'd suggest that post-5.24.0\, we (propbably Tony or I perhaps) move all the code that handles parsing flags\, setting taint\, looking at euid\, getting the values of ENV vars\, etc\, into a separate source file (called secinit.c say - for secure init)\, then rework the hell out of it.

+1

For example\, off the top of my head​:

* Coalesce the at least three places which parse perl flags\, into a single function that can if necessary be called multiple times\, possibly with a 'phase' arg that determines what actions are skipped each time. That would remove any requiring for -T to be the first arg\, for example.

* Use #defines etc so that certain functions are only available within this one source file and not the rest of the code. So any other code in the core that tried to mess with uid's\, say\, would see a compile error. Possibly env vars would only be accessible from within the src file too\, plus accessor functions to allow %ENV etc to work?

* Have warn/die functions in secinit.c which can test whether PerlIO has been initialised yet\, and if not try printing directly to FD 2 or the platform equivalent. The normal warn/die functions might then be made inaccessible from within that src file.

The above is all very hand-wavey\, but hopefully I've expressed the general idea.

-- H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/ using perl5.00307 .. 5.23 porting perl5 on HP-UX\, AIX\, and openSUSE http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/ http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

[p5pRT]

From @timbunce

On Fri\, Apr 01\, 2016 at 04​:11​:49PM +0100\, Dave Mitchell wrote​:

On Tue\, Jan 26\, 2016 at 09​:55​:03AM -0800\, John Lightsey wrote​:

While investigating taint mode as a possible solution to other flaws\, we came across a flaw in the implementation of taint mode itself.

I'd like to make a general comment here. Currently our start-up code is a complete mess\, and virtually unaditable for security correctness.

I'd suggest that post-5.24.0\, we (propbably Tony or I perhaps) move all the code that handles parsing flags\, setting taint\, looking at euid\, getting the values of ENV vars\, etc\, into a separate source file (called secinit.c say - for secure init)\, then rework the hell out of it.

+1

Tim.

[p5pRT]

From @craigberry

On Sun\, Mar 27\, 2016 at 11​:05 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Sun Mar 27 16​:31​:59 2016\, tonyc wrote​:

Maybe we're approaching this the wrong way.

Right now\, PERLIO_DEBUG is enabled in non--DDEBUGGING perls\, and file creation/output is enabled based on the environment variable.

Perhaps​:

a) it should only be enabled on -DDEBUGGING perls\, and

b) require a -Di flag (which appears to be unused)

This avoids most of the mess from PL_tainting not being available early enough.

It's technically back-incompatible for maint\, but it's a feature that's mostly of used in debugging PerlIO layers\, for which you can use a -DDEBUGGING perl or a debugger.

Patch.

I didn't remove PerlIO_debug itself under -DDEBUGGING\, since CPAN modules might be using it without a -DDEBUGGING guard (and this might go into maint.)

The patch works as advertised. This is really what PerlIO debugging should have done originally.

Having to set both PERLIO_DEBUG *and* use -Di is a slightly weird interface\, but making it default to stderr as previously suggested would address that. Or doing away with PERLIO_DEBUG in the environment entirely.

Either way that's a change to documented behavior. Is there any sane (or only slightly insane) real world use of PERLIO_DEBUG that would get broken? Probably not\, but I don't get out much.

[p5pRT]

From @craigberry

On Fri\, Apr 1\, 2016 at 10​:40 AM\, H.Merijn Brand \h\.m\.brand@&#8203;xs4all\.nl wrote​:

On Fri\, 1 Apr 2016 16​:11​:49 +0100\, Dave Mitchell \davem@&#8203;iabyn\.com wrote​:

On Tue\, Jan 26\, 2016 at 09​:55​:03AM -0800\, John Lightsey wrote​:

While investigating taint mode as a possible solution to other flaws\, we came across a flaw in the implementation of taint mode itself.

I'd like to make a general comment here. Currently our start-up code is a complete mess\, and virtually unaditable for security correctness.

I'd suggest that post-5.24.0\, we (propbably Tony or I perhaps) move all the code that handles parsing flags\, setting taint\, looking at euid\, getting the values of ENV vars\, etc\, into a separate source file (called secinit.c say - for secure init)\, then rework the hell out of it.

+1

Sounds like a plan. If the new argument parsing is called from PERL_SYS_INIT3\, it might even be possible to avoid breaking source compatibility for embedders.

For example\, off the top of my head​:

* Coalesce the at least three places which parse perl flags\, into a single function that can if necessary be called multiple times\, possibly with a 'phase' arg that determines what actions are skipped each time. That would remove any requiring for -T to be the first arg\, for example.

* Use #defines etc so that certain functions are only available within this one source file and not the rest of the code. So any other code in the core that tried to mess with uid's\, say\, would see a compile error. Possibly env vars would only be accessible from within the src file too\, plus accessor functions to allow %ENV etc to work?

* Have warn/die functions in secinit.c which can test whether PerlIO has been initialised yet\, and if not try printing directly to FD 2 or the platform equivalent. The normal warn/die functions might then be made inaccessible from within that src file.

A slightly different approach I've been thinking about would do all the command line parsing very early and save the results in a plain old C struct. As the different bits of the interpreter get instantiated\, we would copy those results into the relevant places in the interpreter struct. At the end of start-up the original struct could be zeroed and freed.

This based on the observation that some of the trouble we're in comes from trying to do everything as Perlishly as possible\, even if Perl doesn't exist yet (or only half exists). This might be a way to separate concerns somewhat.

[p5pRT]

From @jhi

On Friday-201604-01 11​:11\, Dave Mitchell wrote​:

I'd suggest that post-5.24.0\, we (propbably Tony or I perhaps) move all the code that handles parsing flags\, setting taint\, looking at euid\, getting the values of ENV vars\, etc\, into a separate source file (called secinit.c say - for secure init)\, then rework the hell out of it. For example\, off the top of my head​:

A smashing idea. But just call it init.c.

[p5pRT]

From @tonycoz

On Sun Mar 27 21​:05​:28 2016\, tonyc wrote​:

On Sun Mar 27 16​:31​:59 2016\, tonyc wrote​:

On Sun\, Mar 27\, 2016 at 05​:58​:41PM -0400\, Ricardo Signes wrote​:

* "Craig A. Berry" \craig\.a\.berry@&#8203;gmail\.com [2016-03-25T22​:55​:42]

Thanks. What can I do to move this forward? If we don't need an embargo\, can I just put the patch on a smoke-me branch and merge if all is well? I don't know anything about how the ticket gets moved to the public queue.

I guess we don't\, because it looks like someone has effectively disclosed a significant part of this​:

https://twitter.com/hackerfantastic/status/707314526107127809

I'm not sure that's the same issue\, since I'm not sure taint is involved.

Maybe we're approaching this the wrong way.

Right now\, PERLIO_DEBUG is enabled in non--DDEBUGGING perls\, and file creation/output is enabled based on the environment variable.

Perhaps​:

a) it should only be enabled on -DDEBUGGING perls\, and

b) require a -Di flag (which appears to be unused)

This avoids most of the mess from PL_tainting not being available early enough.

It's technically back-incompatible for maint\, but it's a feature that's mostly of used in debugging PerlIO layers\, for which you can use a -DDEBUGGING perl or a debugger.

Patch.

I didn't remove PerlIO_debug itself under -DDEBUGGING\, since CPAN modules might be using it without a -DDEBUGGING guard (and this might go into maint.)

Patch with tests and another to use STDERR when PERLIO_DEBUG isn't set (or when tainting).

We've had some discussion on what to do in 5.25 - what do we want to do for maint and 5.24?

On Fri\, 1 Apr 2016 11​:41​:36 -0500\, Craig A. Berry wrote​:

Having to set both PERLIO_DEBUG *and* use -Di is a slightly weird interface\, but making it default to stderr as previously suggested would address that. Or doing away with PERLIO_DEBUG in the environment entirely.

Either way that's a change to documented behavior. Is there any sane (or only slightly insane) real world use of PERLIO_DEBUG that would get broken? Probably not\, but I don't get out much.

I could *maybe* see test code depending on it. I don't see anything in CPAN this would break.

But as is it's a serious security issue - I think the security fix outways backwards compatibility here.

Tony

[p5pRT]

From @tonycoz

0003-perl-127380-default-PERLIO_DEBUG-Di-to-use-STDERR.patch

```diff From 377ba011482e9b35cb36b2fb74716f4de818aae1 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Thu, 7 Apr 2016 15:35:42 +1000 Subject: (perl #127380) default PERLIO_DEBUG/-Di to use STDERR This includes under taint, just as other -D switches write to stderr when taint is on. --- perlio.c | 8 ++++---- t/run/switchDx.t | 17 ++++++++++++++--- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/perlio.c b/perlio.c index 20c2fa3..27710e3 100644 --- a/perlio.c +++ b/perlio.c @@ -364,11 +364,11 @@ PerlIO_debug(const char *fmt, ...) PL_perlio_debug_fd = PerlLIO_open3(s, O_WRONLY | O_CREAT | O_APPEND, 0666); else - PL_perlio_debug_fd = -1; + PL_perlio_debug_fd = PerlLIO_dup(2); /* stderr */ } else { - /* tainting or set*id, so ignore the environment, and ensure we - skip these tests next time through. */ - PL_perlio_debug_fd = -1; + /* tainting or set*id, so ignore the environment and send the + debug output to stderr, like other -D switches. */ + PL_perlio_debug_fd = PerlLIO_dup(2); /* stderr */ } } if (PL_perlio_debug_fd > 0) { diff --git a/t/run/switchDx.t b/t/run/switchDx.t index aa7ecf9..bca46e0 100644 --- a/t/run/switchDx.t +++ b/t/run/switchDx.t @@ -12,7 +12,7 @@ skip_all "DEBUGGING build required" unless $::Config{ccflags} =~ /DEBUGGING/ or $^O eq 'VMS' && $::Config{usedebugging_perl} eq 'Y'; -plan tests => 6; +plan tests => 8; my $perlio_log = "perlio$$.txt"; END { @@ -31,8 +31,19 @@ END { ok(-e $perlio_log, "... perlio debugging file found with -Di and PERLIO_DEBUG"); unlink $perlio_log; - fresh_perl_is("print qq(hello\n)", "\nEXECUTING...\n\nhello\n", + fresh_perl_like("print qq(hello\n)", qr/define raw/, { stderr => 1, switches => [ "-TDi" ] }, - "No perlio debug file with -T.."); + "Perlio debug output to stderr with -TDi (with PERLIO_DEBUG)..."); ok(!-e $perlio_log, "...no perlio debugging file found"); } + +{ + local $ENV{PERLIO_DEBUG}; + fresh_perl_like("print qq(hello)", qr/define raw/, + { stderr => 1, switches => [ '-Di' ] }, + "-Di defaults to stderr"); + fresh_perl_like("print qq(hello)", qr/define raw/, + { stderr => 1, switches => [ '-TDi' ] }, + "Perlio debug output to STDERR with -TDi (no PERLIO_DEBUG)"); +} + -- 2.1.4 ```

[p5pRT]

From @craigberry

On Thu\, Apr 7\, 2016 at 12​:58 AM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

Patch with tests and another to use STDERR when PERLIO_DEBUG isn't set (or when tainting).

I can only find two patches​:

0001-perl-127380-only-trace-to-PERLIO_DEBUG-if-Di-is-supp.patch 0003-perl-127380-default-PERLIO_DEBUG-Di-to-use-STDERR.patch

The 0002-xxx patch seems to be missing and I assume it would create t/run/switchDx.t because 0003 tries to modify it and it's not there.

We've had some discussion on what to do in 5.25 - what do we want to do for maint and 5.24?

I suggest we move forward with Tony's patches as soon as we get all the pieces together. Then we should tweak the docs to PERLIO_DEBUG to correspond with the new reality.

[p5pRT]

From @tonycoz

On Mon Apr 11 19​:20​:57 2016\, craig.a.berry@​gmail.com wrote​:

On Thu\, Apr 7\, 2016 at 12​:58 AM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

Patch with tests and another to use STDERR when PERLIO_DEBUG isn't set (or when tainting).

I can only find two patches​:

0001-perl-127380-only-trace-to-PERLIO_DEBUG-if-Di-is-supp.patch 0003-perl-127380-default-PERLIO_DEBUG-Di-to-use-STDERR.patch

The 0002-xxx patch seems to be missing and I assume it would create t/run/switchDx.t because 0003 tries to modify it and it's not there.

Thanks.

Attached.

Tony

[p5pRT]

From @tonycoz

0002-perl-127380-add-tests-for-Di.patch

```diff From c397302755ed84fc40d4c6489da743be9368f6d5 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Thu, 7 Apr 2016 15:14:29 +1000 Subject: (perl #127380) add tests for -Di --- MANIFEST | 1 + t/run/switchDx.t | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 t/run/switchDx.t diff --git a/MANIFEST b/MANIFEST index 8690604..91ca4e0 100644 --- a/MANIFEST +++ b/MANIFEST @@ -5542,6 +5542,7 @@ t/run/switcha.t Test the -a switch t/run/switchC.t Test the -C switch t/run/switchd-78586.t See whether bug 78586 is fixed t/run/switchd.t Test the -d switch +t/run/switchDx.t Test the -D switch t/run/switches.t Tests for the other switches (-0, -l, -c, -s, -M, -m, -V, -v, -h, -z, -i) t/run/switchF1.t Pathological tests for the -F switch t/run/switchF2.t Pathological tests for the -F switch diff --git a/t/run/switchDx.t b/t/run/switchDx.t new file mode 100644 index 0000000..aa7ecf9 --- /dev/null +++ b/t/run/switchDx.t @@ -0,0 +1,38 @@ +#!./perl -w +BEGIN { + chdir 't' if -d 't'; + @INC = '../lib'; + require './test.pl'; + skip_all_if_miniperl(); +} + +use Config; + +skip_all "DEBUGGING build required" + unless $::Config{ccflags} =~ /DEBUGGING/ + or $^O eq 'VMS' && $::Config{usedebugging_perl} eq 'Y'; + +plan tests => 6; + +my $perlio_log = "perlio$$.txt"; +END { + unlink $perlio_log; +} +{ + unlink $perlio_log; + local $ENV{PERLIO_DEBUG} = $perlio_log; + fresh_perl_is("print qq(hello\n)", "hello\n", + { stderr => 1 }, + "No perlio debug file without -Di..."); + ok(!-e $perlio_log, "...no perlio.txt found"); + fresh_perl_is("print qq(hello\n)", "\nEXECUTING...\n\nhello\n", + { stderr => 1, switches => [ "-Di" ] }, + "Perlio debug file with both -Di and PERLIO_DEBUG..."); + ok(-e $perlio_log, "... perlio debugging file found with -Di and PERLIO_DEBUG"); + + unlink $perlio_log; + fresh_perl_is("print qq(hello\n)", "\nEXECUTING...\n\nhello\n", + { stderr => 1, switches => [ "-TDi" ] }, + "No perlio debug file with -T.."); + ok(!-e $perlio_log, "...no perlio debugging file found"); +} -- 2.1.4 ```

[p5pRT]

From @craigberry

On Mon\, Apr 11\, 2016 at 10​:58 PM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Mon Apr 11 19​:20​:57 2016\, craig.a.berry@​gmail.com wrote​:

The 0002-xxx patch seems to be missing and I assume it would create t/run/switchDx.t because 0003 tries to modify it and it's not there.

One small nit here. The test throws a warning when skipped on non-DEBUGGING build​:

$ ./perl -Ilib run/switchDx.t 1..0 # Skip DEBUGGING build required Use of uninitialized value $perlio_log in unlink at run/switchDx.t line 19.

Easily fixed by moving the declaration of $perlio_log up above the skip_all​:

Inline Patch

```diff diff --git a/t/run/switchDx.t b/t/run/switchDx.t index bca46e0..acb2995 100644 --- a/t/run/switchDx.t +++ b/t/run/switchDx.t @@ -8,13 +8,14 @@ BEGIN { use Config; +my $perlio_log = "perlio$$.txt"; + skip_all "DEBUGGING build required" unless $::Config{ccflags} =~ /DEBUGGING/ or $^O eq 'VMS' && $::Config{usedebugging_perl} eq 'Y'; plan tests => 8; -my $perlio_log = "perlio$$.txt"; END { unlink $perlio_log; } ```