search

hunterhacker / jdom

Java manipulation of XML made easy
Other
348 stars 118 forks source link

Got security warning on JDom2 2.0.6: CVE-2021-33813 #189

Closed steinarb closed 2 years ago

steinarb commented 3 years ago

I got a security warning on JDom 2.0.6, because of CVE-2021-33813: https://nvd.nist.gov/vuln/detail/CVE-2021-33813

From the severity of CVE-2021-33813 it looks like this is something that needs to be fixed.

oosterholt commented 3 years ago

A pull request is already created: #188

hunterhacker commented 3 years ago

If you call builder.setExpandEntities(false) then JDOM won't expand entities. The issue here is people may try to set this only via direct calls to the parser and not with the official JDOM solution (which takes precedence).

https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html

I intend to accept the pull request (doesn't hurt, small benefit) but I'm not sure I can push to Maven without either Rolf or a lot of trouble.

oosterholt commented 3 years ago

Any news on this? Did you accept the pull request and did you manage to push to maven?

hunterhacker commented 3 years ago

I accepted. I don't have the maven credentials since that was Rolf's business.

oosterholt commented 3 years ago

Ok. Nice. So, when can we expect a release?

hunterhacker commented 3 years ago

Maybe a non-maven for now?

memonahad commented 3 years ago

@hunterhacker any idea on when we could see an official Maven release with this fix?

jcoker85 commented 3 years ago

@hunterhacker Also interested in a Maven release date as soon as possible. Thanks!

albertus82 commented 3 years ago

@hunterhacker There are a lot of libs that ultimately depend on jdom2 and now every application that use one of them has CVE-2021-33813, so I hope that a Maven release will follow shortly. Thank you in advance.

reactivejson commented 3 years ago

@hunterhacker we are waiting for a maven release as well, thanks in advance!

ar commented 3 years ago

Hope you can consider the super simple PR #185 as part of the release.

It would make things easier for projects targeting JDK17.

rolfl commented 3 years ago

OK, folk, lots of catching up to do, a mail configuration SNAFU resulted in me missing notifications on JDOM for a while, and I have to collect some content together to push this through (and obviously work to ensure I am not a single point of failure on this process).

Please be patient while I work though all the checks/balances, and ensure all the parts align before pushing a new release.

rolfl commented 3 years ago

Right, merged to master all the changes in the backlog, including a number of fixes and test case changes related to this issue.

Going to get back at this in the next day - will need to address some other issues before releasing a new build still.

schlm3 commented 3 years ago

Another ten days later still no release. What are your plans concerning the release of the security fix?

ksaiganeshreddy commented 3 years ago

when can we expect a release with this security fix??

swapneel-kore commented 3 years ago

I am interested in an update with this security fix as well. Any indication when a release could be coming?

arvindjagtap8927 commented 3 years ago

I am also interested in this security fix. When this fix will be released?

hunterhacker commented 3 years ago

I've been waiting for @rolfl to step in here. I'm not exactly sure why he's not. Another email snafu? I tried pinging him privately.

Meanwhile, if you're looking at this issue and are worried about the risks of expanding entities, call the nicely named function setExpandEntities(false) which turns off expanding entities. That always works perfectly.

It's important to note the PR doesn't change any default behaviors. Entities still expand by default. The PR only changes what happens when you don't call the nicely named function but instead talk to the underlying parser directly by setting the obscurely named "http://xml.org/sax/features/external-general-entities" feature to false. If that's what your code does, then might I suggest you change your code to use the nicely named function as well.

I recognize once there's a CVE people want an update. I'm just pointing out the update won't change default behaviors and will only improve the behavior of code that has a calling pattern that isn't robust.

chadlwilson commented 3 years ago

Thanks for helping out @hunterhacker.

To me, it seems the linking of the CVE here, and its current generic description (arguably a bit alarmist?) is creating a slightly misleading impression to folks who are seeing this pop up (possibly from a transitive dependency) that a fix in the library is the only thing required to magically close off a possible XXE attack for people's system.

In reality it seems all the fix does is make the library act according to "principle of least surprise" if a consumer followed the earlier OWASP generic recommendation (prior to this commit) or was doing the generic SAX thing via setFeature.

So the library never prevented anyone from blocking off XXE. One just needed to do it via the setExpandEntities route, not the setFeature route.

So it's a little confusing to me. Seems the earlier OWASP recommendation wasn't tested/verified!? setFeature does have quite a disclaimer also: https://github.com/hunterhacker/jdom/blob/03a391ad6f5a10b3f6f13e5a620412d0e60351c5/core/src/java/org/jdom2/input/SAXBuilder.java#L778-L783

If I read correctly, the current plan/merged PR is still dependent on people disabling the functionality with either the builder or the setFeature route, as it doesn't change the behaviour to "secure by default" which would cut off the vulnerability for those who are consuming JDOM transitively via another library. That library would have to have been following the earlier OWASP recommendation in its code, and the behaviour will change. So direct consumers still need to review their code and usages of SAXBuilder, and some of them may not have been doing anything to disable XXE at all in the first place.

My broad summary (please correct me if I am wrong) is

  1. if the consuming code makes no attempt to call setFeature("http://xml.org/sax/features/external-general-entities", false) or setExpandEntities(false) then they have a potential XXE attack problem regardless of this CVE (depending on how they receive+use XML documents) making the CVE irrelevant.
  2. if the consuming code already called setExpandEntities(false) in relevant places, then they are not affected by this CVE and can safely suppress it.
  3. if the consuming code already called setFeature("http://xml.org/sax/features/external-general-entities", false) on its own and expected it to be enough to protect them, then the fix in #188 will magically fix it for them without code change; however they still have the ability to resolve it with setExpandEntities(false) right now.

Am I missing something? I am curious how many real world usages are in the #3 group.

Nevertheless, if we look at this CVE more as

setFeature should work as expected to disable entity expansion

with the broad class of

unintentional security misconfiguration possibly allows unintended XXE

that would seem more accurate to me than

An XXE issue in SAXBuilder in JDOM through 2.0.6 allows attackers to cause a denial of service via a crafted HTTP request

... which does not seem an accurate description of the problem to me, and assumes specific usage context.

j-be commented 3 years ago

@chadlwilson This still doesn't change the fact that (A) the CVE was published over a month ago and (B) the commit fixing the issue (if I am not mistaken) has been there since February and (C) nothing seems to have happened since.

So while I do agree that this fix is not a silver bullet for avoiding XXE, I'm still itchy about how this project handles the issue. They could have just thrown out a half-baked 2.0.6.1 as 2.0.6 + PR#188 advertised as such with just the fix, and do whatever they are doing now for a more polished 2.0.7. So people interested in not having this showing up in their vulnerability scanner could just switch to 2.0.6.1, all other can wait for 2.0.7.

chadlwilson commented 3 years ago

@j-be To be fair, commits were merged a few weeks ago, so that's not "nothing", however It's a bit odd that a quick release hasn't been cut after, I agree. Since the project has had basically no development for over 6 years it seems (as measured by releases), any signs of life are better than nothing.

Nevertheless, the attribution and description of the CVE in this case is a bit dubious IMO. It has created a lot of noise here, amplified by it being on a mature, but old library that is still a common dependency deep down inside a lot of Java software. I feel some more active/mature projects might have had cause to dispute some aspects of the CVE, however no CVE can be looked at blindly without taking into consideration the context of the software's usage. To me, this means context-dependent suppression is a reality of life.

While I agree that it is impossible to assess all ones transitive dependencies, and vulnerability scanner noise is annoying (it's what lead me here also), it doesn't help make our systems more secure to have (what seems to me to be) FUD out there in CVE descriptions and their linked CWE. Let's say a new version is released. I upgrade. The noise goes away, but am I any more secure? Quite likely not. Might make us feel better, but that's not the same as being more secure.

If you don't have the inclination to dig yourself, one suggestion I would have is to look at the next most immediate consuming dependency(ies?) up your chain, and ask for them to assess whether that library is actually vulnerable in its usage of jdom. If not, you can probably suppress and move on. If so, they can follow the suggestions above to mitigate, and then can suppress?

j-be commented 3 years ago

@chadlwilson I agree with most of what you say. But I have to disagree on the "am I more secure".

What I (average developer) tend to do if something pops up on the scanner is: does the update break anything? If not: upgrade. I don't even care about what is or was affected, and if and how it could be exploited in our specific software. As long as a patched version is available and doesn't break anything I upgrade - no questions asked. In the end: chances are I don't even grasp if and how and what the security implications are when directly using a library, leave aside having to check umpteen layers of transitive dependencies for potentially exploitable situations.

The average developer may not even be able to determine this, either because of skill, or simply because assessment means sinking a lot of time into evaluating the issue. Changing a version in pom.xml or build.gradle is done in seconds. If you have a security department in your company it is a different story. But truth is: some of us simply don't. So for some of us the scanners and timely patches is all we have.

With all of this: "Am I more secure?" I'd say "Hell yeah" - not for every individual case, but in long term for sure. I personally simply don't have the time, and for some not even the skill to assess every single vulnerability. And underestimating a single one may be Game Over.

chadlwilson commented 3 years ago

Well, you are taking my words rather out of context by applying them generally, rather than to this specific case as I articulated, so that's not really fair. As a general rule, upgrade first, ask questions later makes sense. I spoke about this specific case, and indicated that maybe it does not.

I did explicitly say "it is impossible to assess all ones transitive dependencies" the equivalent as you, and I also approach upgrading this way. I was just trying to help others, by synthesising what I read across the place, and indicating that in this specific case, it may not affect them if their direct usage is a particular style, and that chasing the devs here because a 3rd party reported a security vulnerability on the basis of "THERE'S A CVE FIX IT FIX IT" and asking them to make the noise go away even if it does little doesn't appear very fair.

Software and supply chain complexity is a big problem for users for sure. It relies on each layer in the chain to be able to translate context to the layer above; to avoid using dead/deprecated dependencies and migrate away; to keep being maintained have have an easy+regular patch process. We don't appear to have a sufficient way of keeping on top of that complexity right now. We have these scanners that cut through the layers, say "oh you have jdom here" and then present users with information that they often don't have the expertise to judge (security department or not) because they are working with a higher level abstraction. They all arrive on Github on the deepest dependency going "fix it fix it", and the middle tiers that actually use that dependency, have necessary context, and may already have a mitigation in place aren't always interrogated for the necessary context. Just reflecting that it doesn't seem to help us very much as either "don't want to go deeper" users of a transitive dependency or "direct" users, who have control of the code to have vagueness in vuln descriptions.

j-be commented 3 years ago

@chadlwilson Sorry, I actually understood your statement as "generally" - my bad.

But for this specific case I still hold up my criticism: if other libraries use jdom's setFeature("http://xml.org/sax/features/external-general-entities", false) rather than setExpandEntities(false) they are well within spec, and should be safe.

Furthermore, the OWASP recommendation does that as well. It seems only because of http://apache.org/xml/features/disallow-doctype-decl, false that piece of code is not affected - as long as Xerces 2 is used under the hood that is.

Hence, I would not consider this "noise". It may not affect everybody here - I for one am pretty sure by now I am not. But I still want this fixed asap, as I may be affected tomorrow without knowing it (think new dependency comes in, Bob the intern parses XML, stuff like that), or I may be simply wrong and I am affected after all.

We could of course hunt down every single library using jdom the vulnerable way and patch all of those. And I'm not saying that is a bad idea: every additional layer helps. But the root cause for this is jdom not behaving to spec, not other libraries using jdom the wrong way - that is a different topic all together.

So as far as I am concerned: count me in on the "THERE'S A CVE FIX IT FIX IT": It is a vulnerability (no doubt about that), an exploit is possible (I don't mean to say likely, but that is not even my point), so it needs to be fixed. Now. Period.

BrianGGreen commented 3 years ago

Any update here?

tballison commented 3 years ago

Any update here?

Ditto. https://issues.apache.org/jira/browse/TIKA-3506

Many, many thanks for jdom2!

robbr commented 3 years ago

Thanks for the hard work on this one JDOM team!

I have read the comments and have learned some things.

Like the others, I would appreciate it very much if the fix would released to Maven,

hunterhacker commented 3 years ago

Does anyone work at IBM who could ping Rolf Lear there and check why he came back and then left?

cspocsai commented 3 years ago

Really appreciate the hard work JDOM team! If you have time please release it to maven as we are waiting for it. Thanks in advance!

mkoree commented 3 years ago

@hunterhacker Thank you for the update on this. We are also impacted by the issue hence would appreciate a Maven release as soon as possible. Thanks!

CoolR7 commented 3 years ago

We are also impacted by this issue when can we expect a maven release.. Thanks !

hamburml commented 3 years ago

This is crazy.

xmwuwei commented 3 years ago

Any update?

Kpatric commented 3 years ago

Any update on the release?

hunterhacker commented 3 years ago

Did anyone working at IBM reach out to Rolf?

rolfl commented 3 years ago

a few things...

The above stuff is going to take a number of days to sort out what to do, whether to put out a new JDOM 3.x with different Java version compatibility, etc? Also, it will take a bunch of time to sort out the certificates, and understand how the new release process should work with oss.sonatype

Given other factors, I have simply not had the full stretches of time available to sort out all these issues.... yes, it looks like just a few days work to get it done, but there will need to be a bunch of considerations that need to be agreed on ahead of time, and I have simply not had time (for personal reasons) to get that all done. .... and yes, some of those personal reasons have meant that I have not even had internet connections for long stretches of time (social distancing in the wilderness of Canada for summer holidays is a good thing.... ).

So, there's no real urgency to this PR since, as Json mentioned, the "fix" that this PR applies is EXACTLY what setExpandEntities(false) does : https://github.com/hunterhacker/jdom/blob/aa6f028f8ddbfebcf69380f984c6c1461f8bf85c/core/src/java/org/jdom2/input/SAXBuilder.java#L718

schlm3 commented 3 years ago

The base issue is not an issue - turning off entity expansion is as simple as setting builder.setExpandEntities(false) (As Jason mentioned here: Got security warning on JDom2 2.0.6: CVE-2021-33813 #189 (comment) there's no need for the new/redundant mechanism.... the current code is JUST FINE

That's not true, imho. Apart from the official JDOM proprietary API with setExpandedEntity(...), JDom is also supporting the official way of configuring the underlying SaxParser using SaxBuilder.setFeature(..). A developer using saxBuilder.setFeature("http://xml.org/sax/features/external-general-entities", false) should safely assume, that this call does what it is expected to do. BUT IT DOES NOT. This is the BUG you should fix since months now.
If you think, that saxBuilder.setFeature("http://xml.org/sax/features/external-general-entities", false) should not be used at all with JDOM, then deprecate/remove that API from JDOM or at least state it clearly in the Docs (which wouldn't solve any of the existing code using the API today of course).

I am very aware of the fact, that you do not get paid for this kind of work and that you are trying to do your best to further maintain the project. But if you state, that "this is not a bug", simply because you are not able to deliver the fix, then there is really something wrong.

rolfl commented 3 years ago

Trying to understand your comment schlm3 - note the base issue is that expanding entities is a security risk. I get that. The stated and accepted resolution for all parties as I can tell, is to turn off entity expansion.

JDOM's documentation is clear that the right way to do that is to call setExpandEntity(false) - which does set the sax feature, but it also does MORE than that - turning off just that feature has side effects that need to be accounted for when parsing.

Setting features on the underlying SAX parser directly is supported with the setFeature function, yes, but the documentation also clearly states http://www.jdom.org/docs/apidocs/org/jdom2/input/SAXBuilder.html#setFeature(java.lang.String,%20boolean) :

NOTE: SAXBuilder requires that some particular features of the SAX parser be set up in certain ways for it to work properly. The list of such features may change in the future. Therefore, the use of this method may cause parsing to break, and even if it doesn't break anything today it might break parsing in a future JDOM version, because what JDOM parsers require may change over time. Use with caution.

So, a developer using setFeature(...) cannot safely assume the call does what is expected. That function exposes the internals of the sax parser in a dangerous way, and is documented that way.

Now, I agree that it is preferable that setting the feature does the same as setExpandEntities(false), and that it does not do the same as that call, BUT, even if it did the same, it would still be the WRONG way to do it using JDOM.

The right JDOM way to resolve the security issue is to use setExpandedEntities(...) ..... and yes, the setFeature(...) option should be a reliable backup, but the priority for this is thus lower.

I am not saying that this bug does not exist, I am saying that the correct way to address the security vulnerability is to use the existing, documented, and reliable way to turn off entity expansion.

The dangerous, setFeature() way will be fixed too... just not as urgently.

Note that on a personal level, when this issue came through, I was in the process of winding up my previous employment at IBM, and I am now employed elsewhere. I was not in a position where I could spend days trying to recreate and validate a build environment that produces JAR files compatible with Java 1.5 and later. I am still not there in my new job - I have commitments that are hard to put aside to spend days trying to build code that already works, when used as documented.

If you are desperate to use the not-recommended way to set the feature, the code in the repo does things in the right order now, and just needs to be built in an environment where it still passes all the test cases in Java 1.5, and in the android build environments. If you could help document how to locate the right Java SDK's and recreate build steps to get it validated on android, that would save me a bunch of time.

schlm3 commented 3 years ago

Dear Rolf, Thank you for your detailed explanation. That really explains a lot to me. I am pretty sure, that the quoted "NOTE" (from the setFeature() documentation) is indeed not clear enough to most developers using that API. I mean, how should a developer which uses that API know which features are not save to use with this method? If all features are not save to use, why do you still provide that setFeature() method? Why isn't it deprecated?

To be clear: all of our own, internal code uses JDom in the save way (not using setFeature(..), but setExpandEnities(false)). But as @j-be stated before, I can not speak for third-party libraries code. So our next library update which uses JDom transitively might come with the vulnerable configuration in it as long as JDOM does not fix this and delivers the update to maven.

Concerning your personal situation, I can totally understand that. I just had the impression, that you do not take the issue serious. Concerning backward compatibility, is there really someone still using Java 1.5 ? Unfortunately, I do not have any experience with Java for Android.

j-be commented 3 years ago

@eschulma Not actually true, see here. It is really easy to overlook though and I, too, did not see it as I wrote my comment a few weeks ago.

Anyway, I still think, that if everybody thinks supporting saxBuilder.setFeature("http://xml.org/sax/features/external-general-entities", false) is a good idea, so should JDom2.

If not, I'd propose to throw an IllegalArgumentException, or something like that if someone tries to use it anyway. On this things imho. its better to be safe than sorry.

And as @schlm3 I really doubt 1.5 is still worth supporting. Imho. even 7 is questionable, 8 is as far as I'd ever go.

albertus82 commented 3 years ago

To ensure compatibility with JRE 6, the new --release flag of javac might be enough: http://openjdk.java.net/jeps/247 (available in JDK 9+). Unfortunately the lowest supported target version is 6, not 5.

ionutss commented 3 years ago

Still waiting for an official release. This can't be fixed in our code, as mentioned above, since it is used by other 3rd parties.

eneklo commented 3 years ago

We are forced to remove this artifact from our production systems because this vulnerability has became too old and there is no fix released. Probably we will upload a fixed version to our internal Maven repository in order to keep temporarily our application in production, and then will consider how to opt out definitely. I think we can consider this project abandoned; maybe someone else could take care of taking it forward, anyway thank you for the work done so far.

eschulma commented 3 years ago

@eneklo Let's be supportive, I see commits from 10 days ago, this hardly seems abandoned. If you are going to make a private fixed version, why not open a pull request and benefit everyone.

j-be commented 3 years ago

@eschulma As far as I understood there is already a PR for this, i.e. #188, which has been merged quite a while ago. So as @rolfl already pointed out at the very end of his comment from a month ago, "releasing a fixed version" is equivalent to "build from master and deploy".

What I really don't understand, is why nobody from the project team does just that. A half-baked 2.0.6.1-jre8 release with Java 8 as minimum requirement and unknown Android support is most probably what 99% of people here are waiting for. Once that is done I for one do not care how long it takes to get the next "good" version for Java 5 and Android ready - because I think (Disclaimer: uneducated guess) very few people actually do.

Btw. if someone could express in more detail what is holding up the release from a technical perspective I am sure somebody here could step in and help.

So for all I am concerned, it could as well be, that the team is not able to publish new releases anymore - for whatever reason. And if that truly is the case, recent commits are imho. not a metric for how alive this project is.

hunterhacker commented 3 years ago

Hi, I’m still here. Wave!

The challenge is Rolf handled all JDOM2 releases, and I don’t have the recipe or the credentials to push a new build out for Maven. I could, however, put a build on the JDOM.org website, if people would find that useful. Let me know.

Earlier in the thread Rolf explained how he wanted to be careful in doing the release so that doing a tiny (mostly cosmetic) fix here doesn’t cause issues with compatibilities that trip up people who will just blindly pull in the update via Maven.

It’s a real risk, and so while it’s true I could ask somewhere to get special access to the credentials and figure out the system he used to push it out, there’s definitely risk there it’ll break something. It’s more risk if I do than if he does, so I’d prefer if he did it, but he’s sure taking a long time now isn’t he. :)

eneklo commented 3 years ago

Hi @hunterhacker, I just forked the project, created a branch named jdom-2.0.6.1 from the tag JDOM-2.0.6 and cherry picked the following 4 commits (I hope I included all the relevant ones):

I built using Apache Ant 1.9.16 and Oracle JDK 1.7.0_75 that is the same version used to build v2.0.6 which is on Maven Central; then I compared the resulting JAR with the one of v2.0.6: there is only one class file changed: SAXBuilder.class; I also compared SAXBuilder.java source with master and there are no code differences (only some fixed typos on master).

build.log attached.

Now, theoretically and obviously with your kind permission, I might sign the new artifact and upload it to Sonatype using my Group ID and the version 2.0.6.1, but I think that you could do the same thing avoiding a fork.

Thank you.

eneklo commented 3 years ago

Sorry, but I honestly don't know what else to do to help unlock this situation.

hunterhacker commented 3 years ago

Eneklo, how's it work when you do the "maven" ant target?

Rolf wrote up how he got things into Sonatype long ago for JDOM 1.x: https://github.com/hunterhacker/jdom/wiki/JDOM1.1.2-and-Maven

He wrote things up a bit more tersely for JDOM 2.x: https://github.com/hunterhacker/jdom/wiki/Build%26Release-process-for-JDOM

He's the only one who ever held the Sonatype account credentials. I've asked Sonatype if they might give them to me.

eneklo commented 3 years ago

@hunterhacker BUILD SUCCESSFUL, see attached log. Obviously Ant asked my gpg passphrase to sign the artifacts.

The jdom2-2.0.6.1-maven-bundle.jar archive contains the following entries:

Archive:  jdom2-2.0.6.1-maven-bundle.jar
 Length   Method    Size  Cmpr    Date    Time   CRC-32   Name
--------  ------  ------- ---- ---------- ----- --------  ----
       0  Stored        0   0% 08/11/2021 17:57 00000000  META-INF/
     104  Defl:N       92  12% 08/11/2021 17:57 8a330166  META-INF/MANIFEST.MF
 1008525  Defl:N   966572   4% 08/11/2021 17:57 ba0e3926  jdom2-2.0.6.1-javadoc.jar
     673  Defl:N      534  21% 08/11/2021 17:57 e651ffba  jdom2-2.0.6.1-javadoc.jar.asc
  860244  Defl:N   821651   5% 08/11/2021 17:57 cb577fd1  jdom2-2.0.6.1-sources.jar
     673  Defl:N      534  21% 08/11/2021 17:57 47ac281e  jdom2-2.0.6.1-sources.jar.asc
  304947  Defl:N   282442   7% 08/11/2021 17:57 f44bbbfb  jdom2-2.0.6.1.jar
     673  Defl:N      535  21% 08/11/2021 17:57 27792dbf  jdom2-2.0.6.1.jar.asc
    4600  Defl:N     1951  58% 08/11/2021 17:57 88e41643  jdom2-2.0.6.1.pom
     673  Defl:N      533  21% 08/11/2021 17:57 63af86f9  jdom2-2.0.6.1.pom.asc
--------          -------  ---                            -------
 2181112          2074844   5%                            10 files