Skip to content
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

Fix RT 84403 - 'Security problem: missing "start" mode dumps ENV to output page' #15

Merged
merged 10 commits into from Jan 13, 2015

Conversation

MartinMcGrath
Copy link
Collaborator

This pull request resolves the issue raised in https://rt.cpan.org/Public/Bug/Display.html?id=84403

Application.pm

A new runmode named no_runmodes is now called rather than dump_html when no run modes are specified. This returns a message to the user reporting the problem, without exposing anything which may raise security concerns. The POD has been updated, asking the user to think about potential security issues when calling dump_html.

basic.t/TestApp.pm

Add tests for the new runmode

load_tmpl_hook.t

Test required an update as it was dependant on Application.pm returning the output of dump_html

In addition to the changes above, some very minor changes to the indentation.

If there are any issues please let me know.

Update to Application.pm to address rt bug 84403:

https://rt.cpan.org/Public/Bug/Display.html?id=84403

Create a new runmode (named no_runmodes) to display an error when no runmodes exist, as previously calling dump_html() may present a security issue.

Alter call in run_modes to call no_runmodes rather than dump_html
Add a new test for no_runmodes, which replaces dump_html when no run modes are provided.

Add a test using TestApp.pm to cater for the dump_html test.
Add a runmode to named dump_htm to call dump_html. This is to cover a test added to basic.t
Fix typo in runmode called during dump_html test
This test required an update as it was dependant on the Application.pm returning the output of dump_html() if no runmodes existed.
add Module::Build to requires to stop the message: "Module::Build was not found in configure_requires! Adding it now"
Alignment
Add a warning to the POD for dump_html()
@fionnb
Copy link

fionnb commented Feb 19, 2014

I would like to STRONGLY endorse the application of this patch.
I just was about to open an report for exactly this issue when I found it already addressed by Martin. The problem has been introduced with commit 61d3276 already but probably did not cause major hassle until it arrived in recent debian repos lately. The security implications of an unexpected and potentially uncontrollable var dump to the world are very serious. We also have lost quite some time trying to find out where this unexpected dump came from in the first place and how it was caused. As an added "bonus", the output of dump_html is not even a valid html page.

@MartinMcGrath
Copy link
Collaborator Author

@fionnb I've created a pull request to ensure dump_html returns a valid html page.

@fionnb
Copy link

fionnb commented Feb 19, 2014

On 19.02.2014 15:35, Martin McGrath wrote:

@fionnb https://github.com/fionnb I've created a pull request to
ensure dump_html returns a valid html page.

Thanks, but that does not address the implicit, serious security issue.

See also:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=739505

kind regards!

@jerlbaum
Copy link
Collaborator

Hi Guys --

On Wed, Feb 19, 2014 at 10:27 AM, fionnb notifications@github.com wrote:

Thanks, but that does not address the implicit, serious security issue.

See also:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=739505

This bug report is wrong. There was no change in 2008 which "caused" this
"problem."

CGI::Application has always called dump_html(), since it was first
written. It's been that way for more than a decade, and in that time there
has never been any report of any break-in or security problem as a result
of this.

The purpose of calling dump_html() is to demonstrate that the application
module is working. If a developer installs a bit of code on their web
server, and then fails to actually configure it in any way, of course they
are going to create problems for themselves. Imagine the dev who installs
something like "PhpMyAdmin" and never changes the default password. The
security problem is the developer who is failing to do their job in a
professional manner... and there's no patch for that.

Jesse

Jesse Erlbaum
The Erlbaum Group, LLC
817 Broadway, 4th floor
New York, NY 10003
212-684-6161 (office)
917-647-3059 (mobile)
646-688-4029 (fax)
jesse@erlbaum.net

@MartinMcGrath
Copy link
Collaborator Author

fionnb

This second pull request was to address the "bonus" issue you raised, the first which you endorse, apparently because you experienced the problem, would seem to match your requirements. If this is not the case perhaps a short test case replicating your problem would help.

Jesse,

My patch does not remove dump_html, rather it's no longer called by default when no runmodes are specified. You can still call dump_html. As you say the developer should be aware of what they're doing by deliberately dumping $ENV to a page.

In my patch the default runmode when none are provided reports this back to the user, rather than the output of dump_html(). This could be used "to demonstrate that the application module is working", without exposing anything unnecessarily.

Thanks

@fionnb
Copy link

fionnb commented Feb 19, 2014

On 19.02.2014 16:36, jerlbaum wrote:

This bug report is wrong. There was no change in 2008 which "caused" this
"problem."

Hi Jesse!

