Skip to content

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

Merged
garydgregory merged 2 commits into
apache:masterfrom
melloware:BEANUTILS-520
May 28, 2019
Merged

BEANUTILS-520: Mitigate CVE-2014-0114 by enabling SuppressPropertiesB…#7
garydgregory merged 2 commits into
apache:masterfrom
melloware:BEANUTILS-520

Conversation

@melloware

@melloware melloware commented May 23, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor Author

Thanks @garydgregory looks like all checks passed now!

@kinow kinow left a comment

Copy link
Copy Markdown
Member

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

Siebes commented Jul 16, 2019

Copy link
Copy Markdown

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

@melloware

Copy link
Copy Markdown
Contributor Author

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

@chtompki

Copy link
Copy Markdown
Member

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

@ricardovdbroek

Copy link
Copy Markdown

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

dguiney commented Oct 1, 2019

Copy link
Copy Markdown

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
Copy Markdown
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

dguiney commented Oct 1, 2019

Copy link
Copy Markdown

Perfect. thanks @melloware

@dguiney

dguiney commented Oct 1, 2019

Copy link
Copy Markdown

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
Copy Markdown
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