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

TRUNK-6041: Added csrf Token to user session #173

Closed
wants to merge 1 commit into from

Conversation

jnsereko
Copy link
Contributor

@jnsereko jnsereko commented Nov 12, 2021

Attending to the CSRF token issue in reference application.
I think we give a session a salt and a boolean to verify if it is still valid or not.

Each time a session is created, the SessionListener#sessionCreated(...) is called.
We use this time to append create a new salt cache if it wasn't in existence before.

Each a session is deleted, the SessionListener#sessionDestroyed(...) is called.
We use this time to append clear the cache.

In my thinking, this would be better that creating a filter that creates a new salt every time a request is made. For a big system like OpenMRS, this would cause some delay.

Whats left to work..

In the login servlet, either in this repository or in reference application

Cache<String, Boolean> csrfPreventionSaltCache = (Cache<String, Boolean>) httpSessionEvent.getSession()
    .getAttribute("csrfPreventionSaltCache");
    
String salt = csrfPreventionSaltCache.get()
httpReq.setAttribute("csrfPreventionSalt", salt);

Register a Filter

public class ValidateSalt implements Filter  {

    @Override
    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
        throws IOException, ServletException {

        // Assume its HTTP
        HttpServletRequest httpReq = (HttpServletRequest) request;

        // Get the salt sent with the request
        String salt = (String) httpReq.getParameter("csrfPreventionSalt");

        // Validate that the salt is in the cache
        Cache<String, Boolean> csrfPreventionSaltCache = (Cache<String, Boolean>)
            httpReq.getSession().getAttribute("csrfPreventionSaltCache");

        if (csrfPreventionSaltCache != null &&
                salt != null &&
                csrfPreventionSaltCache.getIfPresent(salt) != null){

            // If the salt is in the cache, we move on
            chain.doFilter(request, response);
        } else {
            // return 403 forbidden operation
            
        }
    }

    @Override
    public void init(FilterConfig filterConfig) throws ServletException {
    }

    @Override
    public void destroy() {
    }
}

Configure a filter in the servlet-context

...
<filter>
    <filter-name>${project.parent.artifactId}ValidateSalt</filter-name>
    <filter-class>${project.parent.groupId}.${project.parent.artifactId}.filter.ValidateSalt</filter-class>
</filter>
...
<filter-mapping>
    <filter-name>validateSalt</filter-name>
    <url-pattern>/accounts/</url-pattern>
</filter-mapping>
...

in forms that are worth

...
<form action="/transferMoneyServlet" method="get">
    <input type="hidden" name="csrfPreventionSalt" value="<c:out value='${csrfPreventionSalt}'/>"/>
    ...
</form>
...

Issue Link: https://issues.openmrs.org/browse/TRUNK-6041
Disscussion: https://talk.openmrs.org/t/security-vulnerability-in-openmrs-2-4-0/34879/8

I am open for any kind of discussion on this. Thank you
cc @isears @ibacher @dkayiwa @sherrif10 @ (Sarah E Elder)

@dkayiwa
Copy link
Member

dkayiwa commented Nov 23, 2021

@jnsereko instead of reinventing the wheel, wouldn't it be better to reuse something like this? https://owasp.org/www-project-csrfguard/

@jnsereko
Copy link
Contributor Author

Wow! @dkayiwa
I have never heard about this tool, but I am going to give it a try and then share the feedback.
From its description, it's like it's a nice library.

@dkayiwa dkayiwa closed this Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants