Security Vulnerability - Action Required: XXE vulnerability in the newest version of the NullAway

[Crispy-fried-chicken]

I think your project may be vulnerable to Improper Restriction of XML External Entity Reference. It shares similarities to a recent CVE disclosure CVE-2021-3878 in the stanfordnlp/CoreNLP. The vulnerable methods are as follows:

1. com.uber.nullaway.fixserialization.XMLUtil.writeInXMLFormat(FixSerializationConfig config, String path) in the file nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java.
2. com.uber.nullaway.fixserialization.FixSerializationConfig.FixSerializationConfig(String configFilePath, int serializationVersion) in the file nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java

The source vulnerability information is as follows:

Vulnerability Detail: CVE Identifier: CVE-2021-3878 Description: corenlp is vulnerable to Improper Restriction of XML External Entity Reference Reference:https://nvd.nist.gov/vuln/detail/CVE-2021-3878. Patch: https://github.com/stanfordnlp/corenlp/commit/e5bbe135a02a74b952396751ed3015e8b8252e99.

Vulnerability Description: This vulnerability occurs because of the Improper Restriction of XML External Entity Reference. Given that the XML schema files which is compromised by a hacker, the victim conducts regular process may result in an XML External Entity (XXE) Injection attack.

Recommended Actions: The corresponding fixes are similar to CVE-2021-3878 to some extent. I have provided the following fixes by applying several patching statements, ensuring that the external entities and DTDs are not loaded when parsing and processing XML documents using the document builder. You can call the function safeDocumentBuilderFactory I defined below instead of directly calling DocumentBuilderFactory.newInstance() to create a DocumentBuilderFactory object to avoid XXE attacks.

  public static DocumentBuilderFactory safeDocumentBuilderFactory() {
    DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
    try {
      dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
      dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
      dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
      dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
      dbf.setFeature("http://apache.org/xml/features/dom/create-entity-ref-nodes", false);
      dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
    } catch (ParserConfigurationException e) {
      log.warn(e);
    }
    return dbf;
  }

Considering the potential riskes it may have, I am willing to cooperate with your to verify, address, and report the identified vulnerability promptly through responsible means. If you require any further information or assistance, please do not hesitate to reach out to me. Thank you and looking forward to hearing from you soon.

[msridhar]

@Crispy-fried-chicken thanks for the report. Do you have a realistic scenario where this can be exploited, and if so, can you describe it? I think to make this exploitable, a user would have to be feeding untrusted XML inputs into NullAway, which to me seems highly unlikely.

Also can you comment on how this issue was discovered? It looks like you have opened an identical issue on several GitHub repositories. If this was discovered using some kind of automated tooling / static analysis, it is courteous to disclose that in the issue report, so that maintainers have more context on what is going on.

All that said, it doesn't seem too difficult to fix this. If you could submit a pull request that would be helpful, and that way, you can sign the CLA so we can properly accept your code.

[Crispy-fried-chicken]

@msridhar thanks for your reply.

@Crispy-fried-chicken thanks for the report. Do you have a realistic scenario where this can be exploited, and if so, can you describe it? I think to make this exploitable, a user would have to be feeding untrusted XML inputs into NullAway, which to me seems highly unlikely.

After checking, I found that there is a call chain ErrorProneCLIFlagsConfig()-->com.uber.nullaway.fixserialization.FixSerializationConfig.FixSerializationConfig(String configFilePath, int serializationVersion) in your project, and the parsed xml file should be EP_FL_NAMESPACE + ":FixSerializationConfigPath "; If this configuration file is tampered with, it may lead to external entity injection attacks, thereby putting the user's system at risk. Maybe there are other call chains that I didn't find. I think this vulnerability should be fixed, or the user's system will be at risk.

Also can you comment on how this issue was discovered? It looks like you have opened an identical issue on several GitHub repositories. If this was discovered using some kind of automated tooling / static analysis, it is courteous to disclose that in the issue report, so that maintainers have more context on what is going on.

We detected this vulnerability through our self-developed recurring vulnerability detection tool. This tool mainly uses static analysis methods, and it has a high detection accuracy in our dataset. We have also received positive feedback from other projects before.

All that said, it doesn't seem too difficult to fix this. If you could submit a pull request that would be helpful, and that way, you can sign the CLA so we can properly accept your code.

That's for sure! I will submit a pull request.

[Crispy-fried-chicken]

@Crispy-fried-chicken thanks for the report. Do you have a realistic scenario where this can be exploited, and if so, can you describe it? I think to make this exploitable, a user would have to be feeding untrusted XML inputs into NullAway, which to me seems highly unlikely.

Also can you comment on how this issue was discovered? It looks like you have opened an identical issue on several GitHub repositories. If this was discovered using some kind of automated tooling / static analysis, it is courteous to disclose that in the issue report, so that maintainers have more context on what is going on.

All that said, it doesn't seem too difficult to fix this. If you could submit a pull request that would be helpful, and that way, you can sign the CLA so we can properly accept your code.

Hi, I've already submit a pull request(that is https://github.com/uber/NullAway/pull/902) . please sign the CLA and check it. Thank you!

[msridhar]

After checking, I found that there is a call chain ErrorProneCLIFlagsConfig()-->com.uber.nullaway.fixserialization.FixSerializationConfig.FixSerializationConfig(String configFilePath, int serializationVersion) in your project, and the parsed xml file should be EP_FL_NAMESPACE + ":FixSerializationConfigPath "; If this configuration file is tampered with, it may lead to external entity injection attacks, thereby putting the user's system at risk. Maybe there are other call chains that I didn't find. I think this vulnerability should be fixed, or the user's system will be at risk.

I agree that if some attacker could take control of the XML input passed in via this configuration flag, they could use it to perform an injection attack. However, I believe that in nearly any realistic case, the attacker would have had to gain some kind of local access in the first place to control this input. You would have to configure NullAway such that the attacker could somehow remotely control the XML configuration file in order to have a vulnerability otherwise. So, in my opinion, the risk here is low. But the fix is easy so we should do it.

We detected this vulnerability through our self-developed recurring vulnerability detection tool. This tool mainly uses static analysis methods, and it has a high detection accuracy in our dataset. We have also received positive feedback from other projects before.

Thanks for that context. As noted above, I suggest you provide this information up front with future reports.

That's for sure! I will submit a pull request.

I have reviewed your pull request, thanks!