View Issue Details

IDProjectCategoryView StatusLast Update
0000239Adventure PHP FrameworkSicherheit // Securitypublic2015-10-12 12:19
ReporterChristianAchatzAssigned ToChristianAchatz 
PrioritynormalSeverityfeatureReproducibilityN/A
Status closedResolutionfixed 
Product Version2.1 
Target Version3.0Fixed in Version3.0 
Summary0000239: Improve form security
DescriptionForm handling security should be improved by the following actions:

* Enable XSS input protection filter by default (improved version of http://wiki.adventure-php-framework.org/XSS_Schutz_via_InputFilter)
* "action" attribute of the <form /> tag should be escaped in every case (not just if automatically preset).
Additional Information<?php
namespace APF\core\filter;

use APF\core\filter\ChainedContentFilter;
use APF\core\filter\FilterChain;

class XssProtectionInputFilter implements ChainedContentFilter
{

    /**
     * @param FilterChain $chain
     * @param string $input
     *
     * @return string
     */
    public function filter(FilterChain &$chain, $input = null)
    {
        $_POST = $this->sanitize($_POST);
        $_GET = $this->sanitize($_GET);
        $_REQUEST = $this->sanitize($_REQUEST);
        $_SERVER['REQUEST_URI'] = $this->cleanValue(urldecode($_SERVER['REQUEST_URI']));

        return $chain->filter($input);
    }

    /**
     * Recursively sanitizes data
     *
     * @param array $data
     *
     * @return array
     */
    protected function sanitize(array $data)
    {
        foreach ($data as $key => $value) {
            $data[$key] = is_array($value) ? $this->sanitize($value) : $this->cleanValue($value);
        }

        return $data;
    }

    /**
     * @param string $input
     *
     * @return string
     */
    protected function cleanValue($input)
    {
        return strip_tags(html_entity_decode($input, ENT_QUOTES, $this->charset));
    }
}
TagsNo tags attached.
Codereferenz: ([Datei]:[Zeile])
Namespacecore

Activities

dingsda

2014-10-06 20:18

developer   ~0000510

gab es das nicht schonmal und wurde dann wieder entfernt?
http://forum.adventure-php-framework.org/viewtopic.php?f=8&t=602&p=5632#p5632

warum wieder einführen?

ChristianAchatz

2014-10-06 22:50

administrator   ~0000511

Guter Einwand! Warum ich dieses Item eröffnet habe war die Diskussion mit einem Kollegen über die Ergebnisse eines Security-Tests. Dieser fiel sehr gut aus, was hauptsächlich auf die Verwendung des oben geposteten Codes zurückzuführen ist.

Aus diesem Grund halte ich es für absolut sinnvoll, diesen mit dem APF auszuliefern und standardmäßig zu aktivieren. Wie siehst du das?

ChristianAchatz

2014-10-12 22:25

administrator   ~0000518

Escapen des "action"-Attributs findet nun immer statt. Einführen des XSS-Filters noch ausstehend.

dingsda

2014-10-15 13:01

developer   ~0000520

wie in dem forenbeitrag von screeze schon angemerkt sollte das escaping immer kontextabhängig geschehen. ein input-filter tut das nicht.

ein input filter kann halt unmöglich wissen, was schädlich ist und was nicht.
so wie der gorm dafür sorgt, das sql-injection nicht möglich ist so sollte auch template-engine sollte dafür sorgen das der output entsprechen escaped wird, damit xss nicht möglich ist.

probleme mit dem vorgeschlagenen input-filter hier speziell:
strip_tags ist zu vorsichtig. es entfernt alles nach einem "<". möchte ein user ein passwort wie jk<ew732h!kjkl verwenden würde an der verarbeitenden stelle nur jk ankommen.
auch wenn jemand 1<4 schreibt würde das schon abgeschnitten werden.
das posten von html-code wie es z.b. hier oder in jedem forum möglich ist würde auch nicht gehen.

für das input-filtern wird vom framework ja mit den form-filtern und validatoren schon einiges angeboten, was dem entwickler hilft user-input zu filtern.

auch die methode getAttributesAsString bietet xss-sicherheit, das die attributwerte durch htmlspecialchars gejagt werden.

weitere sicherheit würde noch http://tracker.adventure-php-framework.org/view.php?id=147 bringen.
hier würde direkt kontext-abhängig escaped werden.
da fällt mir auch ein, dass vielleicht das ExpressionEvaluationTag auch entsprechen überarbeitet werden sollte, dass es in der transform-methode den string noch durch htmlspecialchars jagt.

zu deinem zweiten punkt bezüglich des action-attributs habe ich mir den commit auch nochmal angeschaut. wenn ich das richtig sehe ist das escaping des action-attributes beim setzen unnötig, da bei der methode getAttributesAsString nochmal htmlspecialchars verwendet wird.

$action = $this->getAttribute(self::ACTION_ATTRIBUTE_NAME);
if ($action === null) {
$this->setAttribute(self::ACTION_ATTRIBUTE_NAME, self::getRequest()->getRequestUri());
}

 // transform the form including all child tags
$htmlCode = (string) '<form ';
$htmlCode .= $this->getAttributesAsString($this->attributes, $this->attributeWhiteList);

reicht also auch schon vollkommen aus.

ChristianAchatz

2014-10-15 16:40

administrator   ~0000521

Hallo dingsda,

danke für dein Feedback. Ich habe in der Zwischenzeit nochmals weiter drüber nachgedacht und halte den Input-Filter für gangbar aber tatsächlich nur optional - sprich jedes Projekt muss für sich entscheiden, ob es den Filter einsetzen will/kann.

Bei einem Security-Test wurde kürzlich berichtet, dass Formulare anfällig für XSS sind, weil URL-Parameter auf diese Weise eingeschleust werden können. Beispiel:

http://example.com?"><script>alert('XSS')</script>=1

Das wurde (scheinbar) davon nicht erfasst. Ich war zuerst auch etwas stutzig, scheinbar gab es aber Konstellationen, in denen das nicht escaped wurde. Aus diesem Grund habe ich das - offensichtlich doppelte - escapen hinzugefügt. Ich schaue mir das nochmal an, danke für den Hinweis!

Vorschlag: wir nehmen den XssFilter in das APF auf, lassen ihn aber per default deaktiviert und kümmern uns nochmal um 0000147. Einverstanden?

ChristianAchatz

2015-01-28 20:34

administrator   ~0000540

Filter in Release aufgenommen. Verwundbarkeit sollte nicht gegeben sein.

Issue History

Date Modified Username Field Change
2014-10-06 17:12 ChristianAchatz New Issue
2014-10-06 17:12 ChristianAchatz Status new => assigned
2014-10-06 17:12 ChristianAchatz Assigned To => ChristianAchatz
2014-10-06 20:18 dingsda Note Added: 0000510
2014-10-06 22:50 ChristianAchatz Note Added: 0000511
2014-10-12 22:25 ChristianAchatz Note Added: 0000518
2014-10-15 13:01 dingsda Note Added: 0000520
2014-10-15 16:40 ChristianAchatz Note Added: 0000521
2015-01-28 20:34 ChristianAchatz Note Added: 0000540
2015-01-28 20:34 ChristianAchatz Status assigned => resolved
2015-01-28 20:34 ChristianAchatz Fixed in Version => 3.0
2015-01-28 20:34 ChristianAchatz Resolution open => fixed
2015-10-12 12:19 ChristianAchatz Status resolved => closed