Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 5.1.1
    • Fix Version/s: 5.1.2
    • Labels:
      None
    • Environment:
      WildFly 8.2.0.Final

      Description

      Roller continues using previous salt value which sent from client as POST parameter. this leads fixing of salt value in the form element of html, and brings ServletException("Security Violation") by ValidateSaltFilter at some use cases (e.g. long-term editing over 60 minutes) unexpectedly.

      Seems to that the cause is existence of org.apache.roller.weblogger.ui.struts2.util.UIAction#setSalt(String) method. this overwrites salt with previous value which sent by client as POST parameter. it's unnecessary behavior because new salt value comes through preceding invocation of UIAction#setRequest(Map).

      Original discussion in the mailing list:
      http://markmail.org/search/?q=list%3Aorg.apache.roller.user#query:list%3Aorg.apache.roller.user+page:1+mid:tnqn4qjuwmwun4oh+state:results

      1. ROL-2058.patch
        0.7 kB
        Kohei Nozaki

        Activity

        Hide
        djohnson David Johnson added a comment -

        Thanks for finding and fixing this Kohei!

        Author: snoopdave
        Date: Sun Mar 1 14:45:17 2015
        New Revision: 1663125

        URL: http://svn.apache.org/r1663125
        Log: ROL-2058 no salt renewal on request

        Show
        djohnson David Johnson added a comment - Thanks for finding and fixing this Kohei! Author: snoopdave Date: Sun Mar 1 14:45:17 2015 New Revision: 1663125 URL: http://svn.apache.org/r1663125 Log: ROL-2058 no salt renewal on request
        Hide
        ghuber Greg Huber added a comment -

        We are now get this error:

        Error setting expression 'salt' with value ['ZzHQrZiPk1ZJW5mBt7V4', ]

        ie it is missing a setSalt(..), might need a no-opp method on UIAction.

        btw the salt value will still timeout after:

        cache.salt.timeout=3600

        1 hour, so the problem will still exist. May want to increase this.

        Show
        ghuber Greg Huber added a comment - We are now get this error: Error setting expression 'salt' with value ['ZzHQrZiPk1ZJW5mBt7V4', ] ie it is missing a setSalt(..), might need a no-opp method on UIAction. btw the salt value will still timeout after: cache.salt.timeout=3600 1 hour, so the problem will still exist. May want to increase this.
        Hide
        xkylex Kohei Nozaki added a comment -

        The cause of that error is the definition of struts.devMode=true in struts.properties (http://stackoverflow.com/questions/21018018/unexpected-exception-caught-setting-xxx-on-class-xxx-error-setting-expressio). I've never set it to true so I haven't noticed that. is there any bad effect on this?

        IMHO 1 hour timeout is good enough because now the clock restarts at every page transition by this fix. is there still any problematic situation?

        Show
        xkylex Kohei Nozaki added a comment - The cause of that error is the definition of struts.devMode=true in struts.properties ( http://stackoverflow.com/questions/21018018/unexpected-exception-caught-setting-xxx-on-class-xxx-error-setting-expressio ). I've never set it to true so I haven't noticed that. is there any bad effect on this? IMHO 1 hour timeout is good enough because now the clock restarts at every page transition by this fix. is there still any problematic situation?
        Hide
        ghuber Greg Huber added a comment - - edited

        I would say we need the no-opp setSalt(..) for completeness (messages are there for a reason). The timeout is personal I guess.

        I never really like the way the salt csrf works here for struts UI as its leads to problems of saving (restart the server and the salt cache is gone). I prefer to use an interceptor instead of the filter, and then return back to the action rather than the general exception page on any salt issue. It is then possible to resubmit the page with a new salt value.

        <interceptor-ref name="UIActionSaltInterceptor" >
        <param name="excludeMethods">*</param>
        <param name="includeMethods">save</param>
        </interceptor-ref>

        public class UIActionSaltInterceptor extends MethodFilterInterceptor {
        
        	private static final Logger log = LoggerFactory
        			.getLogger(UIActionSaltInterceptor.class);
        
        	private String inputResultName = Action.INPUT;
        
        	/**
        	 * Set the <code>inputResultName</code> (result name to be returned when
        	 * action fails the salt check). Default to {@link Action#INPUT}
        	 * 
        	 * struts.xml interceptor parameter:
        	 * 
        	 * <param name="inputResultName">input</param>
        	 * 
        	 * @param inputResultName
        	 *            what result name to use when there is a salt error.
        	 */
        	public void setInputResultName(String inputResultName) {
        		this.inputResultName = inputResultName;
        	}
        
        	/**
        	 * Intercept {@link ActionInvocation} and returns a
        	 * <code>inputResultName</code> when action fails the salt check.
        	 * 
        	 * @return String result name
        	 */
        	@Override
        	protected String doIntercept(ActionInvocation invocation) throws Exception {
        		Object action = invocation.getAction();
        
        		if (action instanceof UIAction) {
        
        			UIAction theAction = (UIAction) action;
        
        			final ActionContext context = invocation.getInvocationContext();
        			HttpServletRequest request = (HttpServletRequest) context
        					.get(ServletActionContext.HTTP_REQUEST);
        
        			// Check post
        			if (("POST").equals(request.getMethod())) {
        
        				SaltCache saltCache = SaltCache.getInstance();
        				if (saltCache.isCacheEnabled()) {
        
        					String salt = (String) request.getParameter("salt");
        
        					if (salt == null || saltCache.get(salt) == null
        							|| saltCache.get(salt).equals(false)) {
        
        						if (log.isDebugEnabled())
        							log.debug("Failed salt check on action "
        									+ theAction
        									+ ", returning result name 'input'");
        
        						// Indicate the error to the user
        						theAction.addError("error.permissions.deniedSalt");
        
        						return inputResultName;
        
        					}
        
        					// Cleanup
        					saltCache.remove(salt);
        				}
        			}
        		}
        
        		return invocation.invoke();
        	}
        } 
        
        Show
        ghuber Greg Huber added a comment - - edited I would say we need the no-opp setSalt(..) for completeness (messages are there for a reason). The timeout is personal I guess. I never really like the way the salt csrf works here for struts UI as its leads to problems of saving (restart the server and the salt cache is gone). I prefer to use an interceptor instead of the filter, and then return back to the action rather than the general exception page on any salt issue. It is then possible to resubmit the page with a new salt value. <interceptor-ref name="UIActionSaltInterceptor" > <param name="excludeMethods">*</param> <param name="includeMethods">save</param> </interceptor-ref> public class UIActionSaltInterceptor extends MethodFilterInterceptor { private static final Logger log = LoggerFactory .getLogger(UIActionSaltInterceptor.class); private String inputResultName = Action.INPUT; /** * Set the <code>inputResultName</code> (result name to be returned when * action fails the salt check). Default to {@link Action#INPUT} * * struts.xml interceptor parameter: * * <param name= "inputResultName" >input</param> * * @param inputResultName * what result name to use when there is a salt error. */ public void setInputResultName( String inputResultName) { this .inputResultName = inputResultName; } /** * Intercept {@link ActionInvocation} and returns a * <code>inputResultName</code> when action fails the salt check. * * @ return String result name */ @Override protected String doIntercept(ActionInvocation invocation) throws Exception { Object action = invocation.getAction(); if (action instanceof UIAction) { UIAction theAction = (UIAction) action; final ActionContext context = invocation.getInvocationContext(); HttpServletRequest request = (HttpServletRequest) context .get(ServletActionContext.HTTP_REQUEST); // Check post if (( "POST" ).equals(request.getMethod())) { SaltCache saltCache = SaltCache.getInstance(); if (saltCache.isCacheEnabled()) { String salt = ( String ) request.getParameter( "salt" ); if (salt == null || saltCache.get(salt) == null || saltCache.get(salt).equals( false )) { if (log.isDebugEnabled()) log.debug( "Failed salt check on action " + theAction + ", returning result name 'input'" ); // Indicate the error to the user theAction.addError( "error.permissions.deniedSalt" ); return inputResultName; } // Cleanup saltCache.remove(salt); } } } return invocation.invoke(); } }
        Hide
        xkylex Kohei Nozaki added a comment -

        I created a couple of new issues so that we can talk about it further:

        ROL-2068 - Error setting expression 'salt' with value ['...', ] in struts.devMode=true
        ROL-2069 - Improvement of salt processing

        As to ROL-2068 I believe we can just add an no-op method, but as to ROL-2069 I think we need to talk some about it further.

        Show
        xkylex Kohei Nozaki added a comment - I created a couple of new issues so that we can talk about it further: ROL-2068 - Error setting expression 'salt' with value ['...', ] in struts.devMode=true ROL-2069 - Improvement of salt processing As to ROL-2068 I believe we can just add an no-op method, but as to ROL-2069 I think we need to talk some about it further.
        Hide
        djohnson David Johnson added a comment -

        5.1.2 released

        Show
        djohnson David Johnson added a comment - 5.1.2 released

          People

          • Assignee:
            djohnson David Johnson
            Reporter:
            xkylex Kohei Nozaki
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development