LFI and RCE protection guidelines #2580

kalpmehta · 2018-07-22T19:48:45Z

LFI and RCE protection guidelines. Also formatted Security Guidelines.

This PR is a:

New topic
Content update
Content fix or rewrite
Bug fix or improvement

Summary

Includes LFI and RCE protection guidelines.

whatsnew
Added local file inclusion (LFI) and remote code execution (RCE) protection guidelines to the Code Magento Standards.

LFI and RCE protection guidelines

magento-cicd2 · 2018-07-22T19:48:47Z

All committers have signed the CLA.

jeff-matthews · 2018-07-23T12:55:08Z

@buskamuza, please review the LFI and RCE guidelines that @kalpmehta added.

jeff-matthews · 2018-07-23T12:57:59Z

guides/v2.2/coding-standards/technical-guidelines.md

@@ -707,7 +707,7 @@ class SampleEventObserverThatModifiesInputs

 15.1. Use prepared statements for SQL queries.

-15.2. Broken Authentication protection.
+###15.2. Broken Authentication protection.


@kalpmehta, you need to remove all of the hashtags that you added. They're rendering as plaintext in the HTML.

kalpmehta · 2018-07-23T15:43:40Z

@jeff-matthews that's weird, removed all the hashtags as requested.

buskamuza · 2018-07-23T20:14:15Z

guides/v2.2/coding-standards/technical-guidelines.md

@@ -761,6 +761,18 @@ class SampleEventObserverThatModifiesInputs

 15.8. Frequently update the third-party libraries used in the project/component to eliminate known vulnerabilities.

+15.9. LFI protection.


Please include abbreviation explanation, as in "13. Command line interface (CLI)".

buskamuza · 2018-07-23T20:20:21Z

guides/v2.2/coding-standards/technical-guidelines.md

+
+15.10. RCE protection.
+
+15.10.1. SHOULD avoid usage of eval(), passthru(), system(), shell_exec()


