[7.4-only] LPS-122191 Removes unnecessary FlashMagicBytes checks #9540
Conversation
The FlashMagicBytesUtil checks were added to prevent [CSRF attack using uploaded flash files](https://issues.liferay.com/browse/LPS-52121). We will no longer support Adobe Flash in DXP 7.4. Adobe will stop providing security fixes for the platform in December 2020 and browsers will prevent any content from running past that date. `FlashMagicBytesUtil` uses a `PushbackInputStream` to read the first bytes of an `InputStream` and then simply rewinds it and provides means to access it. That means that when removing the usage, we can simply use the original `InputStream` to keep the same behaviour. To avoid a major kernel breaking change, we're simply deprecating both `FlashMagicBytesUtil` and `FlashMagicBytesUtilTest` instead of deleting them.
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
ci:test:relevant |
|
ci:test:sf |
❌ ci:test:sf - 0 out of 1 jobs passed in 39 secondsClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-122191 1 Failed Jobs:For more details click here.
|
liferay-continuous-integration
added
ci:test:relevant - failure
and removed
ci:test:relevant - pending
labels
|
ci:test:sf |
❌ ci:test:relevant - 0 out of 1 jobs passed in 1 minuteClick here for more details.Base Branch:Branch Name: master ci:test:relevant - 0 out of 1 jobs PASSED1 Failed Jobs:For more details click here.Failures unique to this pull:
For upstream results, click here. |
❌ ci:test:sf - 0 out of 1 jobs passed in 0 msClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-122191 1 Failed Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#3537 |
|
Jenkins Build:test-portal-source-format#3538 |
|
ci:test:sf |
|
ci:test:relevant |
liferay-continuous-integration
added
ci:test:relevant - pending
ci:test:stable - pending
and removed
ci:test:stable - pending
labels
|
Hey @shuyangzhou . Sorry for the mess with the CI... :-( I'm sending you this PR so that you can review the last two commits, which affect the backend infrastructure. You can look at the preceding commits at your will, but they are mostly frontend infra stuff. Given that all the rest has been accepted by the security team, once those two commits are clear, this can be forwarded to Brian for merge. CCing @jbalsas and @topolik in case they want to add something before this is forwarded to master. |
Note that this fixes the problem at liferay-appsec#143 (comment). The diagnostic of the problem is explained in the comments of the same PR of the error, starting at liferay-appsec#143 (comment). This commit introduces new methods to distinguish between parameter types and functionality. The idea is to stop using dynamic type checks (instanceof) to do one thing or the other and make it explicit in the code to avoid future errors. Regarding the two functionalities I mention, there are two ways to consume the InputStream: one is to skip bytes, the other to directly seek the position from where to start reading. That's why I created two different method with two different names.
|
ci:test:sf |
liferay-continuous-integration
added
ci:test:sf - pending
and removed
ci:test:sf - success
labels
|
ci:test:relevant |
✔️ ci:test:sf - 1 out of 1 jobs passed in 37 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-122191 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#4251 |
|
See #9544 |
This is the continuation of liferay-appsec#143
Based on the analysis done at Analyze security relevance of FlashMagicBytes check):
From @topolik:
All browsers in our possible compatibility matrix for 7.4 will have dropped Flash by the time we release as per their roadmaps:
Based on this, I think it's safe to remove the
FlashMagicByteschecks.The solution:
FlashMagicBytesUtiluses aPushbackInputStreamto read the first bytes of anInputStreamand then simply rewinds it and provides means to access it. That means that when removing the usage, we can simply use the originalInputStreamto keep the same behaviour.FlashMagicBytesUtilandFlashMagicBytesUtilTestinstead of deleting them.