Forum Discussion

hattesen's avatar
hattesen
Adept
9 years ago

LiQL injection attacks and quotes in Rest API 2.0

I am sorry if this subject has already been covered, but I have searched forums, knowledge bases and Google for any answers and have come up empty handed.

 

I am wondering how LiQL is protected against:

  1. LiQL injection attacks (when query parameters contain user-entered data) like SQL Injection attacks.
  2. LiQL handling of query parameters that contain single quotes.

 

Example (made up):

 

<#assign replies = rest("2.0","/search?q=" + "SELECT id, body FROM messages WHERE topic.tag = '${topic_tag}' AND depth > 0"?url).data.items />

 

If the variable topic_tag comes from a user-entered value, and it itself contains a single quote ['], the rest query string will have unmatched quotes, and would probably cause the query to fail.

 

<#assign topic_tag = "don't_do_this" />

 

... would cause the query to be ...

 

"/search?q=SELECT id, body FROM messages WHERE topic.tag = 'don't_do_this' AND depth > 0"

 

The part of the query colored red would surely upset the query parser, right?

 

Shouldn't variables that are expanded into LiQL be escaped to prevent this from ever happening?

 

 

 

 

 

 

 

 

8 Replies

Sort By
  • FaisalK's avatar
    FaisalK
    Lithium Alumni (Retired)
    9 years ago

    Excellent post, Hattesen. It's a general best practice that any user input be escaped and encoded as appropriate and according to the syntax in which it will appear before it's processed or used in any part of the community. This includes all untrusted input such as any part of the URL, any input fields, any HTTP headers, search terms, etc. This gets a bit tricky with labels and tags allowing single or double quotes as you have already pointed out.

     

    Re: your SQL injection concern - LiQL's syntax is very similar to a SQL statement. You are absolutely right again that tainted user input could definitely influence a LiQL query statement in freemarker templates. However, a major difference being that influencing the LiQL query will not give you access to the underlying SQL tables/db schema. That's because the LiQL query (REST API V2) is an abstraction layer which essentially maps to predefined objects and not SQL tables, for instances messages, authors, boards, etc. For every query we look at each of the fields to ensure that they match the object definition for the provided collection name. Influencing the syntax of the query will only allow an attacker to maneuver within the confines of prepackaged objects restricted through the lens of permissions assigned to the session.

     

    Back to your question about the query syntax. This syntax will result in an error message.

    SELECT id, body FROM messages WHERE labels.text = 'don't_do_this'

     

    However, this syntax will *NOT* result is an error.

    SELECT id, body FROM messages WHERE labels.text = "don't_do_this"

  • hattesen's avatar
    hattesen
    Adept
    9 years ago
    But there needs to be a generic and SAFE way to avoid attacks and syntax errors, regardless of WHERE term content.

    I would imagine, that the String builtin jsString might do the job...

    <#assign replies = rest("2.0","/search?q=" + "SELECT id, body FROM messages WHERE topic.tag = '${topic_tag?js_string}' AND depth > 0"?url).data.items />

    See http://freemarker.org/docs/ref_builtins_string.html#ref_builtin_js_string

    However, this is a serious security and stability problem that needs a "best practice" to be defined by the Lithium developers. Believing that something MIGHT be safe, just isn't acceptable in this sort of situation.

  • hattesen's avatar
    hattesen
    Adept
    9 years ago

    With regards to your comment about "the LiQL query will not give you access to the underlying SQL tables/db schema". Wouldn't you be able to construct a query that would add this to a WHERE term:

     

    ... OR true=true

     

    ... wouldn't that make the query return the entire content of the table, thereby possibly revealing too much information?

     

    In other words: Can you categorically say, that an injection attack on a WHERE term can never result in additional information being returned in the result of the API 2.0 REST call?

  • FaisalK's avatar
    FaisalK
    Lithium Alumni (Retired)
    9 years ago

    I agree in principle, however, this is no different than coding in Java or JS or any other language. Incorporating untrusted user input is never a good idea. And like many of those other languages, it's left up to the developer, freemarker in this case, to incorporate the security best practices (?html, js_string, etc). 

     

    This type of customization is not considered a user level task, it should only be attempted by a trained and knowledgeable person like yourself.

  • FaisalK's avatar
    FaisalK
    Lithium Alumni (Retired)
    9 years ago

    Another excellent point. You are correct again, OR true=true might return everything but only to the extent what the user's permissions should allow them to see. Nothing more, nothing less. By design and by default, the user is able to interact with the API and get exactly the same information he or she is allowed to see via the GUI.

     

    In other words, your design choices should not try to override what information is presented to the user solely on constructing a LiQL query. 

  • hattesen's avatar
    hattesen
    Adept
    9 years ago
    When coding Java against a database, you use prepared statements with parameters for this very reason.

    Lithium developers should not be expected to foresee injection attacks and manually escaping strings. At the very least, a "best practice" should be established, and recommendations should be made for using ?ja_string, or whatever is deemed best for escaping quotes and double quotes.
  • hattesen's avatar
    hattesen
    Adept
    9 years ago
    What about restadmin calls made on behalf of a user? Wouldn't that return data that the user would not be able to obtain via calls to the API?
  • FaisalK's avatar
    FaisalK
    Lithium Alumni (Retired)
    9 years ago

    Your design choices should not try to override what information is presented to the user solely on constructing a LiQL query. When using restadmin you should keep that principle in mind even more front and center. Think of restadmin as the exec() equivalent, just b/c it's there doesn't mean it's safe to use for every situation (unless you absolutely know what you are doing).

     

    You have raised some very astute points, thanks for a very exciting and well informed discussion on this important topic. Feel free to open up a Support case and if you like I can call you to discuss further.

     

    And last but not least, sorry for a shameless plug but I'm hiring a Lead Security Engineer, do you  know anyone? Your kind of skill set and knowledge is essential to be successful at this role (based in San Francisco, CA).

     

    -Faisal