-
Notifications
You must be signed in to change notification settings - Fork 118
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
Got security warning on JDom2 2.0.6: CVE-2021-33813 #189
Comments
A pull request is already created: #188 |
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. |
Any news on this? Did you accept the pull request and did you manage to push to maven? |
I accepted. I don't have the maven credentials since that was Rolf's business. |
Ok. Nice. So, when can we expect a release? |
Maybe a non-maven for now? |
@hunterhacker any idea on when we could see an official Maven release with this fix? |
@hunterhacker Also interested in a Maven release date as soon as possible. Thanks! |
@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. |
@hunterhacker we are waiting for a maven release as well, thanks in advance! |
Hope you can consider the super simple PR #185 as part of the release. It would make things easier for projects targeting JDK17. |
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. |
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. |
Another ten days later still no release. |
when can we expect a release with this security fix?? |
I am interested in an update with this security fix as well. Any indication when a release could be coming? |
I am also interested in this security fix. When this fix will be released? |
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. |
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 So the library never prevented anyone from blocking off XXE. One just needed to do it via the So it's a little confusing to me. Seems the earlier OWASP recommendation wasn't tested/verified!? jdom/core/src/java/org/jdom2/input/SAXBuilder.java Lines 778 to 783 in 03a391a
If I read correctly, the current plan/merged PR is still dependent on people disabling the functionality with either the builder or the My broad summary (please correct me if I am wrong) is
Am I missing something? I am curious how many real world usages are in the Nevertheless, if we look at this CVE more as
with the broad class of
that would seem more accurate to me than
... which does not seem an accurate description of the problem to me, and assumes specific usage context. |
@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 |
@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? |
@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 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. |
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. |
I wonder if it's possible that adding a suffix of Alternatively, Snyk doesn't consider any version fixed right now based on vulnerable versions being listed as The fact that this ticket is not closed yet possibly won't help either. OWASP Dependency Check, based on NVD/NIST data does seem to automatically consider Vulnerable
OK
|
Now Snyk reports v2.0.6.1 as NOT vulnerable. |
Any plans to have a release to Maven central? Thanks |
@ottlinger the 2.0.6.1 release shows up on maven central for me: https://search.maven.org/artifact/org.jdom/jdom2/2.0.6.1/jar |
@jamesagnew not sure how long a sync takes, but https://mvnrepository.com/artifact/org.jdom/jdom does not show the newest version yet and I'm unable to fetch with Maven default settings:
|
@ottlinger Thats because you are looking at the wrong artifact. https://mvnrepository.com/artifact/org.jdom/jdom2 |
Hello, Are there any plans to port the fix for CVE-2021-33813 to jdom v1.x? We're experiencing issues with Apache Commons JXPath, which is not compatible with jdom v2.x.. |
FWIW the maven-release-plugin still uses JDom v1 (which may cause some strange line ending issues during release https://issues.apache.org/jira/browse/MRELEASE-1025 ) (But maven-release-plugin would IMO be better off replacing JDom 1 with JDom 2, rather than getting a JDom 1 with the security issue fixed... since that would make it possible to fix MRELEASE-1025) |
No current plans. If you're worried about entity expansion, call builder.setExpandEntities(false). The only actual concern is if you (a) intend not to expand entities, (b) don't use the explicit JDOM flag to turn this off but instead (c) directly tell the underlying parser to turn them off, so then (d) you're surprised that your call to the parser didn't apply because JDOM supersedes your complex direct call with its own simple explicit flag setting. The challenge is once the CVE is out there things are tainted up the chain. I recognize that. And sigh, maybe you can convince me it's worth a chunk of my time. I should really charge support for fixing bugs in software I released 10+ years ago. "The first 10 years are free!" :) |
Thanks, @hunterhacker! Well v1.1.3 was released Feb, 2012, so we have some time before the support becomes paid :) Sorry to bother you again and sorry for taking some of your time! If it's not a lot of work to backport the fix to v1.x, would you consider doing it? Thanks & Regards, |
NexusIQ now have the following possible mitigation on its JDOM2 2.0.6 critical warning:
I.e. this issue is now fixed as far as NexusIQ is concerned. |
It's almost a year passed since the issue started. Wonder if there will be a formal release or not. |
@chadlwilson I opened this issue, and I'm good with closing this as fixed by 2.0.6.1 now (in case someone was waiting for my input?) |
Summing up this thread
@hunterhacker could you close this one please? :) |
i also met |
About:
@chadlwilson Thanks for trying to summarize it! However, does it mean that https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#saxbuilder is not correct when explaining that doing the following is enough to protect from XXE attacks:
This code doesn't use either Thanks |
@vmassol It looks like that change was made 4 months ago in OWASP/CheatSheetSeries@c63fdaf as part of OWASP/CheatSheetSeries#966 so you might want to take it up over there in OWASP land. Before Feb 2023 it used to look like https://github.com/OWASP/CheatSheetSeries/blob/ef4beb83b59d474d02ec05afa7d79efe554dc3e8/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.md#saxbuilder which is rather different. |
@chadlwilson good point, thanks for your reply! What they wrote is probably right but it would have been nice to get a confirmation from one of the devs of JDOM2 (I'd expect that before writing the instructions they validated them with the JDOM2 project). |
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.
The text was updated successfully, but these errors were encountered: