Skip to content
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

BEANUTILS-520: Mitigate CVE-2014-0114 by enabling SuppressPropertiesB… #7

Merged
merged 2 commits into from
May 28, 2019
Merged

BEANUTILS-520: Mitigate CVE-2014-0114 by enabling SuppressPropertiesB… #7

merged 2 commits into from
May 28, 2019

Conversation

melloware
Copy link
Contributor

@melloware melloware commented May 23, 2019

Fixes CVE-2014-0114: https://nvd.nist.gov/vuln/detail/CVE-2014-0114

This patch by default enables the SuppressPropertiesBeanIntrospector.SUPPRESS_CLASS. So you are secure by default.

To opt-out and allow access to the "class" property making it work like BeanUtils 1.9.3 or lower simply add this one line of code to remove the feature.

final BeanUtilsBean bub = new BeanUtilsBean(); 
bub.getPropertyUtils().removeBeanIntrospector(SuppressPropertiesBeanIntrospector.SUPPRESS_CLASS);

This makes the library more secure by default and but still allows backward compatibility.

…eanIntrospector.SUPPRESS_CLASS by default.
@garydgregory
Copy link
Member

Please rebase on master and you should have a green build here, I updated JaCoCo which now gives us a green build on Java 13-EA.

@melloware
Copy link
Contributor Author

Thanks @garydgregory looks like all checks passed now!

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me too, with good and simple tests. 👍

@garydgregory garydgregory merged commit dd48f4e into apache:master May 28, 2019
@melloware melloware deleted the BEANUTILS-520 branch May 28, 2019 12:37
@Siebes
Copy link

Siebes commented Jul 16, 2019

Thank you. Will a release be occurring soon that includes this mitigation?

@melloware
Copy link
Contributor Author

@Siebes I hope so. I have been waiting for a 2.0.0 release of BeanUtils for a while.

@chtompki
Copy link
Member

Yes, I'm working towards that. Pardon my being remiss over it. I've been fairly busy lately.

@ricardovdbroek
Copy link

I noticed this fix was merged into version 1.x and is part of the 1.9.4 tag. What's usually the timeline for getting a new version in maven?

@dguiney
Copy link

dguiney commented Oct 1, 2019

I upgraded my lib to 1.9.4 and added the suggested opt-out to my code.

But PropertyUtilsBean.getSimpleProperty() is still throwing a "NoSuchMethodException" when trying to get the property of "class".

My code is

BeanMap beanMap = new BeanMap(myObject);
PropertyUtilsBean propertyUtils = new PropertyUtilsBean();
for (Object propertyNameObject : beanMap.keySet()) {
     String propertyName = (String) propertyNameObject;
     try {
          myObjPropertyValue = propertyUtils.getProperty(myObject, propertyName);
     } catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
          Errors errors = ErrorUtils.createBindException();
          errors.reject("Error occured while returing property values and propertyName: %s", propertyName);
          throw new ErrorEnabledRuntimeException(errors);
     }

@melloware
Copy link
Contributor Author

Your code is wrong. Change to..

BeanMap beanMap = new BeanMap(oldObject); 
PropertyUtilsBean propertyUtils = new PropertyUtilsBean(); 
propertyUtils.removeBeanIntrospector(SuppressPropertiesBeanIntrospector.SUPPRESS_CLASS);

for (Object propertyNameObject : beanMap.keySet()) { 
   ...
}

@dguiney
Copy link

dguiney commented Oct 1, 2019

Perfect. thanks @melloware

@dguiney
Copy link

dguiney commented Oct 1, 2019

Just to follow up. If the security violation is because of trying to access the properties of "class" . Why not exclude "class" properties when building "BeanMap"?

@melloware
Copy link
Contributor Author

I think the seucrity violation is being able to set the property using property utils. Reading it would be fine. the ability to manipulate it is what is a security issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants