New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base.pm broken in Perl 5.24.1 rc2 #15490
Comments
From chris.travers@gmail.comWhen relying on the @INC including the current working directory base no longer follows the same resolution path as use, leading to very complex and difficult to debug error messages. It moreover no longer even follows its existing documentation and this leads to very strange, ordering-dependent bugs which will break a *lot* of software. Please fix it properly or not at all. This idea of mostly supporting . in the INC but not really is going to cause a lot of people a LOT of headache and lead to systems which are not really any more securable. See my blog post http://ledgersmbdev.blogspot.se/2016/07/notes-on-security-separation-of.html on the problems with this fix |
From chris.travers@gmail.comHere is a link to a downstream bug report to Debian based on the same changes: Again, the big problem is that the problems that this patch introduces to this module are very opaque and very difficult to troubleshoot. Frankly removing . from @INC would be less disruptive and less painful than expecting developers to figure out the strange, ordering dependent bugs that come out of this one. |
From zefram@fysh.orgChris Travers wrote:
We *are* going to fix it properly: we're heading in the direction of The changes that you're seeing now are those expedient fixes. Yes, base.pm is especially problematic, because it gets used a lot more than To avoid these problems, you should use parent.pm instead of base.pm. -zefram |
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgI wrote:
And I forgot to say: if for a particular application you really want a -zefram |
From chris.travers@gmail.comRemoving . from @INC would frankly cause fewer problems than this modification to base.pm. There are several very significant problems here: 1. @INC no longer has a distinct meaning which makes debugging far harder In my view, support . in @INC or not but don't do this funny half-way way which makes undocumented (and probably undocumentable) changes to behavior. I wear a lot of hats. I do system administration, software development, some pen testing. Debian's rollout of this change broke things that took me a day to debug (because nobody ever expects that problems will come from undocumented behavior changes in standard libraries), and I don't think I will be alone there. When I found out the patch was here too, I filed this bug report. But then I asked myself, "what is the scope of this vulnerability?" and "Are my systems at risk?" After careful review I determined the business systems I had built were not, but that this was a real problem with a far reach, and that these patches don't actually help that much in securing a system from a sysadmin perspective. I recognize that this is a difficult situation, and security is important to me too. And I recognize there is probably intense pressure to release a fix yesterday. But this is a bad idea and if you go this route, you will be answering variations of this bug report every day for years to come. One of the things Debian did right was to introduce a system setting to disable implicit current working directories in @INC. I recognize that with a portable language like Perl, that may not be practical. But you have a lot of options that would be real solutions to this problem which would be a lot better than what you have now. They include: 1. A simple compile time option to fix the behavior (woohoo! Perl fixed! now the downstream distributors have to decide whether to release two versions or just the fixed one) 2. Make a fallback if an environment variable is set. There are probably half a dozen other approaches that would allow for people who still need the old behavior to get it without introducing strange, order-dependent, opaque bugs into large numbers of Perl codebases out there. A second point is that there are security implications for perl scripts out there and that fixing this on the module level fails to account for the fact that the module is not in a position to understand the implications of the change. As it stands right now, my view is that system administrators are the only ones who can secure their systems, and that it is critical that they understand the problems in this area. So for that reason, I have started a full disclosure series on this problem discussing different attack scenarios and how system administrators can prevent them (from what to look for in scripts that run on their systems, how to secure scripts, including ones they write themselves, and how to avoid problems from running programs like prove). Every exploit I plan to discuss will work with these patches in place so that system administrators are in a better position to assess vulnerability of their systems. It is humanly possible to secure a system with this issue. But I am not convinced it is humanly possible to understand the implications of making this the module's responsibility. |
From @craigberryOn Mon, Aug 1, 2016 at 10:13 PM, Chris Travers via RT
That wasn't the conclusion of the extensive discussions that have I expect some version of removing '.' from @INC will happen (probably As far as things that can go wrong with base.pm being too subtle, |
From @xsawyerxChris, while I understand the objections you have and I appreciate this adds possible complications for you, I think you're missing background and you are making incorrect assumptions. You are both making assumptions on us not considering things you have (surprise, we have...) and on how easy it would be to solve the problem the way you suggest - which ignores backwards compatibility assurance, different security mechanisms across a multitude of operating systems and file systems, and even code complexity relating to magical entries in @INC. I intend to reject this ticket unless I find a reasonable objection. |
From chris.travers@gmail.comOn Tue Aug 02 09:47:32 2016, xsawyerx@cpan.org wrote:
We fixed our complications after several hours of debugging. My concern is that this is going to break *a lot* of codebases out there and frankly it is *very difficult* to know in advance which ones they will be. And it makes it harder to determine how secure a system is for the administrator but adds very little complexity for an attacker. The patch makes systems less comprehensibel, adds complex to troubleshoot bugs, and leaves systems with --- AT BEST --- a reason to have a false sense of security.
That's why I suggested a need for a system-wide fallback, which is what Debian wisely did. It enables sysadmins to test their systems, enable the additional security features if and when helpful.
Two quick points: 1. If you are going to change behavior of core modules, it needs to be clearly documented, along with information relating to expected new error conditions and how to fix them. This as not been done and AT A MINIMUM you could treat this as fixed by changing the documentation and loudly announcing likely breakage in advance of release. If you would reject even that modest requirement, then I don't know what to tell you. But at a minimum if you won't restore behavior, then the alternative would be to heavily document *all* expected breakage from this change including error messages and document solutions. 2. As I see it, my concern stands that sysadmins still are the only ones capable of resolving this and only locally currently, and so I will do my best to make sure that the IT professional community understand both the problem and how to prevent it as an attack vector without assuming the safety of any modules. I assume you would agree that this is not anything like a fix that system administrators can rely on for the safety of their systems and so I would hope you would understand that full disclosure is, really, the best that we can do. |
From chris.travers@gmail.comBTW, the other real concern this brings up for someone like myself who has How long before someone panics and breaks something again? I spent a fair bit of time looking into the issue. I wear many hats -- But I also concluded that in terms of exploitability, this approach didn't The second thing I determined was that this was trivially preventable on In other words, with a day's work or so (what I spent debugging this the I have to tell you, I have been primarily writing in Perl 5 since 2007 and You just need a way to choose for now to strip . from the default @INC. On Mon, Aug 1, 2016 at 7:21 PM, Zefram via RT <perlbug-followup@perl.org>
-- Efficito: Hosted Accounting and ERP. Robust and Flexible. No vendor |
From chris.travers@gmail.comOn Mon, Aug 1, 2016 at 7:16 PM, Zefram via RT <perlbug-followup@perl.org>
Good. That would waste people's time a *whole lot less* than the current 1. The bugs it introduces are very opaque and currently the behavior But by making a code contract only sometimes binding you create extremely The first thing to do when faced with a security problem is to make sure
So, remove it unless an environment variable is set or something, or make
Well it cost me a day of debugging the fix when it came out in Debian, so But it also is nowhere near a complete fix, which is why it is critical Major topics I have planned include prove, scripts that process data in Because right now system administrators can reasonably protect their
Then I hope *at least* you announce the breakage in a big way, properly If you would like to follow the full disclosure entries I will email you
-- Efficito: Hosted Accounting and ERP. Robust and Flexible. No vendor |
From zefram@fysh.orgYou have misunderstood the vulnerability. "prove foo.pl" is vulnerable The problem is specifically with the . that exists in @INC by default. In the absence of a global change to the initialisation of @INC, the The reason why we're also doing the much nastier @INC twiddling in modules I agree with you on the matter of documentation. We do need to make the -zefram |
From chris.travers@gmail.comZefram: No, you misunderstand me. The point there is that prove is a nice way to demonstrate the vulnerability in test scripts and that test frameworks must be insecure by design (because otherwise they would block problems found in production systems. Test frameworks are arbitrary code execution frameworks and it is important for administrators to secure them assuming that scripts run during the tests are vulnerable. That's my point. The focus of these posts is to look at full attack scenarios and what administrators can do to stop them. Surely you would agree that administrators running test systems should assume that tested code may do require inside an eval. Would you not agree? My next blog post will be on the question of the security of pl/perlU stored procedures and user defined functions running in PostgreSQL. |
From chris.travers@gmail.comThere are also, as it occurs to me, a number of things that could be done to make these sorts of changes (even if you insist on moving to nebulous software contracts) far more tolerable to developers. These include: 1. A real effort to make the failure cases transparent. For example, failing to require without . you could check the current directory for the file and warn if it exists. That would make debugging these sorts of problems easy and even more valuable than changes docs. 2. Clear documentation describing the sorts of errors that result from this change and their solutions. At least with the Debian backport (and the RC2 tarball) these things are not done. base.pm does not conform to documented behavior or to past behavior and therefore has a serious bug. You can address the bug in any of a number of ways (and documentation/transparency of error conditions is one) but it needs to be addressed. |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Tue Aug 02 21:53:07 2016, chris.travers@gmail.com wrote:
That was my complaint in ticket #15484
Which is exactly the change I have made to base.pm. Something like my patch will be included in 5.24.1 proper. -- Father Chrysostomos |
From chris.travers@gmail.comOn Wed, Aug 3, 2016 at 8:08 AM, Father Chrysostomos via RT <
Great :-)
Also good news!
-- Efficito: Hosted Accounting and ERP. Robust and Flexible. No vendor |
I don't see any further action needed here. Chris seems to be happy. I think this ticket can be closed. |
+1. Closing. Thank you very much. |
Migrated from rt.perl.org#128800 (status was 'open')
Searchable as RT128800$
The text was updated successfully, but these errors were encountered: