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

March 20, 2019 #126

Closed
buskamuza opened this issue Mar 19, 2019 · 9 comments
Closed

March 20, 2019 #126

buskamuza opened this issue Mar 19, 2019 · 9 comments
Labels
meeting notes Topic requests and notes from meetings

Comments

@buskamuza
Copy link
Contributor

buskamuza commented Mar 19, 2019

Please add your topic as a comment to the issue. Use following format:
Topic description and link to PR, if any (duration in min)

Meeting Notes

See below.

🎥 Recording

@buskamuza buskamuza added the meeting notes Topic requests and notes from meetings label Mar 19, 2019
@buskamuza buskamuza changed the title March 19, 2019 March 20, 2019 Mar 19, 2019
@lenaorobei
Copy link
Contributor

Is there any cases when object instantiation via new keyword is allowed in Magento? magento/magento-coding-standard#64 (5 min)

@kandy
Copy link
Contributor

kandy commented Mar 20, 2019

Issue: \Magento\Framework\Serialize\Serializer\Json is not binary safe
see example

<?php
$examples = array(
    'Valid ASCII' => "a",
    'Valid 2 Octet Sequence' => "\xc3\xb1",
    'Invalid 2 Octet Sequence' => "\xc3\x28",
    'Invalid Sequence Identifier' => "\xa0\xa1",
    'Valid 3 Octet Sequence' => "\xe2\x82\xa1",
    'Invalid 3 Octet Sequence (in 2nd Octet)' => "\xe2\x28\xa1",
    'Invalid 3 Octet Sequence (in 3rd Octet)' => "\xe2\x82\x28",
    'Valid 4 Octet Sequence' => "\xf0\x90\x8c\xbc",
    'Invalid 4 Octet Sequence (in 2nd Octet)' => "\xf0\x28\x8c\xbc",
    'Invalid 4 Octet Sequence (in 3rd Octet)' => "\xf0\x90\x28\xbc",
    'Invalid 4 Octet Sequence (in 4th Octet)' => "\xf0\x28\x8c\x28",
    'Valid 5 Octet Sequence (but not Unicode!)' => "\xf8\xa1\xa1\xa1\xa1",
    'Valid 6 Octet Sequence (but not Unicode!)' => "\xfc\xa1\xa1\xa1\xa1\xa1",
);

foreach ($examples as $str) {

    if (json_encode($str) === false) {
        echo "Cannot json encode str :[$str] - Error: " . json_last_error_msg() ."\n";
    }
}

will output:

annot json encode str :[�(] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[��] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[�(�] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[�(] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[�(��] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[�(�] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[�(�(] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[�����] - Error: Malformed UTF-8 characters, possibly incorrectly encoded
Cannot json encode str :[������] - Error: Malformed UTF-8 characters, possibly incorrectly encoded

@buskamuza
Copy link
Contributor Author

buskamuza commented Mar 20, 2019

@kandy , for the encoding:

  • create an item in the backlog and prioritize with PO
  • possible options:
    • use serialization/unserialization w/ no objects allowed
    • use base64. In this case we need to traverse through arrays, it's more complicated and slow
    • discuss with security team
    • discuss with @piotrekkaminski

@buskamuza
Copy link
Contributor Author

buskamuza commented Mar 20, 2019

\Magento\Framework\Encryption\Encryptor::encrypt() uses base64 which increases data being stored, for example, in cache.
@kandy:

  • remove base64 from encrypt
  • make encoding binary safe (see above discussion). E.g., use serialization instead for caching scenario

@buskamuza :

  • it's a breaking change
  • first, let's resolve the encoding issue

@piotrekkaminski
Copy link

@buskamuza @kandy strongly disagree on adding serialization. We had tons of issues in the past and even right now with unserialize leading to RCE. Why is base64 a problem? Is it a measurable performance impact? @szurek ??

@kandy
Copy link
Contributor

kandy commented Mar 20, 2019

@piotrekkaminski see related ticket magento/magento2#21334

@piotrekkaminski
Copy link

Also - what are the use cases for binary data (as unicode seems to work ok)?

@piotrekkaminski
Copy link

@kandy they mention encryption in the ticket as the culprit. How it relates to serialization/json encoding?

@kandy
Copy link
Contributor

kandy commented Mar 20, 2019

@piotrekkaminski The encryption produces a binary data that cannot be serialized, as a result, we add base64 encoding on top of encryption that increase traffic between servers and cause performance degradation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting notes Topic requests and notes from meetings
Projects
None yet
Development

No branches or pull requests

4 participants