[PATCH] Make Module::Build set PERL_UNSAFE_INC. (CVE-2016-1238)

[gregoa]

In Debian we are currently applying the following patch to
Module-Build.
We thought you might be interested in it too.

From: Niko Tyni <ntyni@debian.org>
Date: Fri, 8 Jul 2016 15:55:37 +0200
Subject: [PATCH] Make Module::Build set PERL_UNSAFE_INC.

Cf. CVE-2016-1238

Author: Todd Rinaldo <toddr@cpan.org>
Origin: https://gist.githubusercontent.com/toddr/d77d8d5fa9caa8f96b7758a126caa4dc/raw/3b1a327efdd9a6babf5eed8fb9c241a6d4909be6/fix.patch

The patch is tracked in our Git repository at
https://anonscm.debian.org/cgit/pkg-perl/packages/libmodule-build-perl.git/plain/debian/patches/0004-Make-Module-Build-set-PERL_UNSAFE_INC.patch

Thanks for considering,
gregor herrmann,
Debian Perl Group

[kenahoo]

Hi Gregor,

Do you have any insight into why this patch originated? Was there a problem it corrects?

[karenetheridge]

https://www.cve.mitre.org/cgi-bin/cvename.cgi?name=2016-1238
https://code.activestate.com/lists/perl5-porters/231180/

[gregoa]

It's about "CVE-2016-1238: Important unsafe module load path flaw", cf. e.g. https://code.activestate.com/lists/perl5-porters/231168/

https://rt.perl.org/Public/Bug/Display.html?id=127810 shows some history, and contains the patches for EUMM which I understand have been merged and released today. (https://rt.perl.org/Ticket/Display.html?id=127834 is currently not public but is referenced in lots of commits in maint-5.2{2,4}.)

Niko's patch does the same for Module::Build in my understanding.

Hope that clears it up a bit ;)
gregor

[gregoa]

Oh, and the Debian Security Announcement also has a summary: https://www.debian.org/security/2016/dsa-3628

[Leont]

I'm not sure this patch is completely sensible. If the build/test/install stages need "." to be in @INC, then probably so does the configuration stage. Hence adding it only in the former is a bit odd to me.

And likewise to me it would make more sense to set PERL_USE_UNSAFE_INC} in the build stages iff it is also set during configuration (though that may make more sense ), though there may not be much practical alternative.

The point of that code (for better or worse) in the Build script is to remember the situation during configuration time, this patch is kind of breaking that symmetry.

[jmdh]

PERL_USE_UNSAFE_INC=1 does also need to be set in Build.PL's environment, and the general consensus in https://rt.perl.org/Ticket/Display.html?id=127810 seemed to be that should be done by the invoker (eg cpan clients, and in Debian the Debian-specific build tools). That together with this patch certainly fixed a large class of build failures we discovered when removing '.' from @inc. It's possible that we've jumped the gone on forwarding this upstream, in which case apologies.

Mentioning @Nynti and @toddr as other expert parties...

[toddr]

@gregoa @jmdh @Leont @karenetheridge @kenahoo:

Where the environment variable gets injected seems like a detail I agree should be discussed. But I think it's important we first get agreement in the Perl RT about the plan to use the environment variable to begin with. https://rt.perl.org/Public/Bug/Display.html?id=127810#txn-1412454

The ticket necessarily had to wait for public disclosure of the CVE but now is the time to move forward. Right now I'm getting silence about moving forward with my original suggested approach and I'd really like to get toolchain gang to participate in the conversation. I hope to hear from you there.

Todd

[karenetheridge]

But I think it's important we first get agreement in the Perl RT about the plan to use the environment variable to begin with.

My first question: what happens in newer perls if Module::Build doesn't make any changes? Do distributions become unbuildable? How and why?

[toddr]

Summary:

1. blead a.k.a. 5.26 get changed so unless you use a magic incantation, . is not in @inc by default. This is acually ok for almost everything except the build stack.
2. M:B, M::B::Tiny, EUMM, CPAN shells get changed so that PERL_UNSAFE_INC=1 is set during build. This causes Perl to re-enable . in @inc on all executions of perl for that process and more importantly all child process. This is why the env var has to be used because you need the entire build stack to have access to . in @inc.

So I guess my response is EUMM/M:B have to be changed in conjunction with perl's removal of . by default or this won't work.

[toddr]

also for reference https://github.com/toddr/perl/issues/6 and https://github.com/toddr/perl/issues/4

[toddr]

blead now provides an @inc without . via the compile option -Ddefault_inc_excludes_dot. Under this option, @inc can be forced to have . in @inc if the environment variable PERL_USE_UNSAFE_INC is set to 1.

This RT has been opnened and is currently a blocker for 5.26 to make this a default build option. https://rt.perl.org/Public/Bug/Display.html?id=130467

Prior to us flipping this, I would like to get support for this in Module::Build. I'm going to submit a pull request in a minute. Hopefully we can either merge or discuss alternatives to where this needs to go? If I recall 18 month ago Todd, the location chosen for getting it into the M::B environment was a little arbitrary so I'm open to to ideas.