Oh yes, there is. I even linked the commmit id four your convenience,
did you see?
The initialisation of run_mode to call dump_html by default moved from
setup() to be hard coded in run_mode().
So, until 2008, if you overloaded setup() - which everyone does - you
had NO start->dump_html default at all in run_mode!

After the change, you ALWAYS have dump_html as a default run_mode unless
you explicitly redefine it in your code. Before the change this was not
the case.
A company I work at sells and runs a major enterprise application system
written in perl and this change caused pages with sensitive data to
appear in case of an error because some submodules did not have start
defined in run_mode. This problem occurs ONLY with the new version and
not with the old one.

Facts: Behaviour of an otherwise unmodified application changed due to
an update of Application.pm. Data that should not be shown AND was not
shown when using versions before v4.19 was now on screen. Customers were
not amused. Considerable amounts of manpower where spent trying to trace
the origin of the unexpected "leak" through several application layers.
So, please don't tell me that nothing happened.

CGI::Application has always called dump_html(), since it was first
written. It's been that way for more than a decade, and in that time there
has never been any report of any break-in or security problem as a
result
of this.

I'd dare to say that this default should never have been defined that
way at all. Not the version before 08, and even more so the one after.
Direction of potentially sensitive data into a publicly accessible
entity should ALWAYS be the result of a willful act of the developer and
NEVER be triggered by any automatism whatsoever. As an added "bonus" in
this case, the documentation (README, Changelog) does neither mention
the potentially dangerous "default" nor the behavioral change in the API
in any way.

The chances to be hit by this definitely have increased with the above
mentioned code change for the reasons explained.

Thank you for reading. I hope to have clarified any doubts now about why
this IS indeed an important fix and it should be applied without hesitation.

@jerlbaum
Copy link
Collaborator

On Wed, Feb 19, 2014 at 3:23 PM, fionnb notifications@github.com wrote:

Oh yes, there is. I even linked the commmit id four your convenience,
did you see?
The initialisation of run_mode to call dump_html by default moved from
setup() to be hard coded in run_mode().
So, until 2008, if you overloaded setup() - which everyone does - you
had NO start->dump_html default at all in run_mode!

Can you tell me which version this changed with?

Jesse Erlbaum
The Erlbaum Group, LLC
817 Broadway, 4th floor
New York, NY 10003
212-684-6161 (office)
917-647-3059 (mobile)
646-688-4029 (fax)
jesse@erlbaum.net

@markstos
Copy link
Owner

Can you tell me which version this changed with?

Version 4.19 is mentioned in the issue history as when the change happened.

@jerlbaum
Copy link
Collaborator

Yes, thanks! Found it.

On Feb 19, 2014, at 5:11 PM, Mark Stosberg notifications@github.com wrote:

Can you tell me which version this changed with?

Version 4.19 is mentioned in the issue history as when the change happened.

Reply to this email directly or view it on
GitHubhttps://github.com//pull/15#issuecomment-35556021
.

@eseyman
Copy link

eseyman commented Feb 23, 2014

FYI, I've released updates for Fedora and EPEL (the Fedora branch for RHEL and clones).

Edit to commit, ensure output of no_runmodes outputs valud HTML, as per:

markstos#16
bket pushed a commit to bitrig/bitrig-ports that referenced this pull request Aug 1, 2014
Fix RT 84403 - 'Security problem: missing "start" mode dumps ENV to output
page'
markstos/CGI--Application#15

While here remove groff and fix runtime depends.
www/p5-CGI-PSGI is optional, include it as people nowadays run PSGI and are
moving away from MOD_PERL.

From maintainer Ian McWilliam

Written by: Christian Weisgerber <naddy@openbsd.org>
bket pushed a commit to bitrig/bitrig-ports that referenced this pull request Aug 1, 2014
Fix RT 84403 - 'Security problem: missing "start" mode dumps ENV to output
page'
markstos/CGI--Application#15

While here remove groff and fix runtime depends.
www/p5-CGI-PSGI is optional, include it as people nowadays run PSGI and are
moving away from MOD_PERL.

From maintainer Ian McWilliam

Written by: Christian Weisgerber <naddy@openbsd.org>
@MartinMcGrath
Copy link
Collaborator Author

With CPAN day coming up in 10 days time I was wondering if it we could merge this (and #16) and have a new CPAN release, since the current release is from 2011.

Alternatively if there's anything I can do to help please let me know.

Thanks

MartinMcGrath added a commit that referenced this pull request Jan 13, 2015
Fix RT 84403 - 'Security problem: missing "start" mode dumps ENV to output page'

Merging to address https://rt.cpan.org/Public/Bug/Display.html?id=84403
@MartinMcGrath MartinMcGrath merged commit 56046f3 into markstos:master Jan 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants