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

[SECURITY] Fix several cross-site scripting flaws #1

Merged
merged 1 commit into from
Jan 3, 2018

Conversation

tyll
Copy link

@tyll tyll commented Jul 27, 2017

Advisory: https://www.redteam-pentesting.de/advisories/rt-sa-2016-007

The advisory is not yet public, but it is scheduled to be released soon.

@opi99 opi99 self-requested a review July 27, 2017 13:24
@opi99 opi99 added the bug label Jul 27, 2017
@@ -1109,6 +1109,9 @@ protected function init()

$this->gp = $this->utilityFuncs->getMergedGP();

if (!$this->settings['uniqueFormID']) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, if 'uniqueFormID' is set, we have the same issue.
So removing this if statement?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move the preg_replace to line 1120 and sanitize $randomID unconditionally.

@@ -439,39 +439,39 @@ protected function fillDefaultMarkers()
}
$markers['###HIDDEN_FIELDS###'] = '
<input type="hidden" name="id" value="' . $GLOBALS['TSFE']->id . '" />
<input type="hidden" name="' . $name . '" value="1" />
<input type="hidden" name="' . htmlspecialchars($name) . '" value="1" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why hsc this? FormValuesPrefix is a formhandler setting and isn't influenced by post/get vars?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @reinhardfuehricht can explain this. In some other places $name isn't getting escaped currently.

@@ -1109,6 +1109,9 @@ protected function init()

$this->gp = $this->utilityFuncs->getMergedGP();

if (!$this->settings['uniqueFormID']) {
$this->gp['randomID'] = preg_replace('/[^0-9a-z]/', '', preg_quote($this->gp['randomID']));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preg_quote seems superfluous.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is deprecated, obsolete and unmaintained but would a backport fix of v.2.4.1 to the Formhandler v2.0.2 (last one available for T3 6.2ELTS) be to "preg_replace / quote" and "htmlspecialchars" the same lines in the corresponding files in the older version?

formhandler_2.0.2\Classes\Controller\Tx_Formhandler_Controller_Form.php
and
formhandler_2.0.2\Classes\View\Tx_Formhandler_View_Form.php

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formhandler 2.0.2 released to TER should have them inside, or take a look by AoE https://github.com/AOEpeople/formhandler

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's 2.0.2 but the old version with no fixes (like htmlspecialchars($name)) inside.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@gizmo21 gizmo21 Nov 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks opi99 for that one. That's at least exactly what I have done:
https://github.com/KaffDaddy/formhandler/commit/366e728b81d2a7c9c83bb29e8522a8eab736df33

So it seems that is best one can do on a 6.2ELTS release.

Also thanks tyll for discovering it 1year ago.

@SvenJuergens
Copy link

@tyll @Phaeilo is this ready to merge?

@tyll
Copy link
Author

tyll commented Nov 9, 2017

FYI: Unfortunately I do not have the time to take deeper looks into this issue.

@opi99
Copy link
Collaborator

opi99 commented Jan 3, 2018

It's not complete but merging

@opi99 opi99 closed this Jan 3, 2018
@opi99 opi99 reopened this Jan 3, 2018
@opi99 opi99 merged commit 0b56d7e into pluspol-interactive:master Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants