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

Improve the XML transforming code - add protection against XXE. #17722

Merged
merged 1 commit into from Oct 15, 2020

Conversation

kwart
Copy link
Member

@kwart kwart commented Oct 14, 2020

This PR covers XML XXE issues identified by the Sonar.

The main part of the fix are these lines:

            transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
            transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");

XML formatting code was moved to StringUtil class and the tests were added.

EE: https://github.com/hazelcast/hazelcast-enterprise/pull/3831

@kwart kwart added this to the 4.1 milestone Oct 14, 2020
@kwart kwart requested a review from a team as a code owner October 14, 2020 12:04
@kwart kwart self-assigned this Oct 14, 2020
@ghost
Copy link

ghost commented Oct 14, 2020

The job Hazelcast-pr-builder of your PR failed (log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   CPMemberAddRemoveTest.when_snapshotIsTakenWhileRemovingCPMember_newMemberInstallsSnapshot:770->HazelcastTestSupport.assertTrueEventually:1355->HazelcastTestSupport.assertTrueEventually:1253->lambda$when_snapshotIsTakenWhileRemovingCPMember_newMemberInstallsSnapshot$17:773 expected:<[CPMember{uuid=acd94f9b-e75a-40f2-85fc-37829e2cdb78, address=[127.0.0.1]:5702}, CPMember{uuid=4edb6b0e-3cad-41cc-9898-d675253a2b9d, address=[127.0.0.1]:5703}, CPMember{uuid=ac4e1af3-357a-45f7-915f-595b73806e6c, address=[127.0.0.1]:5704}]> but was:<[CPMember{uuid=acd94f9b-e75a-40f2-85fc-37829e2cdb78, address=[127.0.0.1]:5702}, CPMember{uuid=2c374f42-f004-4c03-a4a5-2f01e1d72aee, address=[127.0.0.1]:5701}, CPMember{uuid=4edb6b0e-3cad-41cc-9898-d675253a2b9d, address=[127.0.0.1]:5703}]>
[INFO] 
[ERROR] Tests run: 32026, Failures: 1, Errors: 0, Skipped: 979
[INFO] 

[ERROR] There are test failures.

@kwart kwart removed the request for review from mmedenjak October 15, 2020 08:11
@kwart
Copy link
Member Author

kwart commented Oct 15, 2020

Thanks for the reviews. Merging.

@kwart kwart merged commit 73df1ea into hazelcast:master Oct 15, 2020
@kwart kwart deleted the fix/4.1/xml-xxe-security branch October 15, 2020 08:12
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Oct 15, 2020
@hazelcast hazelcast deleted a comment Oct 15, 2020
@hazelcast hazelcast deleted a comment from kwart Oct 15, 2020
@hazelcast hazelcast deleted a comment from kwart Oct 15, 2020
@cwansart
Copy link

Will this be fixed in the 3.x versions as well?

@kwart
Copy link
Member Author

kwart commented Oct 23, 2020

Will this be fixed in the 3.x versions as well?

I'll look into backporting this PR today.

@cwansart
Copy link

Thank you.

@thomasleplus
Copy link

Hello,

I am trying to understand what would be the potential attack vector for this issue to asses its severity in my use case. Are the fixed classes used to handle static XML files (e.g. configuration files) or could the XML be provided dynamically at runtime by a user. Of course the latter would be a bigger security concern.

Regards,

Tom

@mmedenjak
Copy link
Contributor

@kwart can you elaborate on the above?

@kwart
Copy link
Member Author

kwart commented Feb 15, 2021

@thomasleplus There is no known attack vector that would use the dynamic XML configuration. The fixed classes handle static XML configuration files.

@thomasleplus
Copy link

@kwart thanks, I appreciate the clarification.

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

Successfully merging this pull request may close these issues.

None yet

6 participants