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

Restrict access to property paths on Class references #28261

Closed
rstoyanchev opened this issue Mar 31, 2022 · 20 comments
Closed

Restrict access to property paths on Class references #28261

rstoyanchev opened this issue Mar 31, 2022 · 20 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches
Milestone

Comments

@rstoyanchev
Copy link
Contributor

No description provided.

@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Mar 31, 2022
@rstoyanchev rstoyanchev added this to the 5.3.18 milestone Mar 31, 2022
@jhoeller jhoeller added the status: backported An issue that has been backported to maintenance branches label Mar 31, 2022
@artem-smotrakov
Copy link

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

dicer commented Mar 31, 2022

@artem-smotrakov
Copy link

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

@jhoeller
Copy link
Contributor

@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
Copy link

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

skydreamerr commented Apr 1, 2022

@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
Copy link

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
Copy link
Member

bclozel commented Apr 1, 2022

@skydreamerr please create a new issue for this.

@jhoeller
Copy link
Contributor

jhoeller commented Apr 1, 2022

@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
Copy link
Contributor Author

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

@marcelstoer
Copy link

marcelstoer commented Apr 1, 2022

@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
Copy link

copernico commented Apr 1, 2022

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

@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
Copy link
Member

bclozel commented Apr 1, 2022

@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
Copy link
Contributor

jhoeller commented Apr 1, 2022

@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
Copy link

Enerccio commented Apr 1, 2022

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
Copy link
Contributor

jhoeller commented Apr 1, 2022

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
Copy link
Member

bclozel commented Apr 4, 2022

@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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches
Projects
None yet
Development

No branches or pull requests