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

SEC-2137: Session fixation protection cannot be disabled when concurrent session control is enabled #2363

Closed
spring-issuemaster opened this issue Feb 26, 2013 · 12 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link

commented Feb 26, 2013

Nick Williams (Migrated from SEC-2137) said:

Related to: SEC-2002
Blocked by: SEC-2135

Please see the following snippet from our application's security config:

...
    <session-management invalid-session-url="/web/login/timedOut"
                        session-fixation-protection="none"
                        session-authentication-error-url="">
        <concurrency-control expired-url="/web/login/expired"
                             max-sessions="5"
                             error-if-maximum-exceeded="false"
                             session-registry-alias="sessionRegistry" />
    </session-management>
...

The presence of <concurrency-control> causes a ConcurrentSessionControlStrategy to be created. Since this extends SessionFixationProtectionStrategy it forces the enabling of session migration, even if session fixation protection is set to "none". I have verified this by looking at the code. This is pretty big problem, and the documentation certainly does not indicate that this was intentional.

A change to how this whole system works was proposed in SEC-2135. The resolution for that enhancement request will also fix this bug. However, the bug probably needs to exist for tracking and historical purposes.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jun 14, 2013

Nick Williams said:

I have begun work on this in https://github.com/beamerblvd/spring-security/tree/SEC-2135. (Fixing SEC-2135 will, by consequence, fix this as well.)

It doesn't appear that I have the ability to change the status of this to "In Progress." If it matters to anyone, feel free to change the status.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jun 14, 2013

Nick Williams said:

Pull request 35 submitted to fix this new bug.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jun 17, 2013

Rob Winch said:

Thank you for the pull request. I will take a look at this later this week (likely on Thursday)

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jul 25, 2013

Nick Williams said:

I see you've updated the issue. Does this mean you're ready to proceed on the pull request for this and SEC-2002 again? I'd love to move it forward with whatever changes are necessary.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jul 25, 2013

Rob Winch said:

I wonder if you meant SEC-2135 since SEC-2002 was added to 3.2.0.M2?

I am revisiting SEC-2135 and SEC-2137 so we can get them into 3.2.0.RC1. As the pull request stands we need to make the following changes for these two issues:

  • We should have two distinct commits for SEC-2135 and SEC-2137
    ** The SEC-2137 commit should not be based on SEC-2135
    ** These points are important as there is a good chance we will need to only merge SEC-2137 into 3.1.x since it is a bug
  • I may have missed it, but we should have a test for SEC-2137 to ensure this is fixed and does not happen again
  • We should remove SessionFixationProtectionScheme and place the logic for session fixation behind an interface. I know you said that SessionFixationProtectionScheme "is OUR implementation". However, we need to ensure that our implementations are flexible since anything that is public scope becomes our consumer's implementations too. Consider if we needed to add an implementation that requires some sort of configured state (i.e. migrate only a specific set of session attributes). This would be difficult to do under the current implementation since users could not create an enum with the attributes they want to migrate.
  • Our concurrency control implementation should not extend from our session fixation protection implementation.
    ** This change can be seen when considering the "is-a" relationship. Concurrency control is not a form of session fixation protection, so concurrency control should not extend session fixation protection.
    ** Another reason is inheritance means it would be difficult for a user combine the concurrency control with their own implementations of SessionAuthenticationStrategy.
  • As mentioned in some of the pull request comments, we want to ensure our configuration and implementation logic are kept separate. For example, keep BeanDefinitionValidationException limited to the spring-security-config jar, remove methods like SessionFixationProtectionScheme.getFromNamespaceAttribute from the web jar, etc.
  • Both issues need integrated into the Java Config solution

As it stands the design for SEC-2137 can be accomplished with:

  • Create a CompositeSessionAuthenticationStrategy that delegates to any number of SessionAuthenticationStrategy implementations
  • Deprecate ConcurrentSessionControlStrategy and replace with a new ConcurrentSessionControlAuthenticationStrategy
    ** extends Object (nothing else)
    ** only does concurrency control related functions
  • We can continue using SessionAuthenticationStrategy as is for now
  • Create RegisterSessionAuthenticationStrategy - this only registers the sessions
  • For concurrency and session fixation create CompositeSessionAuthenticationStrategy with the following delegates: ConcurrentSessionControlAuthenticationStrategy, SessionAuthenticationStrategy, RegisterSessionAuthenticationStrategy
  • For concurrency control with no session fixation create CompositeSessionAuthenticationStrategy with the following delegates: ConcurrentSessionControlAuthenticationStrategy, RegisterSessionAuthenticationStrategy
  • Breaking the registration of the session into its own class has an advantage that users can easily register sessions and use the session registry for administrative purposes. It also keeps our code simple. It does have the disadvantage of more difficult configuration, but the namespace and java configuration will ensure this is kept simple for most users.

For SEC-2135:

  • Create a base class that is package scope for SessionAuthenticationStrategy (i.e. AbstractSessionAuthenticationStrategy)
  • make SessionAuthenticationStrategy a subclass that can invoke the new session or migrate strategies. This essentially keeps SessionAuthenticationStrategy just as is except most logic moves to the base class
  • make another subclass for ChangeSessionIdAuthenticationSessionStrategy
  • We don't need anything for session-fixation="none" since we can simply omit it in CompositeSessionAuthenticationStrategy or use the existing NullAuthenticatedSessionStrategy.

Please let me know if you plan to make these changes or if you would prefer I do it. The goal is to have RC1 out by mid Aug and I would really like to ensure these changes get in.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jul 25, 2013

