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

Disable loading external DTDs by default, create system property to activate it #6474

Closed
romani opened this issue Feb 24, 2019 · 4 comments
Closed

Comments

@romani
Copy link
Member

romani commented Feb 24, 2019

Ability to load external DTDs is considered as security issue, especially if DTDs are not hosted on https hosting.
Checkstyle used to rely on remote DTD files previously so such ability was enabled by default.
From 8.11 version checkstyle do not rely on remove DTDs if you use standard config, with standard DTDs.

UPGRADE INSTRUCTIONS: Users still can activate not very secure behavior by system property checkstyle.enableExternalDtdLoad, so simply make your CLI execution like java -Dcheckstyle.enableExternalDtdLoad=true -jar .......
Or activate system property by any other way in your checkstyle execution.

ATTENTION: user on old versions of checkstyle that reference to DTD files http (not secure) hosting on sourceforge.net will be forced to upgrade to at least to https versions of DTD files. Removal of DTDs on http hosting will be done in scope of - #6478.

@romani
Copy link
Member Author

romani commented Feb 25, 2019

without new system property checkstyle might failure the same way as failed our UTs:

java.lang.NullPointerException
    at com.sun.org.apache.xerces.internal.impl.dtd.XMLDTDProcessor.endDTD(XMLDTDProcessor.java:1349)
    at com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.endEntity(XMLDTDScannerImpl.java:652)
    at com.sun.org.apache.xerces.internal.impl.XMLEntityManager.endEntity(XMLEntityManager.java:1401)
    at com.sun.org.apache.xerces.internal.impl.XMLEntityScanner.load(XMLEntityScanner.java:1917)
    at com.sun.org.apache.xerces.internal.impl.XMLEntityScanner.skipSpaces(XMLEntityScanner.java:1655)
    at com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.skipSeparator(XMLDTDScannerImpl.java:2082)
    at com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.scanDecls(XMLDTDScannerImpl.java:2058)
    at com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.scanDTDExternalSubset(XMLDTDScannerImpl.java:301)
    at com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl$DTDDriver.dispatch(XMLDocumentScannerImpl.java:1175)
    at com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl$DTDDriver.next(XMLDocumentScannerImpl.java:1045)
    at com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl$PrologDriver.next(XMLDocumentScannerImpl.java:933)
    at com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl.next(XMLDocumentScannerImpl.java:602)
    at com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanDocument(XMLDocumentFragmentScannerImpl.java:505)
    at com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:841)
    at com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:770)
    at com.sun.org.apache.xerces.internal.parsers.XMLParser.parse(XMLParser.java:141)
    at com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.parse(AbstractSAXParser.java:1213)
    at com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser.parse(SAXParserImpl.java:643)
    at com.puppycrawl.tools.checkstyle.XmlLoader.parseInputSource(XmlLoader.java:84)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoader.parseInputSource(ConfigurationLoader.java:197)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoader.loadConfiguration(ConfigurationLoader.java:443)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoader.loadConfiguration(ConfigurationLoader.java:395)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoader.loadConfiguration(ConfigurationLoader.java:372)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoader.loadConfiguration(ConfigurationLoader.java:209)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoaderTest.loadConfiguration(ConfigurationLoaderTest.java:62)
    at com.puppycrawl.tools.checkstyle.ConfigurationLoaderTest.testExternalEntity(ConfigurationLoaderTest.java:410)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
    at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
    at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
    at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

@romani
Copy link
Member Author

romani commented Feb 25, 2019

fix is merged and released in 8.18

@romani
Copy link
Member Author

romani commented Mar 15, 2019

Github will detect vulnerability and report it as:
image

image

we are affected probably by this non-compile dependency in plugins of verify phase:

<maven.sevntu-checkstyle-check.checkstyle.version>
      8.12
</maven.sevntu-checkstyle-check.checkstyle.version>

Fokko added a commit to Fokko/spark that referenced this issue Aug 13, 2019
Fixes a vulnerability from the GitHub Security Advisory Database:

_Moderate severity vulnerability that affects com.puppycrawl.tools:checkstyle_
Checkstyle prior to 8.18 loads external DTDs by default, which can potentially lead to denial of service attacks or the leaking of confidential information.

checkstyle/checkstyle#6474

Affected versions: < 8.18

Ran checkstyle locally.

Closes apache#25432 from Fokko/SPARK-28713.

Authored-by: Fokko Driesprong <fokko@apache.org>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit d8dd571)
dongjoon-hyun pushed a commit to apache/spark that referenced this issue Aug 13, 2019
## What changes were proposed in this pull request?

Backport to `branch-2.4` of #25432

Fixes a vulnerability from the GitHub Security Advisory Database:

_Moderate severity vulnerability that affects com.puppycrawl.tools:checkstyle_
Checkstyle prior to 8.18 loads external DTDs by default, which can potentially lead to denial of service attacks or the leaking of confidential information.

checkstyle/checkstyle#6474

Affected versions: < 8.18

## How was this patch tested?

Ran checkstyle locally.

Closes #25437 from Fokko/branch-2.4.

Authored-by: Fokko Driesprong <fokko@apache.org>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
rluta pushed a commit to rluta/spark that referenced this issue Sep 17, 2019
## What changes were proposed in this pull request?

Backport to `branch-2.4` of apache#25432

Fixes a vulnerability from the GitHub Security Advisory Database:

_Moderate severity vulnerability that affects com.puppycrawl.tools:checkstyle_
Checkstyle prior to 8.18 loads external DTDs by default, which can potentially lead to denial of service attacks or the leaking of confidential information.

checkstyle/checkstyle#6474

Affected versions: < 8.18

## How was this patch tested?

Ran checkstyle locally.

Closes apache#25437 from Fokko/branch-2.4.

Authored-by: Fokko Driesprong <fokko@apache.org>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
kai-chi pushed a commit to kai-chi/spark that referenced this issue Sep 26, 2019
## What changes were proposed in this pull request?

Backport to `branch-2.4` of apache#25432

Fixes a vulnerability from the GitHub Security Advisory Database:

_Moderate severity vulnerability that affects com.puppycrawl.tools:checkstyle_
Checkstyle prior to 8.18 loads external DTDs by default, which can potentially lead to denial of service attacks or the leaking of confidential information.

checkstyle/checkstyle#6474

Affected versions: < 8.18

## How was this patch tested?

Ran checkstyle locally.

Closes apache#25437 from Fokko/branch-2.4.

Authored-by: Fokko Driesprong <fokko@apache.org>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants
@romani and others