Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

A simple invalid 2 octet sequence can cause an exception (which then … #29

Closed
wants to merge 1 commit into from

Conversation

Saeven
Copy link

@Saeven Saeven commented Feb 1, 2018

A change is necessary to prevent routine penetration tests, or user-land attacks from causing critical exceptions. Users shouldn't be able to craft malicious input that causes 500s when using components such as Zend Form.

Did my best to eyeball formatting conventions, phpcs is in a panic with the given config.

Debug added as second arg to ctor; but could separately be moved into (or complemented by) a setDebug method.

…causes your server to 500), the behavior was detected when working with Zend Form.

A change is necessary to prevent routine penetration tests, or user-land attacks from causing critical exceptions. Users shouldn't be able to craft malicious input that causes 500s.
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Closing as inapplicable

@@ -6,7 +6,8 @@ All notable changes to this project will be documented in this file, in reverse

### Added

- Nothing.
- Default behavior changed when malformed is pushed into toUtf8. Rather than dumping the bad string in an exception (dangerous behavior), it will return a blank string.
Copy link
Member

Choose a reason for hiding this comment

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

This is not dangerous behavior: it is documented and expected behavior.

Passing around an empty string when an invalid value to escape is passed in can cause much more serious issues.

@@ -6,7 +6,8 @@ All notable changes to this project will be documented in this file, in reverse

### Added

- Nothing.
- Default behavior changed when malformed is pushed into toUtf8. Rather than dumping the bad string in an exception (dangerous behavior), it will return a blank string.
- Boolean 'debug' parameter added as second constructor argument, that preserves the old behavior when set to true.
Copy link
Member

Choose a reason for hiding this comment

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

No, please remove this. If you want to run an escaper in debug mode, subclass the Escaper and implement something like:

class DebugEscaper extends \Zend\Escaper\Escaper
{
    public function escapeHtml($toEscape) { return 'ESCAPED-HTML-START' . $toEscape . 'ESCAPED-HTML-END'; }
}

@weierophinney
Copy link
Member

@Ocramius For context, please see the related issue in zend-form:

I asked @Saeven to open the issue here, because it is applicable, and there's a generalized issue in escapeHtmlAttr() when malformed UTF-8 is present for the value.

The fix is needed; the question is whether the approach here is correct, or needs revisions.

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2018

@weierophinney we will bring the issue offline first and discuss it. The original issue is clear, the fix is not valid though, and the default behavior is correct, while this particular patch would only provide a new attack surface.

Invalid values do need to crash systems by default, as otherwise:

  • bugs go unnoticed
  • logic execution continues even with invalid output (think legal documents/PDFs with unexpected blank fields, for example, and the chaos that causes later on)

@weierophinney
Copy link
Member

@Ocramius Just now catching up on the Slack discussion and zend-form discussion; I reacted hastily.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants