Wicket
  1. Wicket
  2. WICKET-2801

User input can inject property model expressions using StringResourceModel

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Invalid
    • Affects Version/s: 1.4.7
    • Fix Version/s: None
    • Component/s: wicket
    • Labels:
      None

      Description

      Applications that use StringResourceModel to render localized strings using a model and value arguments are subject to a security issue which allows users to perform property model expressions on the given model.

      For instance, the following statement:

      new StringResourceModel( "key", userModel, new Object[]

      { input.getModelObject() }

      )

      Would expand property model expressions from input's object against userModel's object, effectively allowing users to access unintended data from userModel's object.

      Consider the localization data:
      key=User $

      {name}

      said:

      {0}

      The user input:
      input.getModelObject() = "My password is $

      {pass}

      ."

      The StringResourceModel's object would yield a string like:
      User lhunath said: My password is secret

      Find attached test case which illustrates this problem using WicketTester.

      1. WICKET-2801-1.tbz2
        2 kB
        Maarten Billemont

        Activity

        Hide
        Jeremy Thomerson added a comment -

        What Igor says is exactly right on this - you are using StringResourceModel to concatenate "system input" + "user input". This is EXACTLY like concatenating "SELECT * FROM Users WHERE id = " + "user supplied ID". One is from the application, one is from the user.

        This is simply NOT what StringResourceModel was created for. See the javadocs - where it says clearly that StringResourceModel is for LOCALIZATION. Then read what localization is: http://en.wikipedia.org/wiki/Language_localization - it's not the same as templating http://en.wikipedia.org/wiki/Template_(software_engineering) - combining application and user input into something to render.

        As mentioned previously, you are absolutely free to create your own subclass that handles your use case. But you can't force us to do this for you. Wicket makes it super easy for you to extend these components and do it however you want or need to.

        Show
        Jeremy Thomerson added a comment - What Igor says is exactly right on this - you are using StringResourceModel to concatenate "system input" + "user input". This is EXACTLY like concatenating "SELECT * FROM Users WHERE id = " + "user supplied ID". One is from the application, one is from the user. This is simply NOT what StringResourceModel was created for. See the javadocs - where it says clearly that StringResourceModel is for LOCALIZATION. Then read what localization is: http://en.wikipedia.org/wiki/Language_localization - it's not the same as templating http://en.wikipedia.org/wiki/Template_(software_engineering ) - combining application and user input into something to render. As mentioned previously, you are absolutely free to create your own subclass that handles your use case. But you can't force us to do this for you. Wicket makes it super easy for you to extend these components and do it however you want or need to.
        Hide
        Maarten Billemont added a comment - - edited

        I beg to differ, currently MessageFormat is being abused for something it is not intended for under the veil of uninformed "intent" of StringResourceModel.

        Contrary to what you say, arguments to a format string are not application code; they should not contain any sort of syntax whatsoever. Review MessageFormat's documentation for a description of its purpose which is clearly targeted at generating a string based off of a format string and data arguments that is to be displayed to the end-user.

        I am very well aware that StringResourceModel currently considers its arguments as "application code" in the sense that it considers their contents to be of the same context as that of the localization value. I also understand there are alternative safe methods of approaching this problem. My argument, however, is against the sanity behind the existing API of StringResourceModel. I am yet to learn of any argument for the way things are now, and I have given you plenty against - which I have not seen you dispute. That alone should be enough to revert this bug to an Open state unless you can provide solid arguments as to why my impression of format arguments is so very wrong.

        As for Igor's take on things; you seem to be missing the context of it all. Contrary to what you say, my request is very much to NOT escape user data in any form while it resides within the application. Escaping of data is not something that should be done by a developer as he introduces his data into a certain context - rather that context should be fit to take the data the developer offers him without allowing it to be evaluated as application code by doing any necessary escaping itself, in ITS context and syntax.
        Similarly, "escaping single quotes in case you decide to build an SQL statement" is a fine example of introducing a form of "escaping" in application data. It is as rediculus as you make it out to be since it would be escaping data outside of the context of what you're escaping it for. At the same time, it is actually pretty close to what you're asking wicket developers to do now just before sending their data to StringResourceModel; escaping their own data outside of the context of property model expressions.
        I think if you want to make an analogy; more fitting would be comparing this to new Label("id", "foo"); where foo is actually escaped by default by the framework. As such; consistency dictates the developer can expect the same to happen for when he passes his "foo" to the framework as data elsewhere.

        No, StringResourceModel should NOT allow or feature code injection, because the very purpose of format strings is to cleanly and safely introduce arguments into application code, rather than inject them into it.

        Show
        Maarten Billemont added a comment - - edited I beg to differ, currently MessageFormat is being abused for something it is not intended for under the veil of uninformed "intent" of StringResourceModel. Contrary to what you say, arguments to a format string are not application code; they should not contain any sort of syntax whatsoever. Review MessageFormat's documentation for a description of its purpose which is clearly targeted at generating a string based off of a format string and data arguments that is to be displayed to the end-user. I am very well aware that StringResourceModel currently considers its arguments as "application code" in the sense that it considers their contents to be of the same context as that of the localization value. I also understand there are alternative safe methods of approaching this problem. My argument, however, is against the sanity behind the existing API of StringResourceModel. I am yet to learn of any argument for the way things are now, and I have given you plenty against - which I have not seen you dispute. That alone should be enough to revert this bug to an Open state unless you can provide solid arguments as to why my impression of format arguments is so very wrong. As for Igor's take on things; you seem to be missing the context of it all. Contrary to what you say, my request is very much to NOT escape user data in any form while it resides within the application. Escaping of data is not something that should be done by a developer as he introduces his data into a certain context - rather that context should be fit to take the data the developer offers him without allowing it to be evaluated as application code by doing any necessary escaping itself, in ITS context and syntax . Similarly, "escaping single quotes in case you decide to build an SQL statement" is a fine example of introducing a form of "escaping" in application data. It is as rediculus as you make it out to be since it would be escaping data outside of the context of what you're escaping it for. At the same time, it is actually pretty close to what you're asking wicket developers to do now just before sending their data to StringResourceModel; escaping their own data outside of the context of property model expressions. I think if you want to make an analogy; more fitting would be comparing this to new Label("id", "foo"); where foo is actually escaped by default by the framework. As such; consistency dictates the developer can expect the same to happen for when he passes his "foo" to the framework as data elsewhere. No, StringResourceModel should NOT allow or feature code injection, because the very purpose of format strings is to cleanly and safely introduce arguments into application code, rather than inject them into it.
        Hide
        Igor Vaynberg added a comment -

        what next? should wicket also protect you against sql injection attacks by automatically escaping single quotes in case you decide to build a sql statement using string concatenations? you are responsible for sanitizing user input based on the context in which it will be used.

        Show
        Igor Vaynberg added a comment - what next? should wicket also protect you against sql injection attacks by automatically escaping single quotes in case you decide to build a sql statement using string concatenations? you are responsible for sanitizing user input based on the context in which it will be used.
        Hide
        Jeremy Thomerson added a comment -

        Yes - but what you are doing is not what StringResourceModel is intended to be used for. You are saying "key=User $

        {name} said: {0} " where {0} is a piece of user-contributed data. You should NOT be doing this. The string resources are for internationalizing YOUR strings. So, you should place just "key=User ${name}

        said:" in the StringResourceModel and the user-contributed String in a regular Label.

        If your use case requires that you do anything other than what StringResourceModel was intended for, fine. You can definitely accomplish what you need. But the requirement of securing user data before using it in the StringResourceModel then becomes yours, not the framework's.

        Show
        Jeremy Thomerson added a comment - Yes - but what you are doing is not what StringResourceModel is intended to be used for. You are saying "key=User $ {name} said: {0} " where {0} is a piece of user-contributed data. You should NOT be doing this. The string resources are for internationalizing YOUR strings. So, you should place just "key=User ${name} said:" in the StringResourceModel and the user-contributed String in a regular Label. If your use case requires that you do anything other than what StringResourceModel was intended for, fine. You can definitely accomplish what you need. But the requirement of securing user data before using it in the StringResourceModel then becomes yours, not the framework's.
        Hide
        Maarten Billemont added a comment - - edited

        What you are passing as arguments is definitely considered "data" rather than "application code". In that light, it must remain untouched by any processing - whether "expected" or not.

        Am I to understand that evaluating property expressions is a feature of the arguments? Surely that cannot be the intended effect, for who would ever need their user data (not application code) to get evaluated as property expressions. Conversely, I can think of a very many cases where one needs it not to be evaluated as such.

        I believe the property evaluation string should be solely the value of the localization key, not the arguments given to it.

        And if indeed it is to be considered "expected", then it should be very clearly noted as such in the documentation with a bold warning sign demanding users escape each of their arguments before passing them along - though truthfully, I do not expect even this will attract the necessary attention of all developers unknowingly dealing with this issue.

        Show
        Maarten Billemont added a comment - - edited What you are passing as arguments is definitely considered "data" rather than "application code". In that light, it must remain untouched by any processing - whether "expected" or not. Am I to understand that evaluating property expressions is a feature of the arguments? Surely that cannot be the intended effect, for who would ever need their user data (not application code) to get evaluated as property expressions. Conversely, I can think of a very many cases where one needs it not to be evaluated as such. I believe the property evaluation string should be solely the value of the localization key, not the arguments given to it. And if indeed it is to be considered "expected", then it should be very clearly noted as such in the documentation with a bold warning sign demanding users escape each of their arguments before passing them along - though truthfully, I do not expect even this will attract the necessary attention of all developers unknowingly dealing with this issue.
        Hide
        Igor Vaynberg added a comment -

        you choose to wire user's input directly to a property evaluator, in which case what you see is the expected result. if you dont want it to happen then wrap the model in another that escapes the $ character with wicket's escape sequence $$

        Show
        Igor Vaynberg added a comment - you choose to wire user's input directly to a property evaluator, in which case what you see is the expected result. if you dont want it to happen then wrap the model in another that escapes the $ character with wicket's escape sequence $$
        Hide
        Maarten Billemont added a comment -

        Test case which reproduces this issue.

        Show
        Maarten Billemont added a comment - Test case which reproduces this issue.

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            Maarten Billemont
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development