Security18 hibernate validator vulnerability #7222
Conversation
…s reported to allow an attacker to escalate permissions and access private values and create invalid instances - see CVE-2017-7536. It is reported to be fixed in versions 5.2.5.Final and greater. The upgraded library had changes to the api for constructing ConstaintValidatorContextImpl, used in URLValidatorTest.java. In investigating the changes, it was found that there were further changes to the api in recent versions and it was decided to adapt the code to the latest changes and use the latest available stable hibernate-validator library - 6.1.5.Final. It was also necessary to add a dependency to javax.el due to changes in the library starting with version 5.3.1.Final and later.
…ecifically at this point)
… were getting NoClassDefFoundError: org/hibernate/validator/internal/util/CollectionHelper at runtime while creating an account
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.glassfish</groupId> | ||
| <artifactId>jakarta.el</artifactId> | ||
| <scope>provided</scope> |
There was a problem hiding this comment.
I doubt this is necessary. There isn't a single use of this package as far as I know from the codebase. Using EL in JSF usually doesn't imply adding it to the direct dependencies. Your commit message states it had been added because of the validator dependency, but the Payara BOM already should take care of that. Please try to remove and test.
There was a problem hiding this comment.
Tested this again without jakarta.el - brings back the problem it was addressing
Test set: edu.harvard.iq.dataverse.ingest.IngestUtilTest
Tests run: 12, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.121 s <<< FAILURE! - in edu.harvard.iq.dataverse.ingest.IngestUtilTest
testDirectoryLabels Time elapsed: 0.103 s <<< ERROR!
javax.validation.ValidationException: HV000183: Unable to initialize 'javax.el.ExpressionFactory'. Check that you have the EL dependencies on the classpath, or use ParameterMessageInterpolator instead
at edu.harvard.iq.dataverse.ingest.IngestUtilTest.testDirectoryLabels(IngestUtilTest.java:473)
pom.xml
Outdated
| <artifactId>hibernate-validator</artifactId> | ||
| <version>5.0.3.Final</version> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
This should remain provided or be deleted, as compile is the default.
There was a problem hiding this comment.
IMHO you should take a look at this:
We're using an internal validator class in our code and I'm not sure if this is a supported use case. This should be refactored and use Java 8 Collections API.
There was a problem hiding this comment.
BTW @kcondon I noticed you're testing on Payara 5.201. It's not important for this issue (both 5.201 up to 5.2020.4 use Hibernate Validator 6.1.2 Final), but we should make sure that testers, QA etc all use the same version referenced in pom.xml.
There was a problem hiding this comment.
@poikilotherm - I suspected that this was the reason I couldn't use provided, but refactoring the BuiltInGroupsProvider code is a separate issue IMHO
|
|
||
| assertEquals(true, new URLValidator().isValid(value, context)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testIsValidWithContextButInvalidURL() { | ||
| String value = "cnn.com"; | ||
| ConstraintValidatorContext context = new ConstraintValidatorContextImpl(null, PathImpl.createPathFromString(""), null); | ||
| ConstraintValidatorContext context = new ConstraintValidatorContextImpl(validatorFactory.getClockProvider(), PathImpl.createPathFromString(""),null, null); |
There was a problem hiding this comment.
The complete test class should be refactored to use a JUnit5 @ParameterizedTest, usable for both unit tests.
The context object should be mocked. Creating a real object is of no use here and at least on my machine creates errors. Happy to create a PR against this PR.
|
@poikilotherm I was not aware that this was tied to a specific version of Payara. I think as a policy, there needs to be more clarity on what is supported. After all, in the wild, people will use whatever generation they happen to be on, within some reasonable range, and expect it to work, without any additional instructions from us, that is. OK, will update main test box to payara-5.2020 |
What this PR does / why we need it:an attacker to escalate permissions and access private values and create invalid instances - see CVE-2017-7536. It is reported to be fixed in versions 5.2.5.Final and greater
Which issue(s) this PR closes:Investigate and Address Possible Vulnerability in org.hibernate:hibernate-validator #18
Closes #IQSS/dataverse-security#18
Special notes for your reviewer:
Suggestions on how to test this:The library upgrade is reported to be sufficient to eliminate the security vulnerability. However, it required code changes due to changes in the api and a dependency change due to a change in the initialization behavior of the library
The issue you were seeing earlier may have been due to a specified library conflicting with provide library in Payara. I had thought this was addressed due to a comment by Oliver, but somehow that change may not have gotten into the pull request. I believe it is up to date now and the hibernate validator pom entry references the Payara provided version. org.hibernate.validator.internal.util.CollectionHelper is provided by Payara
Does this PR introduce a user interface change? If mockups are available, please link/include them here:no
Is there a release notes update needed for this change?:no
Additional documentation: