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.

Sign up for GitHub

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

Jump to bottom

Improve DTD/XSD security with regard to remote resources #1183

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Improve DTD/XSD security with regard to remote resources #1183

merged 1 commit into from
Mar 29, 2022

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Mar 4, 2022 •
edited
Loading

Improve DTD/XSD security with regard to remote resources

Fixes #671

Signed-off-by: azerr azerr@redhat.com

@angelozerr angelozerr marked this pull request as draft March 4, 2022 13:57
@angelozerr angelozerr force-pushed the dtd-security branch 7 times, most recently from 43c3a6e to 50a9565 Compare March 4, 2022 17:51
@angelozerr
Copy link
Contributor Author

This PR fixes following problem with our DTD validator:

  • higlight the SYSTEM url which have problem
  • use xml.validation.resolveExternalEntities used to enable/disable the resolved of external entities.
  • use xml.downloadExternalResources.enabled to allow/forbid download of SYSTEM url inside DTD.

Here a demo with this draft PR:

DTDDownloadSupport

@angelozerr angelozerr mentioned this pull request Mar 4, 2022
Closed
@angelozerr angelozerr force-pushed the dtd-security branch 2 times, most recently from bf7ab90 to 1958b29 Compare March 7, 2022 08:48
@angelozerr
Copy link
Contributor Author

angelozerr commented Mar 7, 2022 •
edited
Loading

The PR provides now a new code action to download a given DTD/ entity when downloaded is forbidden:

image

It requires redhat-developer/vscode-xml#673

@angelozerr
Copy link
Contributor Author

@fbricon you can start playing with the PR. All tests are working now.

I need to check if XSD is working too (when you reference xsd with xs:import)

@angelozerr angelozerr force-pushed the dtd-security branch 8 times, most recently from 2105e8a to 3a15e47 Compare March 27, 2022 08:45
@angelozerr angelozerr marked this pull request as ready for review March 27, 2022 09:03
@angelozerr
Copy link
Contributor Author

angelozerr commented Mar 27, 2022 •
edited
Loading

I need to check if XSD is working too (when you reference xsd with xs:import)

It should work with XSD now with the code action too.

@angelozerr angelozerr force-pushed the dtd-security branch 4 times, most recently from c04a69a to da8c2bf Compare March 27, 2022 15:29
fbricon
fbricon reviewed Mar 28, 2022
View reviewed changes
...main/java/org/eclipse/lemminx/extensions/contentmodel/commands/XMLValidationFileCommand.java
// 1. remove the referenced grammar in the XML file from the Xerces grammar pool
// (used by the Xerces validation) and the content model documents cache (used
// by the XML completion/hover based on the grammar)
contentModelManager.evictCacheFor(document);
// 2. trigger the validation for the given XML file
validationService.validate(document);
Map map = JSONUtility.toModel(validationArgs, Map.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens when validationArgs == null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No force download url will be done when validation will occurs. The null arguments is managed at https://github.com/eclipse/lemminx/pull/1183/files#diff-07b75b69658ecc851282ac49a9a36f30a26fcb8e26004e6705d279dc094af2ebR91

fbricon
fbricon reviewed Mar 28, 2022
View reviewed changes
...nx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/model/ContentModelProvider.java
* @return the content model document (XSD, DTD, etc) from the given resource
* key and null otherwise.
*/
CMDocument createCMDocument(String key, boolean resolveExternalEntities);
Copy link
Contributor

@fbricon fbricon Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

resolveExternalEntities or downloadRemoteResources?

Copy link
Contributor Author

@angelozerr angelozerr Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • resolveExternalEntities is used to configure the Xerces SAX parser when any XML, DTD, XSD is loaded. The resolveExternalEntities is a feature that Xerces supports.
  • downloadRemoteResources is managed on CacheResourcesManager side when an URL must be downloaded. Its XMLCacheResolverExtension an implementation of Xerces XMLEntityResolver which provides the capability to returns the content of an URI.

In other words, we need here only resolveExternalEntities to configure Xerces SAX parser.

fbricon
fbricon reviewed Mar 28, 2022
View reviewed changes
...n/java/org/eclipse/lemminx/extensions/dtd/participants/diagnostics/LSPXML11DTDProcessor.java Outdated
*
* <p>
* This class extends {@link XML11DTDProcessor} and customize the XML entity
* manager with the {@link LSPXMLEntityManager} which takes care of download
Copy link
Contributor

Choose a reason for hiding this comment

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

which handles the "download external entities" setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My comment was wrong because it manages any error when download is processed. So I think

any remote resource download errors.

should be better, what do you think about that?

@angelozerr angelozerr changed the title Arbitrary File Read Improve DTD/XSD security with regard to remote resources Mar 28, 2022
@angelozerr angelozerr force-pushed the dtd-security branch 4 times, most recently from f7713ff to 0142632 Compare March 28, 2022 08:15
AlexXuChen
AlexXuChen reviewed Mar 28, 2022
View reviewed changes
Copy link
Contributor

@AlexXuChen AlexXuChen left a comment

Choose a reason for hiding this comment

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

It seems that no validation appears when xml.server.preferBinary is enabled:
xml.server.preferBinary is false
Screenshot from 2022-03-28 11-18-15
xml.server.preferBinary is true
Screenshot from 2022-03-28 11-18-50

AlexXuChen
AlexXuChen reviewed Mar 28, 2022
View reviewed changes
Copy link
Contributor

@AlexXuChen AlexXuChen left a comment

Choose a reason for hiding this comment

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

Everything using the Java server is working for me

@angelozerr
Copy link
Contributor Author

It seems that no validation appears when xml.server.preferBinary is enabled:

@rgrunber can you confirm please that it works with binary in Linux please. I tested with Windows and ot works good.

@angelozerr angelozerr added this to the 0.19.2 milestone Mar 28, 2022
@angelozerr angelozerr added bug Something isn't working validation XSD DTD labels Mar 28, 2022
@rgrunber
Copy link
Contributor

As mentioned, this works for me and I can be merged. I found an odd issue around the code actions when the schema reference URL contains a query string, but the change itself still worked in restricting the download as needed.

@angelozerr angelozerr merged commit c646073 into eclipse:master Mar 29, 2022
@angelozerr
Copy link
Contributor Author

Thank a lot @fbricon @AlexXuChen @rgrunber for your review!

@angelozerr angelozerr added enhancement New feature or request and removed bug Something isn't working labels Mar 29, 2022
@angelozerr angelozerr mentioned this pull request Mar 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbricon fbricon fbricon left review comments

@AlexXuChen AlexXuChen AlexXuChen left review comments

Assignees
No one assigned
Labels
DTD enhancement New feature or request validation XSD
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.

Improve DTD/XSD security with regard to remote resources
4 participants
@angelozerr @rgrunber @fbricon @AlexXuChen