Nick Williams said:

Yes, I meant SEC-2135. My bad. I don't think that's the first time I've gotten those mixed up, either.

First, some questions.

This sounds like a big enough change from how I've currently done it that I'm better off simply deleting my branch, deleting the pull request, and starting over again (particularly now that the Java Config stuff is merged). Do you concur, or do you see some way to salvage this?

You say, "Both issues need integrated into the Java Config solution," and I agree. However, in Spring Framework I would expect this to work something like this (because this is how so many other similar features are configured in Spring Framework):

@Configuration
...
@EnableSessionFixationProtection(scheme = SessionFixationProtectionScheme.CHANGE_SESSION_ID)
@EnableConcurrentSessionControl(more settings)
public class MyConfig
{
    ...
}

However, such an approach would conflict with your saying:

We should remove SessionFixationProtectionScheme and place the logic for session fixation behind an interface. I know you said that SessionFixationProtectionScheme "is OUR implementation". However, we need to ensure that our implementations are flexible since anything that is public scope becomes our consumer's implementations too.

It would seem to me that the Java configuration should, if @EnableWebSecurity is present, find all of the configured beans implementing SessionAuthenticationStrategy and put them together in the composite strategy you suggested. If @EnableSessionFixationProtection is present, it would register OUR default session fixation strategy using the Scheme enum (though the implementation can certainly be removed from the enum). If the user didn't like our default session fixation protection strategy, they could simply create their own SessionAuthenticationStrategy implementation.

Thoughts?

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Jul 25, 2013

Rob Winch said:

My bad. I don't think that's the first time I've gotten those mixed up, either.

No problem...I often times mix up the JIRA ids myself

This sounds like a big enough change from how I've currently done it that I'm better off simply deleting my branch, deleting the pull request, and starting over again (particularly now that the Java Config stuff is merged). Do you concur, or do you see some way to salvage this?

I agree...you are probably better off starting from scratch. You may keep your local branch as some of the logic may be easy to copy paste into the new implementations.

However, in Spring Framework I would expect this to work something like this (because this is how so many other similar features are configured in Spring Framework):

I can see how you might think this. However, Spring Security cannot just wire all instances because it needs to know which element it should wire to. That is why Spring Security uses configure(HttpSecurity).

Even if there was EnableSessionFixationProtection and SessionFixationProtectionScheme, we would not want our logic embedded into the Enum (it would just be used for configuration). Notice that EnableTransactionManagement uses AdviceMode enum and AdviceMode has no logic in it. This keeps our implementations distinct from our configuration options.

If the user didn't like our default session fixation protection strategy, they could simply create their own SessionAuthenticationStrategy implementation.

The issue is that if Spring Security needed to add another type of session fixation (like it is today) that contained state, it would not work well with this design. Spring Security may need to add additional implementations that need to reuse pieces of the logic in the enums and contain state. Since the proposed implementations are enums this would not work very well. We would not be able to remove this code either because it is public.

PS: If you want to see how it relates to other Java Configs here is a snippet of the relations:

  • Spring Web MVC uses @EnableWebMvc to add DelegatingWebMvcConfiguration in the same way that Spring Security uses @EnableWebSecurity to add WebSecurityConfiguration.
  • Spring Web MVC's DelegatingWebMvcConfiguration gathers all the WebMvcConfigurer implementations similar to how Spring Security's WebSecurityConfiguration gathers all the SecurityConfigurer<Filter, WebSecurity> instances

That is where the similarities really end because Spring Security can have multiple elements where as Spring MVC only has a single DispatcherServlet to configure. So @autowire is not an option for how to customize Spring Security's WebSecurityConfigurerAdapter. Instead, they are specified by invoking methods within configure(HttpSecurity http).

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 3, 2013

Nick Williams said:

Okay. As much as I'd love to contribute the fixes for SEC-2135 and SEC-2137, I spent several days on the original work and I just don't have those days anymore. I'm three weeks behind on the book and I'm trying to help the community resolve a critical problem with Java 8 that is affecting Log4j/Logback/Groovy. It would be September before I had any time for this, and that would mean this couldn't happen for 3.2.0.RC1. I need this to happen for 3.2.0.RC1 for other reasons (such as wanting to use the new feature in SEC-2135 in the book).

So, can you take care of these? Of course, use any/all code that I've already wrote to help you along. I just can't make it happen in time for SEC-2135.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 5, 2013

Rob Winch said:

Thanks for responding Nick. I should be able to address this for RC1. Thanks again :)

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 5, 2013

Nick Williams said:

Will you be able to address both SEC-2135 and SEC-2137? What is your expected ETA for RC1?

Thanks!

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 5, 2013

Rob Winch said:

I should be able to address both JIRAs for RC1 with an ETA of 16/Aug/13.

NOTE: While we do take an agile approach (issues may be cut to meet deadlines or deadlines may be pushed to get important issues), JIRA is always the most up to date snapshot of what is known. So in this instance, both JIRAs are scheduled for RC1 which is scheduled for 16/Aug/13. In the event you were not aware the due date is visible by selecting a fix version (i.e. https://jira.springsource.org/browse/SEC/fixforversion/13903 ) You can even view burn-downs to see how "on track" everything is.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 5, 2013

Nick Williams said:

Yes, I'm quite used to seeing the version due dates in JIRA. However, in my experience (at least with Spring Framework) the version has usually released between 1 week and 1 month after the due date. That has been the case with the Spring 4.0 milestones at the very least. My purpose in asking was to find out if you anticipated releasing some time after that due date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.