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

Fix code scanning alert no. 442: Resolving XML external entity in user-controlled data #1078

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

jordanpadams
Copy link
Member

Fixes https://github.com/NASA-PDS/validate/security/code-scanning/442

To fix the problem, we need to disable external entity resolution in the TransformerFactory used in the SchematronTransformer class. This can be done by setting specific features on the TransformerFactory instance to disallow DTDs and external entities. This will prevent XXE attacks by ensuring that the XML parser does not process external entities.

The changes should be made in the buildIsoTransformer method of the SchematronTransformer class. We will set the necessary features on the TransformerFactory instance to secure it against XXE attacks.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…r-controlled data

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@jordanpadams jordanpadams marked this pull request as ready for review December 9, 2024 18:53
@jordanpadams jordanpadams requested a review from a team as a code owner December 9, 2024 18:53
@jordanpadams
Copy link
Member Author

@al-niessner can you review this to make sure it makes sense to you?

Copy link
Contributor

@al-niessner al-niessner left a comment

Choose a reason for hiding this comment

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

The code makes sense. What does not make sense is why. The user is just processing their own data on their own machine. It is not like there is some weird injection going from a user clicking on a funky link (phishing). In other words, they could blow up their machine by injecting malicious code or they could just take a hammer to it. Hammer seems more direct and likely than some convoluted XXE. Probably never even realize XXE was an option, but this code will force the hammer approach.

@jordanpadams
Copy link
Member Author

@al-niessner I agree with the why, but, in the future, if validate is deployed under the hood of some web app, this could potentially eliminate a risk.

@jordanpadams jordanpadams merged commit ca51b85 into main Dec 10, 2024
3 checks passed
@jordanpadams jordanpadams deleted the alert-autofix-XEE branch December 10, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants