update for missing a couple possibly unsafe xml parser #902

Crispy-fried-chicken · 2024-01-31T12:47:06Z

Fixes #901

CLAassistant · 2024-01-31T12:47:12Z

All committers have signed the CLA.

auke- · 2024-01-31T13:23:40Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java

+    } catch (ParserConfigurationException e) {
+      log.warn(e);
+    }


Note that catching and swallowing the exception makes it possible to return a document builder factory which is still vulnerable!

I've already delete this block, please check it again, thank you!

msridhar

Thanks for this contribution! I have a couple of comments that should be easy to address. Also you will need to agree to the CLA before we can merge this contribution (see the comment on the PR by the bot).

msridhar · 2024-01-31T20:35:35Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java

@@ -94,14 +94,25 @@ public static <T> DefaultXMLValueProvider<T> getValueFromTag(
    return new DefaultXMLValueProvider<>(null, klass);
  }

+  public DocumentBuilderFactory safeDocumentBuilderFactory() {


This method should be static. Also can we add a bit of Javadoc as to its purpose?

Of course! Thanks for your comment.

msridhar · 2024-01-31T20:36:25Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java

+  public DocumentBuilderFactory safeDocumentBuilderFactory() {
+    DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+    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);
+    return dbf;
+  }
+


Once we make the XMLUtil#safeDocumentBuilderFactory method static, we can get rid of this method, and just call XMLUtil.safeDocumentBuilderFactory() below.

sure, I've already make the XMLUtil#safeDocumentBuilderFactory method static. please check it again, Thank you!

yuxincs · 2024-02-01T14:19:09Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java

@@ -16,7 +16,7 @@
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * OUT OF OR IN CONNECTION WITH com.uber.nullaway.fixserializationTHE SOFTWARE OR THE USE OR OTHER DEALINGS IN


Can we revert this change? we shouldn't modify license headers unless there's a reason

yuxincs · 2024-02-01T14:23:41Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java

+
+

Can you run our configured formatter (./gradlew spotlessApply) on this PR?

We try to be consistent in formatting and these two blank lines don't seem right.

msridhar

In addition to the comment, based on the CI run looks like there are compile errors. You can run ./gradlew compileJava to ensure that all the code compiles

msridhar · 2024-02-01T17:18:33Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java

@@ -94,14 +94,25 @@ public static <T> DefaultXMLValueProvider<T> getValueFromTag(
    return new DefaultXMLValueProvider<>(null, klass);
  }

+  public static DocumentBuilderFactory safeDocumentBuilderFactory() {


Please add Javadoc describing what this method does

what your project use is jdk 21?

JDK 21 is required in order to run certain tests. Sorry for the hassle.

I‘ve already pushed and the ./gradlew spotlessApply is run successfully.

But the cli ./gradlew compileJava is failed cause it need to use jdk8? I don't know why, but I think it's not caused by the code I've added. Please check it again, and I will try to fix it if you find any error again. Feel sorry to trouble you again.

msridhar

Thanks for the contribution!

codecov · 2024-02-02T15:50:34Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (00a317a) 86.99% compared to head (dcafa95) 86.98%.

Files	Patch %	Lines
...va/com/uber/nullaway/fixserialization/XMLUtil.java	83.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #902      +/-   ##
============================================
- Coverage     86.99%   86.98%   -0.01%     
- Complexity     1958     1959       +1     
============================================
  Files            77       77              
  Lines          6319     6330      +11     
  Branches       1223     1223              
============================================
+ Hits           5497     5506       +9     
- Misses          418      420       +2     
  Partials        404      404

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Crispy-fried-chicken · 2024-02-02T15:56:35Z

Thanks for the contribution!

Thank you for your merge! Considering the possible information leakage consequences of this vulnerability, maybe we can request for a CVE-ID?

msridhar · 2024-02-02T16:02:02Z

Sorry no, I do not believe this deserves a CVE. See my previous comments.

update for missing a couple possibly unsafe xml parser

b6bb504

Crispy-fried-chicken mentioned this pull request Jan 31, 2024

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

Closed

auke- reviewed Jan 31, 2024

View reviewed changes

delete try catch block

4f29605

msridhar reviewed Jan 31, 2024

View reviewed changes

Crispy-fried-chicken added 2 commits February 1, 2024 14:05

changes the method to static

0daa7c2

update

705a3c6

yuxincs reviewed Feb 1, 2024

View reviewed changes

msridhar reviewed Feb 1, 2024

View reviewed changes

update for javadoc and fix some bugs

59ee9c1

Crispy-fried-chicken requested a review from msridhar February 2, 2024 07:54

fix formatting

dcafa95

msridhar approved these changes Feb 2, 2024

View reviewed changes

msridhar enabled auto-merge (squash) February 2, 2024 15:46

msridhar merged commit 848b0b1 into uber:master Feb 2, 2024
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update for missing a couple possibly unsafe xml parser #902

update for missing a couple possibly unsafe xml parser #902

Crispy-fried-chicken commented Jan 31, 2024 •

edited by msridhar

CLAassistant commented Jan 31, 2024 •

edited

auke- Jan 31, 2024

Crispy-fried-chicken Jan 31, 2024

msridhar left a comment

msridhar Jan 31, 2024

Crispy-fried-chicken Feb 1, 2024

msridhar Jan 31, 2024

Crispy-fried-chicken Feb 1, 2024

yuxincs Feb 1, 2024

yuxincs Feb 1, 2024

msridhar left a comment

msridhar Feb 1, 2024

Crispy-fried-chicken Feb 2, 2024

msridhar Feb 2, 2024

Crispy-fried-chicken Feb 2, 2024

Crispy-fried-chicken Feb 2, 2024

msridhar left a comment

codecov bot commented Feb 2, 2024

Crispy-fried-chicken commented Feb 2, 2024

msridhar commented Feb 2, 2024

update for missing a couple possibly unsafe xml parser #902

update for missing a couple possibly unsafe xml parser #902

Conversation

Crispy-fried-chicken commented Jan 31, 2024 • edited by msridhar

CLAassistant commented Jan 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2024

Codecov Report

Crispy-fried-chicken commented Feb 2, 2024

msridhar commented Feb 2, 2024

Crispy-fried-chicken commented Jan 31, 2024 •

edited by msridhar

CLAassistant commented Jan 31, 2024 •

edited