We also do not recommend usage of unserialize, srand, mt_srand, md5 (see https://github.com/magento/magento2/blob/2.2-develop/dev/tests/static/testsuite/Magento/Test/Legacy/_files/security/unsecure_php_functions.php)

We should rephrase 15.10.1 for clarity.

SHOULD NOT use eval(), passthru(), system, shell_exec().

thanks @buskamuza for that.
@szurek sure, totally makes sense.

buskamuza · 2018-07-23T20:21:33Z

@szurek, @melnikovi , @paliarush , @akaplya , @DrewML , please review.

melnikovi · 2018-07-23T20:43:31Z

guides/v2.2/coding-standards/technical-guidelines.md

+15.10.1. SHOULD avoid usage of eval(), passthru(), system(), shell_exec()
+
+15.10.2. SHOULD NOT directly pass user-submitted values to include*(), require*(), create_function(), fopen(), preg_replace()
+
 <!-- LINKS: DEFINITIONS AND ADDRESSES -->


Can we add that we recommend using readonly filesystem? Ability to modify files can lead to many different attacks. Maybe in a new section, since it's more general?

Readonly filesystem is not something implemented on the application level, so there is nothing (except validation) developers can do to follow the requirement.

szurek · 2018-07-23T20:48:40Z

guides/v2.2/coding-standards/technical-guidelines.md

+15.10.1. SHOULD avoid usage of eval(), passthru(), system(), shell_exec()
+
+15.10.2. SHOULD NOT directly pass user-submitted values to include*(), require*(), create_function(), fopen(), preg_replace()
+


I'd recommend adding:

15.10.3. SHOULD NOT use user-submitted data for variable functions.

Maybe a better way to phrase this? I'm referring to $userParam($userData);

@szurek nice addition. Just added.

melnikovi · 2018-07-23T21:30:16Z

guides/v2.2/coding-standards/technical-guidelines.md

+15.10.1. SHOULD NOT use eval(), passthru(), system(), shell_exec(), serialize(), unserialize(), md5(), srand(), mt_srand()
+
+15.10.2. SHOULD NOT directly pass user-submitted values to include*(), require*(), create_function(), fopen(), preg_replace()
+
 <!-- LINKS: DEFINITIONS AND ADDRESSES -->


@kalpmehta what do you think about adding that we recommend using readonly filesystem? Ability to modify files can lead to many different attacks. Maybe in a new section, since it's more general?

Agree it's best as per security to only allow read-only file access. I was wondering what if some extension or custom development needs file write operations as well. There should not be any issues as long as user-submitted values are not passed to these functions.

If extension needs to write/modify files, these files should be located in directories application not trying to include/execute files from (pub/media, var). It's very tricky as sometimes attacks because possible as combination of multiple vulnerabilities.

paliarush · 2018-07-24T16:53:17Z

guides/v2.2/coding-standards/technical-guidelines.md

+
+15.10. Remote Code Execution (RCE) protection.
+
+15.10.1. SHOULD NOT use eval(), passthru(), system(), shell_exec(), serialize(), unserialize(), md5(), srand(), mt_srand()


We already have most of the functions from 15.10.1 prohibited by our static tests. @melnikovi, in scope of CR automation initiative, could you please double check if any of these are not covered yet.

@kalpmehta If we make sure that all these functions are covered by static tests, does it make sense to have this section?

@paliarush yes, will do.

@paliarush I think it makes sense to list all the unsecure functions so that any engineer who comes to refer these guidelines becomes aware of not using those functions at all. This would also be useful to engineers/architects who does manual code reviews or writes code without running tests. However, I am open to remove that section if you think so!

Yes, the section itself makes sense. Though in the future we'd like to cover as many as possible with automated tests.

buskamuza · 2018-07-25T16:53:35Z

✅ review passed from architecture side. Great addition to the guidelines!

dshevtsov · 2018-07-30T15:02:53Z

@jeff-matthews please add whatsnew

12 12 integration

LFI and RCE protection guidelines

e1fcd85

LFI and RCE protection guidelines

jeff-matthews requested a review from buskamuza July 23, 2018 12:54

jeff-matthews self-assigned this Jul 23, 2018

jeff-matthews suggested changes Jul 23, 2018

View reviewed changes

Removed hashtags

c047c77

buskamuza suggested changes Jul 23, 2018

View reviewed changes

jeff-matthews requested review from szurek, akaplya, DrewML, melnikovi and paliarush July 23, 2018 20:26

jeff-matthews approved these changes Jul 23, 2018

View reviewed changes

melnikovi reviewed Jul 23, 2018

View reviewed changes

szurek reviewed Jul 23, 2018

View reviewed changes

kalpmehta added 2 commits July 23, 2018 13:54

Updated insecure functions and full forms

87de230

Update technical-guidelines.md

2d43f1f

melnikovi reviewed Jul 23, 2018

View reviewed changes

DrewML approved these changes Jul 24, 2018

View reviewed changes

paliarush approved these changes Jul 24, 2018

View reviewed changes

melnikovi approved these changes Jul 24, 2018

View reviewed changes

Update technical-guidelines.md

ce95a93

buskamuza approved these changes Jul 25, 2018

View reviewed changes

jeff-matthews added Major Update Significant original updates to existing content 2.2.x 2.3.x Magento 2.3 related changes labels Jul 25, 2018

jeff-matthews added the Technical Updates to the code or processes that alter the technical content of the doc label Jul 25, 2018

jeff-matthews merged commit 20ce863 into magento:master Jul 25, 2018

kalpmehta deleted the patch-3 branch July 25, 2018 17:23

magento-devops-reposync-svc pushed a commit that referenced this pull request Oct 12, 2021

Merge pull request #2580 from magento-commerce/12-12_integration

9039c1e

12 12 integration

		@@ -761,6 +761,18 @@ class SampleEventObserverThatModifiesInputs

		15.8. Frequently update the third-party libraries used in the project/component to eliminate known vulnerabilities.

		15.9. LFI protection.


		15.10. RCE protection.

		15.10.1. SHOULD avoid usage of eval(), passthru(), system(), shell_exec()

		15.10.1. SHOULD avoid usage of eval(), passthru(), system(), shell_exec()

		15.10.2. SHOULD NOT directly pass user-submitted values to include(), require(), create_function(), fopen(), preg_replace()


		15.10. Remote Code Execution (RCE) protection.

		15.10.1. SHOULD NOT use eval(), passthru(), system(), shell_exec(), serialize(), unserialize(), md5(), srand(), mt_srand()

LFI and RCE protection guidelines #2580

LFI and RCE protection guidelines #2580

Uh oh!

Conversation

kalpmehta commented Jul 22, 2018 • edited by dshevtsov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR is a:

Summary

Uh oh!

magento-cicd2 commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-matthews commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalpmehta commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szurek Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buskamuza commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buskamuza commented Jul 25, 2018

Uh oh!

dshevtsov commented Jul 30, 2018

Uh oh!

Uh oh!

kalpmehta commented Jul 22, 2018 •

edited by dshevtsov

Loading

magento-cicd2 commented Jul 22, 2018 •

edited

Loading

szurek Jul 23, 2018 •

edited

Loading

buskamuza commented Jul 23, 2018 •

edited

Loading