Restrict access to property paths on Class references

[rstoyanchev]

No description provided.

[artem-smotrakov]

Hi @rstoyanchev Does this address the recent vulnerability in Spring Framework?

Is Spring Framework 3.x vulnerable? I see the code is quite similar.

[dicer]

@artem-smotrakov See https://spring.io/blog/2022/03/31/spring-framework-rce-early-announcement

[artem-smotrakov]

Hi @dicer My question is exactly about this announcement :) I am looking for a commit that fixe this issue.

[jhoeller]

@artem-smotrakov please note that the vulnerability only materializes on JDK 9+, and Spring Framework 3.x predated JDK 9+ by several years and did not even have full JDK 8 support yet. Running such old versions of Spring on a newer JDK version is technically possible for certain scenarios, but anyone doing so is on their own in terms of support.

Also, Spring Framework 3.2.18 (the latest release before 3.x EOL) is on a December 2016 security patch level. Continued usage of that version misses out on six years of vulnerability patches and defensiveness measures that we applied to 4.x and 5.x in the meantime. A patched version for the present CachedIntrospectionResults change would cover this particular attack vector on the unsupported JDK 9+ but might still keep you exposed to other vulnerabilities even in supported areas of the framework.

For older Spring versions, we strategically suggest an upgrade to 5.3.x, or tactically a workaround as suggested in our blog post: https://spring.io/blog/2022/03/31/spring-framework-rce-early-announcement

[artem-smotrakov]

Hi @jhoeller All valid points, absolutely agree that it would be better to move to 5.3.x

If I understand correctly, this commit fixes the CVE-2022-22965. I'd appreciate if someone for the Spring team confirmed that. Thanks!

002546b

[skydreamerr]

@bclozel we started to get exceptions after the upgrade.

Caused by: org.springframework.beans.NotWritablePropertyException: Invalid property 'beanClassLoader' of bean class [org.eclipse.gemini.blueprint.service.importer.support.OsgiServiceProxyFactoryBean]: Bean property 'beanClassLoader' is not writable or has an invalid setter method. Does the parameter type of the setter match the return type of the getter?

The property has type ClassLoader:

https://github.com/eclipse/gemini.blueprint/blob/d671a74fe8aa631e8f006aad5e43d3b3f1be6359/core/src/main/java/org/eclipse/gemini/blueprint/service/importer/support/AbstractOsgiServiceImportFactoryBean.java#L215

[marcelstoer]

Hi @jhoeller All valid points, absolutely agree that it would be better to move to 5.3.x

If I understand correctly, this commit fixes the CVE-2022-22965. I'd appreciate if someone for the Spring team confirmed that. Thanks!

002546b

I fully agree. I find it irritating that the issues here have no reference to the actual commit (or the other way around). That way you have to dig through the commit history and hope the commit message is the same as or similar enough to the issue title.

I did as you did and arrived at the same commit 👍

[bclozel]

@skydreamerr please create a new issue for this.

[jhoeller]

@skydreamerr Why is that "beanClassLoader" property being populated through declarative configuration? setBeanClassLoader is a programmatic callback that the container itself calls via BeanClassLoaderAware, it's not meant to be populated explicitly. Please double-check the property configuration for that bean; maybe that explicit setting can simply be removed so that the container can perform its standard callback?

[rstoyanchev]

@marcelstoer normally commits are linked to issues. This was not a normal case.

[marcelstoer]

@marcelstoer normally commits are linked to issues. This was not a normal case.

I understand and appreciate this. Maybe just edit this issue (+ #28262) and add something like "Fixed by 002546b" to the description?

[copernico]

Hi @dicer My question is exactly about this announcement :) I am looking for a commit that fixe this issue.

Here you find the commits for both affected release series: https://github.com/SAP/project-kb/blob/vulnerability-data/statements/CVE-2022-22965/statement.yaml

(but I am not from the Spring team, so I am not providing the confirmation you were looking for :-| )

[kennymacleod]

@skydreamerr Why is that "beanClassLoader" property being populated through declarative configuration? setBeanClassLoader is a programmatic callback that the container itself calls via BeanClassLoaderAware, it's not meant to be populated explicitly. Please double-check the property configuration for that bean; maybe that explicit setting can simply be removed so that the container can perform its standard callback?

BeanClassLoaderAware is being used by the Felix OSGi framework, so I can see they may have a legitimate use for BeanClassLoaderAware. In this particular case, the fix for the CVE seems to be breaking pre-existing and valid functionality.

[bclozel]

@kennymacleod please follow up on the dedicated issue here #28269, or create a new one if you believe your case is different.
What Juergen is merely pointing out here, is that the core container will honor this contract and that property configuration is not needed for this property.

[jhoeller]

@kennymacleod I'm sure it's a valid use case for BeanClassLoaderAware itself but that container callback is meant to only be called by the container itself, not specified via declarative user configuration. That's the part I'd like to understand, there is either double configuration here - or accidental overriding since the container will perform its standard setBeanClassLoaderAware call even when user configuration was passed into the same property before. Let's find out whether this is accidental for that specific OSGi setup scenario and whether it even had the intended effect before.

That said, as per my recent comment on #28269, we are aware that this is technically a regression for isolated declarative configuration cases, as a side effect of a defensive security measure. If there are common scenarios where this regression remains unresolvable, we might specifically enable certain ClassLoader configuration scenarios again in a follow-up Spring Framework release.

[Enerccio]

Are you sure that only Java 9+ is affected. By current attack vector, sure, but couldn't there be another one? What I mean is could there be another Class.property that can lead into this attack.

[jhoeller]

To the best of our understanding, from a Spring perspective, this is indeed a Java 9+ only attack vector. It's essentially revisiting an old Class property attack vector that we closed for Java 6-8 many years ago, not having had any follow-ups in the meantime, now reopened at this late point for the unnoticed Java 9+ introduction of Class.getModule().getClassLoader().

That said, our new defensiveness checks in Spring's property descriptors cover any access to ClassLoader properties, on any JDK versions. We do recommend an upgrade to the latest framework version for defensiveness against unknown attack vectors, even on JDK 8, it is just not a necessary step to take for the present CVE with its Java 9+ attack vector.

Last but not least, since the present issue is about access to public facilities on Tomcat's ClassLoader implementation, we also strongly recommend an upgrade to the just-released Tomcat patches, in particular for attack vectors outside of Spring.

[bclozel]

@grubeninspekteur as you can imagine, this has been a stressful week for the Spring community. Discussing publicly the effectiveness of the fix and potential security issues is not the responsible way to share your concerns.

Please use the dedicated channels for that: https://spring.io/security-policy

[grubeninspekteur]

@bclozel Of course, the workload caused by the irresponsible disclosure of the vulnerability last week must have been immense and as Spring users we are grateful for your quick actions. I understand your policy of not discussing these matters publicly and have removed my comment.