Using unsafe Jackson deserialization configuration is security-sensitive

[chenrujun]

Context

Code analysis tool reported a problem about spring-security.
Related Code:

https://github.com/spring-projects/spring-security/blob/5.6.2/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/jackson2/ClientRegistrationMixin.java#L36-L43

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)
@JsonDeserialize(using = ClientRegistrationDeserializer.class)
@JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.ANY, getterVisibility = JsonAutoDetect.Visibility.NONE,
		isGetterVisibility = JsonAutoDetect.Visibility.NONE)
@JsonIgnoreProperties(ignoreUnknown = true)
abstract class ClientRegistrationMixin {

}

Problem

or when the annotation @JsonTypeInfo is set at class, interface or field levels and configured with use = JsonTypeInfo.Id.CLASS or use = Id.MINIMAL_CLASS.

Refs: https://rules.sonarsource.com/java/RSPEC-4544

[nor-ek]

Hi @eleftherias , I can take this if @rwinch is overloaded :)
But tell me if I'm thinking correctly. We should use @JsonTypeInfo(use = Id.NAME)? I've also noticed that there are many occurrences of JsonTypeInfo.Id.CLASS. You can see below most common usages:

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY) 
@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "@class")

Should I fix this and choose one common usage?

[chenrujun]

We should use @JsonTypeInfo(use = Id.NAME)

I have a concern of doing like this: If customer has 2 replicas of his application: application-1 and application-2, they share the session but use different spring-security version(canary release). The ClientRegistration saved in somewhere like Redis, applcation-1 save ClientRegistration using @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS), then application-2 can not load ClientRegistration using @JsonTypeInfo(use = Id.NAME).

[nor-ek]

As I can see they should rely on @JsonTypeName and @JsonSubTypes. in such situation. In my opinion this change needs to be marked as breaking change and properly described (tbh I don't know how such situations are handled in spring-security deployment.

[rwinch]

A few things:

1. The exploit must happen when untrusted users can provide the arbitrary JSON to be deserialized. In a typical application, the Jackson Support is only going to be used on values being serialized/deserialized to session. If a malicious user can write arbitrary data to the session store, then they can already bypass authentication/authorization so it is reasonable to presume that session storage is secured. If a user leverages the Jackson support in another way, then they are responsible for ensuring they trust the data being deserialized. This is the same for classes marked at Serialized. Users are responsible for ensuring that they do not deserialize untrusted data.

2. By default, Spring Security's Jackson Support uses an allow list of types that can be deserialized. Users can expand this by explicitly marking them as allowed. However, if users opt into allowing additional classes they are responsible for ensuring it should be allowed. See Add Allow List to Jackson Support #4370

Given the information above this is not a priority for the Security team at this time. However, if someone is interested in submitting a PR to change this behavior, we can consider it for Spring Security 6 as a breaking change. If someone is interested, the best place to start is to say you would like to work on the issue. Then put together a draft PR with changes to a single class. Please keep in mind that existing tests should pass except for changing the type information to be a name instead of a class.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[spring-projects-issues]

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.