Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4171

getText methods are not documented as evaluating OGNL

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.15.1
    • Fix Version/s: 2.5.8
    • Component/s: Documentation
    • Labels:
    • Flags:
      Important

      Description

      The methods below evaluate OGNL as their first parameter. However they are not documented as evaluating OGNL. We have observed this occurring in one project and are contacting the affected vendors.

      com.opensymphony.xwork2.TextProviderSupport.getText(String, String[])
      com.opensymphony.xwork2.TextProviderSupport.getText(String, List<?>)
      com.opensymphony.xwork2.TextProviderSupport.getText(String)

      These methods are then used by ActionSupport (via its getText methods). None of these methods are documented as evaluating OGNL either.

      This issue is recommending that all of these methods are documented as evaluating OGNL since this may come as a surprise to some developers.

        Issue Links

          Activity

          Hide
          lukaszlenart Lukasz Lenart added a comment - - edited

          I think you a bit demonise here. The first parameter isn't evaluated as an OGNL expression - is just a key used to look up in a resource bundle:

          LocalizedTextUtil, line 683
          String message = TextParseUtil.translateVariables(bundle.getString(key), valueStack);
          

          and just the value from resource bundle is evaluated as an expression which is rather obvious if you have something like this in a properties file

          package.properties
          requiredstring = ${getText(fieldName)} is required.
          

          http://struts.apache.org/development/2.x/docs/localizing-output.html

          I have added a note about evaluation to the docs

          https://cwiki.apache.org/confluence/display/WW/Localization#Localization-Examples

          Show
          lukaszlenart Lukasz Lenart added a comment - - edited I think you a bit demonise here. The first parameter isn't evaluated as an OGNL expression - is just a key used to look up in a resource bundle: LocalizedTextUtil, line 683 String message = TextParseUtil.translateVariables(bundle.getString(key), valueStack); and just the value from resource bundle is evaluated as an expression which is rather obvious if you have something like this in a properties file package.properties requiredstring = ${getText(fieldName)} is required. http://struts.apache.org/development/2.x/docs/localizing-output.html I have added a note about evaluation to the docs https://cwiki.apache.org/confluence/display/WW/Localization#Localization-Examples
          Hide
          coverity_srl Coverity Security Research Laboratory added a comment -

          Lukasz,

          I ran the sample tutorial application and modified the HelloWorld.java as such:

              public String execute() throws Exception {
                  setMessage(getText(getMessage()));
                  return SUCCESS;
              }
          

          And here's the current stack when debugging the tutorial under Eclipse via this URL:

          http://127.0.0.1:8080/tutorial/example/HelloWorld.action?message=${2*3}
          Daemon Thread [http-8080-1] (Suspended (entry into method translateVariables in TextParseUtil))	
          	TextParseUtil.translateVariables(char[], String, ValueStack, Class, TextParseUtil$ParsedValueEvaluator, int) line: 156	
          	TextParseUtil.translateVariables(char[], String, ValueStack, Class, TextParseUtil$ParsedValueEvaluator) line: 127	
          	TextParseUtil.translateVariables(String, ValueStack) line: 49	
          	LocalizedTextUtil.getDefaultMessage(String, Locale, ValueStack, Object[], String) line: 663	
          	LocalizedTextUtil.findText(Class, String, Locale, String, Object[], ValueStack) line: 534	
          	LocalizedTextUtil.findText(Class, String, Locale, String, Object[]) line: 362	
          	TextProviderSupport.getText(String, String, List<?>) line: 208	
          	TextProviderSupport.getText(String) line: 123	
          	HelloWorld(ActionSupport).getText(String) line: 103	
          	HelloWorld.execute() line: 30	
          	NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]	
          	NativeMethodAccessorImpl.invoke(Object, Object[]) line: 57	
          	DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43	
          	Method.invoke(Object, Object...) line: 601	
          	DefaultActionInvocation.invokeAction(Object, ActionConfig) line: 450	
          	DefaultActionInvocation.invokeActionOnly() line: 289	
          	DefaultActionInvocation.invoke() line: 252	
          	DebuggingInterceptor.intercept(ActionInvocation) line: 256	
          	DefaultActionInvocation.invoke() line: 246	
          	DefaultWorkflowInterceptor.doIntercept(ActionInvocation) line: 176	
          	DefaultWorkflowInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98	
          	DefaultActionInvocation.invoke() line: 246	
          	AnnotationValidationInterceptor(ValidationInterceptor).doIntercept(ActionInvocation) line: 265	
          	AnnotationValidationInterceptor.doIntercept(ActionInvocation) line: 68	
          	AnnotationValidationInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98	
          	DefaultActionInvocation.invoke() line: 246	
          	StrutsConversionErrorInterceptor(ConversionErrorInterceptor).intercept(ActionInvocation) line: 138	
          	DefaultActionInvocation.invoke() line: 246	
          	ParametersInterceptor.doIntercept(ActionInvocation) line: 249	
          	ParametersInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98	
          	DefaultActionInvocation.invoke() line: 246	
          	ActionMappingParametersInteceptor(ParametersInterceptor).doIntercept(ActionInvocation) line: 249	
          	ActionMappingParametersInteceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98	
          	DefaultActionInvocation.invoke() line: 246	
          	StaticParametersInterceptor.intercept(ActionInvocation) line: 191	
          	DefaultActionInvocation.invoke() line: 246	
          	MultiselectInterceptor.intercept(ActionInvocation) line: 73	
          	DefaultActionInvocation.invoke() line: 246	
          	CheckboxInterceptor.intercept(ActionInvocation) line: 91	
          	DefaultActionInvocation.invoke() line: 246	
          	FileUploadInterceptor.intercept(ActionInvocation) line: 252	
          	DefaultActionInvocation.invoke() line: 246	
          	ModelDrivenInterceptor.intercept(ActionInvocation) line: 100	
          	DefaultActionInvocation.invoke() line: 246	
          	ScopedModelDrivenInterceptor.intercept(ActionInvocation) line: 141	
          	DefaultActionInvocation.invoke() line: 246	
          	ChainingInterceptor.intercept(ActionInvocation) line: 145	
          	DefaultActionInvocation.invoke() line: 246	
          	PrepareInterceptor.doIntercept(ActionInvocation) line: 171	
          	PrepareInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98	
          	DefaultActionInvocation.invoke() line: 246	
          	I18nInterceptor.intercept(ActionInvocation) line: 176	
          	DefaultActionInvocation.invoke() line: 246	
          	ServletConfigInterceptor.intercept(ActionInvocation) line: 164	
          	DefaultActionInvocation.invoke() line: 246	
          	AliasInterceptor.intercept(ActionInvocation) line: 193	
          	DefaultActionInvocation.invoke() line: 246	
          	ExceptionMappingInterceptor.intercept(ActionInvocation) line: 187	
          	DefaultActionInvocation.invoke() line: 246	
          	StrutsActionProxy.execute() line: 54	
          	Dispatcher.serviceAction(HttpServletRequest, HttpServletResponse, ServletContext, ActionMapping) line: 546	
          	ExecuteOperations.executeAction(HttpServletRequest, HttpServletResponse, ActionMapping) line: 77	
          	StrutsPrepareAndExecuteFilter.doFilter(ServletRequest, ServletResponse, FilterChain) line: 91	
          	ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 235	
          	ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 206	
          	StandardWrapperValve.invoke(Request, Response) line: 233	
          	StandardContextValve.invoke(Request, Response) line: 191	
          	StandardHostValve.invoke(Request, Response) line: 127	
          	ErrorReportValve.invoke(Request, Response) line: 102	
          	StandardEngineValve.invoke(Request, Response) line: 109	
          	CoyoteAdapter.service(Request, Response) line: 298	
          	Http11Processor.process(Socket) line: 857	
          	Http11Protocol$Http11ConnectionHandler.process(Socket) line: 588	
          	JIoEndpoint$Worker.run() line: 489	
          	Thread.run() line: 722	
          

          The result is the value 6 being displayed. OGNL evaluation is occurring via this .getText method.

          Regards

          Show
          coverity_srl Coverity Security Research Laboratory added a comment - Lukasz, I ran the sample tutorial application and modified the HelloWorld.java as such: public String execute() throws Exception { setMessage(getText(getMessage())); return SUCCESS; } And here's the current stack when debugging the tutorial under Eclipse via this URL: http: //127.0.0.1:8080/tutorial/example/HelloWorld.action?message=${2*3} Daemon Thread [http-8080-1] (Suspended (entry into method translateVariables in TextParseUtil)) TextParseUtil.translateVariables( char [], String , ValueStack, Class , TextParseUtil$ParsedValueEvaluator, int ) line: 156 TextParseUtil.translateVariables( char [], String , ValueStack, Class , TextParseUtil$ParsedValueEvaluator) line: 127 TextParseUtil.translateVariables( String , ValueStack) line: 49 LocalizedTextUtil.getDefaultMessage( String , Locale, ValueStack, Object [], String ) line: 663 LocalizedTextUtil.findText( Class , String , Locale, String , Object [], ValueStack) line: 534 LocalizedTextUtil.findText( Class , String , Locale, String , Object []) line: 362 TextProviderSupport.getText( String , String , List<?>) line: 208 TextProviderSupport.getText( String ) line: 123 HelloWorld(ActionSupport).getText( String ) line: 103 HelloWorld.execute() line: 30 NativeMethodAccessorImpl.invoke0(Method, Object , Object []) line: not available [ native method] NativeMethodAccessorImpl.invoke( Object , Object []) line: 57 DelegatingMethodAccessorImpl.invoke( Object , Object []) line: 43 Method.invoke( Object , Object ...) line: 601 DefaultActionInvocation.invokeAction( Object , ActionConfig) line: 450 DefaultActionInvocation.invokeActionOnly() line: 289 DefaultActionInvocation.invoke() line: 252 DebuggingInterceptor.intercept(ActionInvocation) line: 256 DefaultActionInvocation.invoke() line: 246 DefaultWorkflowInterceptor.doIntercept(ActionInvocation) line: 176 DefaultWorkflowInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98 DefaultActionInvocation.invoke() line: 246 AnnotationValidationInterceptor(ValidationInterceptor).doIntercept(ActionInvocation) line: 265 AnnotationValidationInterceptor.doIntercept(ActionInvocation) line: 68 AnnotationValidationInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98 DefaultActionInvocation.invoke() line: 246 StrutsConversionErrorInterceptor(ConversionErrorInterceptor).intercept(ActionInvocation) line: 138 DefaultActionInvocation.invoke() line: 246 ParametersInterceptor.doIntercept(ActionInvocation) line: 249 ParametersInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98 DefaultActionInvocation.invoke() line: 246 ActionMappingParametersInteceptor(ParametersInterceptor).doIntercept(ActionInvocation) line: 249 ActionMappingParametersInteceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98 DefaultActionInvocation.invoke() line: 246 StaticParametersInterceptor.intercept(ActionInvocation) line: 191 DefaultActionInvocation.invoke() line: 246 MultiselectInterceptor.intercept(ActionInvocation) line: 73 DefaultActionInvocation.invoke() line: 246 CheckboxInterceptor.intercept(ActionInvocation) line: 91 DefaultActionInvocation.invoke() line: 246 FileUploadInterceptor.intercept(ActionInvocation) line: 252 DefaultActionInvocation.invoke() line: 246 ModelDrivenInterceptor.intercept(ActionInvocation) line: 100 DefaultActionInvocation.invoke() line: 246 ScopedModelDrivenInterceptor.intercept(ActionInvocation) line: 141 DefaultActionInvocation.invoke() line: 246 ChainingInterceptor.intercept(ActionInvocation) line: 145 DefaultActionInvocation.invoke() line: 246 PrepareInterceptor.doIntercept(ActionInvocation) line: 171 PrepareInterceptor(MethodFilterInterceptor).intercept(ActionInvocation) line: 98 DefaultActionInvocation.invoke() line: 246 I18nInterceptor.intercept(ActionInvocation) line: 176 DefaultActionInvocation.invoke() line: 246 ServletConfigInterceptor.intercept(ActionInvocation) line: 164 DefaultActionInvocation.invoke() line: 246 AliasInterceptor.intercept(ActionInvocation) line: 193 DefaultActionInvocation.invoke() line: 246 ExceptionMappingInterceptor.intercept(ActionInvocation) line: 187 DefaultActionInvocation.invoke() line: 246 StrutsActionProxy.execute() line: 54 Dispatcher.serviceAction(HttpServletRequest, HttpServletResponse, ServletContext, ActionMapping) line: 546 ExecuteOperations.executeAction(HttpServletRequest, HttpServletResponse, ActionMapping) line: 77 StrutsPrepareAndExecuteFilter.doFilter(ServletRequest, ServletResponse, FilterChain) line: 91 ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 235 ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 206 StandardWrapperValve.invoke(Request, Response) line: 233 StandardContextValve.invoke(Request, Response) line: 191 StandardHostValve.invoke(Request, Response) line: 127 ErrorReportValve.invoke(Request, Response) line: 102 StandardEngineValve.invoke(Request, Response) line: 109 CoyoteAdapter.service(Request, Response) line: 298 Http11Processor.process(Socket) line: 857 Http11Protocol$Http11ConnectionHandler.process(Socket) line: 588 JIoEndpoint$Worker.run() line: 489 Thread .run() line: 722 The result is the value 6 being displayed. OGNL evaluation is occurring via this .getText method. Regards
          Hide
          coverity_srl Coverity Security Research Laboratory added a comment -

          Lukasz,

          Thank you for including a notice that evaluation is occurring. I also recommend updating the JavaDocs of ActionSupport.getText and the other affected methods to use stronger wording that OGNL evaluation of untrusted / tainted data is a security issue. (It can allow for remote code execution.) Otherwise, I could still see developers not understanding there's a risk using these APIs.

          Jon

          Show
          coverity_srl Coverity Security Research Laboratory added a comment - Lukasz, Thank you for including a notice that evaluation is occurring. I also recommend updating the JavaDocs of ActionSupport.getText and the other affected methods to use stronger wording that OGNL evaluation of untrusted / tainted data is a security issue. (It can allow for remote code execution.) Otherwise, I could still see developers not understanding there's a risk using these APIs. Jon
          Hide
          newton_dave Dave Newton added a comment -

          Coverity Security Research Laboratory AFAIK the evaluation isn't happening in getText(), rather as part of parameter processing. It's the same reason things like DisplayTag go funky because it uses a minus sign as part of the request parameter.

          Show
          newton_dave Dave Newton added a comment - Coverity Security Research Laboratory AFAIK the evaluation isn't happening in getText(), rather as part of parameter processing. It's the same reason things like DisplayTag go funky because it uses a minus sign as part of the request parameter.
          Hide
          rgielen Rene Gielen added a comment -

          Dave Newton No, parameter processing should be safe here - message property will contain "$

          {2*3}

          " after ParametersInterceptor; but passing the so far unevaluated expression string to getText() will force an OGNL evaluation in Jon's example.

          So far I see a "passing unsanitized user input to an API" issue, which is generally a questionable idea. I agree with Jon that the API JavaDocs should state clearly that expression evaluation will take place, such that users are warned. Nevertheless, I don't see we need further actions like active prevention and such.

          Just an idea: even more valuable than simple JavaDoc could be an annotation for parameters, like @SanitizingRequired or @ExpressionAware...

          Show
          rgielen Rene Gielen added a comment - Dave Newton No, parameter processing should be safe here - message property will contain "$ {2*3} " after ParametersInterceptor; but passing the so far unevaluated expression string to getText() will force an OGNL evaluation in Jon's example. So far I see a "passing unsanitized user input to an API" issue, which is generally a questionable idea. I agree with Jon that the API JavaDocs should state clearly that expression evaluation will take place, such that users are warned. Nevertheless, I don't see we need further actions like active prevention and such. Just an idea: even more valuable than simple JavaDoc could be an annotation for parameters, like @SanitizingRequired or @ExpressionAware...
          Hide
          newton_dave Dave Newton added a comment -

          Rene Gielen Hmm, OK, never mind then. Are issues like the display tag thing caused by type conversion, then?

          Show
          newton_dave Dave Newton added a comment - Rene Gielen Hmm, OK, never mind then. Are issues like the display tag thing caused by type conversion, then?
          Hide
          coverity_srl Coverity Security Research Laboratory added a comment -

          Dave Newton Please refer to the stack trace above and reproducer. Is there something incorrect in my reasoning that ActionSupport.getText / TextProviderSupport.getText doesn't eventually call TextParseUtil.translateVariables, passing in its String param as something that is later evaluated?

          Show
          coverity_srl Coverity Security Research Laboratory added a comment - Dave Newton Please refer to the stack trace above and reproducer. Is there something incorrect in my reasoning that ActionSupport.getText / TextProviderSupport.getText doesn't eventually call TextParseUtil.translateVariables, passing in its String param as something that is later evaluated?
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          It's a side effect - value of key becomes a default message if key cannot be localised in any message bundle.

          TextProviderSupport, line 122
              public String getText(String key) {
                  return getText(key, key, Collections.emptyList());
              }
          

          And extending JavaDocs for ActionSupport#getText isn't the best option as user can always change implementation of TextProvider and use its own.

          Dedicated annotation is a good idea but also at some level - and I would rather change implementation of TextProviderSupport to sanitise all the keys - basically they shouldn't contain any expression - but I can imagine the logic like this: key = message.${fieldName}.required - and this will break :\

          So thus means only dedicated annotation left.

          Show
          lukaszlenart Lukasz Lenart added a comment - It's a side effect - value of key becomes a default message if key cannot be localised in any message bundle. TextProviderSupport, line 122 public String getText( String key) { return getText(key, key, Collections.emptyList()); } And extending JavaDocs for ActionSupport#getText isn't the best option as user can always change implementation of TextProvider and use its own. Dedicated annotation is a good idea but also at some level - and I would rather change implementation of TextProviderSupport to sanitise all the keys - basically they shouldn't contain any expression - but I can imagine the logic like this: key = message.${fieldName}.required - and this will break :\ So thus means only dedicated annotation left.
          Hide
          rgielen Rene Gielen added a comment -

          Lukasz Lenart Yeah, the problem is that calls to getText with expressions may make sense. So you would only want to sanitize user input, but not any API call.

          That said, introducing such annotations would make even more sense if we'd finally introduce a central sanitization API! Say a central sanitizer class with static methods like sanitize(String), sanitize(String, SanitizingOptions ... options)

          Combined with static imports, a safe call to getText(@SanitizingRequired String message) would look like getText(sanitize(userModifieableProperty)).

          The actual sanitizer implementation should be Interface-based, with the Sanitizer class being a static facade using static injection / a factory for a singleton sanitizer implementation. Thus we could provide different sanitizers for different ELs used now and in future.

          WDYT?

          Show
          rgielen Rene Gielen added a comment - Lukasz Lenart Yeah, the problem is that calls to getText with expressions may make sense. So you would only want to sanitize user input, but not any API call. That said, introducing such annotations would make even more sense if we'd finally introduce a central sanitization API! Say a central sanitizer class with static methods like sanitize(String), sanitize(String, SanitizingOptions ... options) Combined with static imports, a safe call to getText(@SanitizingRequired String message) would look like getText(sanitize(userModifieableProperty)). The actual sanitizer implementation should be Interface-based, with the Sanitizer class being a static facade using static injection / a factory for a singleton sanitizer implementation. Thus we could provide different sanitizers for different ELs used now and in future. WDYT?
          Hide
          lukaszlenart Lukasz Lenart added a comment - - edited

          I'm not convinced - only incoming data should be sanitised, ie. in ParametersInterceptor but this can also be problematic as user can expect what he posts is what he gets :\

          I would rather add some warning to the logs or throw an exception if method annotated with @SanitizingRequired received call with unsanitized value - then developer can add the value to exception list or so.

          Show
          lukaszlenart Lukasz Lenart added a comment - - edited I'm not convinced - only incoming data should be sanitised, ie. in ParametersInterceptor but this can also be problematic as user can expect what he posts is what he gets :\ I would rather add some warning to the logs or throw an exception if method annotated with @SanitizingRequired received call with unsanitized value - then developer can add the value to exception list or so.
          Hide
          rgielen Rene Gielen added a comment - - edited

          Lukasz Lenart How would you track a value is sanitized beforehand? Since we encourage use of simple Java types, it might be hard to add metadata to a property to describe whether sanitizing is required or done already.

          IMO ParametersInterceptor's responsibility is to prevent evaluation of expressions while setting parameter properties. But in the end, the now filled property may now contain an expression which was not evaluated yet, but might get evaluated by some API calls in the Action code (see getText(username)). What is the best way to prevent users from shooting their feet without loosing flexibility?

          Going one step further, how about that:

          public enum SanitizingStrategy {
              WARN, CLEANUP, REJECT
          }
          
          @Documented
          public @interface Sanitized {
              
              SanitizingStrategy value() default SanitizingStrategy.CLEANUP;
              SanitizingOptions[] options() default {SanitizingOptions.DETECT_EL};
          }
          
          public class HelloWorld extends ExampleSupport {
          
              public String execute() throws Exception {
                  setMessage(getText(message));
                  setOtherMessage(getText(sanitize(manuallySanitizedMessage)));
                  return SUCCESS;
              }
          
              @Sanitized()
              private String message;
              
              private String manuallySanitizedMessage;
              
              //...
          }
          

          whereby a SanitizingInterceptor would be in the stack to apply sanitizing based on the given @Sanitize annotations, using the Sanitizer-API described in my earlier comment?

          Show
          rgielen Rene Gielen added a comment - - edited Lukasz Lenart How would you track a value is sanitized beforehand? Since we encourage use of simple Java types, it might be hard to add metadata to a property to describe whether sanitizing is required or done already. IMO ParametersInterceptor's responsibility is to prevent evaluation of expressions while setting parameter properties. But in the end, the now filled property may now contain an expression which was not evaluated yet, but might get evaluated by some API calls in the Action code (see getText(username)). What is the best way to prevent users from shooting their feet without loosing flexibility? Going one step further, how about that: public enum SanitizingStrategy { WARN, CLEANUP, REJECT } @Documented public @ interface Sanitized { SanitizingStrategy value() default SanitizingStrategy.CLEANUP; SanitizingOptions[] options() default {SanitizingOptions.DETECT_EL}; } public class HelloWorld extends ExampleSupport { public String execute() throws Exception { setMessage(getText(message)); setOtherMessage(getText(sanitize(manuallySanitizedMessage))); return SUCCESS; } @Sanitized() private String message; private String manuallySanitizedMessage; //... } whereby a SanitizingInterceptor would be in the stack to apply sanitizing based on the given @Sanitize annotations, using the Sanitizer-API described in my earlier comment?
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          So as I understand only this part will be sanitised during setting value? Or latter when this value will be used to call API?

          @Sanitized()
          private String message;
          
          Show
          lukaszlenart Lukasz Lenart added a comment - So as I understand only this part will be sanitised during setting value? Or latter when this value will be used to call API? @Sanitized() private String message;
          Hide
          rgielen Rene Gielen added a comment -

          It only makes sense to sanitize before setting the value, since the annotation information will be lost when the passed as parameter to an API call.

          Thus, there are basically two options:
          1. make Sanitizing a delegate call in ParametersInterceptor
          2. break up steps to separate concerns

          Regarding 2, combined with an earlier (less elaborate) idea: a possible workflow, each step implemented by interceptors, could look like this
          1. prepare naked parameter map, mapping request key-value-pairs to simple Map
          2. sanitize parameter keys, as strict as we do now in ParametersInterceptor; replace map / map keys / entries from step one so that paramter map now only contains sanitized key values
          3. sanitize parameter values based on evaluating keys against the stack, introspect candidate properties; if a candidate property (or its class!) contains the @Sanitize annotation, sanitize value based on this directive; replace map / map values / entries from step two so that paramter map now only contains sanitized keys and values. Important: only the map is sanitized, no model property in the stack is touched yet!
          4. set parameters on the model, based on the sanitized map. Basically this is a more lightweight version of nowadays ParametersInterceptor

          Point 4 is interesting, since now ParametersInterceptor would only serve a single concern. Also, paramPrepareParam would get more lightweight since the already sanitized parameter map would be used for both ParametersInterceptor calls. No need to sanitize two times then

          Show
          rgielen Rene Gielen added a comment - It only makes sense to sanitize before setting the value, since the annotation information will be lost when the passed as parameter to an API call. Thus, there are basically two options: 1. make Sanitizing a delegate call in ParametersInterceptor 2. break up steps to separate concerns Regarding 2, combined with an earlier (less elaborate) idea: a possible workflow, each step implemented by interceptors, could look like this 1. prepare naked parameter map, mapping request key-value-pairs to simple Map 2. sanitize parameter keys, as strict as we do now in ParametersInterceptor; replace map / map keys / entries from step one so that paramter map now only contains sanitized key values 3. sanitize parameter values based on evaluating keys against the stack, introspect candidate properties; if a candidate property (or its class!) contains the @Sanitize annotation, sanitize value based on this directive; replace map / map values / entries from step two so that paramter map now only contains sanitized keys and values. Important: only the map is sanitized, no model property in the stack is touched yet! 4. set parameters on the model, based on the sanitized map. Basically this is a more lightweight version of nowadays ParametersInterceptor Point 4 is interesting, since now ParametersInterceptor would only serve a single concern. Also, paramPrepareParam would get more lightweight since the already sanitized parameter map would be used for both ParametersInterceptor calls. No need to sanitize two times then
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          But thus means ParametersInterceptor must read structure of properties from action/model first - evaluate model - right now we pass root, property and value to OGNL to do the trick.

          Show
          lukaszlenart Lukasz Lenart added a comment - But thus means ParametersInterceptor must read structure of properties from action/model first - evaluate model - right now we pass root , property and value to OGNL to do the trick.
          Hide
          rgielen Rene Gielen added a comment -

          An OGNL PropertyAccessor implementation (not targeted to specific classes, though) might do the trick.

          But this has to be solved to implement such a strategy, yes!

          Show
          rgielen Rene Gielen added a comment - An OGNL PropertyAccessor implementation (not targeted to specific classes, though) might do the trick. But this has to be solved to implement such a strategy, yes!
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Almost the same idea as Rene presented

          Show
          lukaszlenart Lukasz Lenart added a comment - Almost the same idea as Rene presented
          Hide
          lukaszlenart Lukasz Lenart added a comment - - edited

          I have added the following warning to our Security guideline [1] and this can be closed.

          [1] https://cwiki.apache.org/confluence/display/WW/Security#Security-Donotuseincomingvaluesasaninputforlocalisationlogic

          Show
          lukaszlenart Lukasz Lenart added a comment - - edited I have added the following warning to our Security guideline [1] and this can be closed. [1] https://cwiki.apache.org/confluence/display/WW/Security#Security-Donotuseincomingvaluesasaninputforlocalisationlogic

            People

            • Assignee:
              lukaszlenart Lukasz Lenart
              Reporter:
              coverity_srl Coverity Security Research Laboratory
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development