Ruby: Add Server Side Template Injection query #12311
Conversation
alexrford
left a comment
alexrford
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I'm generally happy with the structure of this and appreciate the use of Concepts.qll.
We generally request that new queries are added to the experimental directory first (https://github.com/github/codeql/blob/main/CONTRIBUTING.md#submitting-a-new-experimental-query) - would you mind moving this query to ruby/ql/src/experimental/?
Otherwise I just have some stylistic and naming suggestions.
|
|
||
| /** A call to `ERB.new`, considered as a template construction. */ | ||
| private class ERBTemplateNewCall extends TemplateConstruction::Range, DataFlow::CallNode { | ||
| ERBTemplateNewCall() { this = API::getTopLevelMember("ERB").getAMethodCall("new") } |
There was a problem hiding this comment.
| ERBTemplateNewCall() { this = API::getTopLevelMember("ERB").getAMethodCall("new") } | |
| ErbTemplateNewCall() { this = API::getTopLevelMember("ERB").getAMethodCall("new") } |
ruby/ql/lib/codeql/ruby/security/TemplateInjectionCustomizations.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: intrigus-lgtm <[email protected]>
Co-authored-by: intrigus-lgtm <[email protected]>
Co-authored-by: Alex Ford <[email protected]>
alexrford
left a comment
alexrford
left a comment
There was a problem hiding this comment.
I'm happy with this, just needs a couple of minor changes before we can merge it:
- There's a merge conflict in
Frameworks.qll- probably because another import was added to the file - We need a change note to document the new query. This can be added by creating a new file
ruby/ql/src/change-notes/2023-03-15-ssti-query, e.g:
---
category: newQuery
---
* Added a new experimental query, `rb/server-side-template-injection`, to detect cases where user input may be embedded into a template's code in an unsafe manner.(feel free to change the wording in this note if you'd like).
|
QHelp previews: ruby/ql/src/experimental/template-injection/TemplateInjection.qhelpServer-side template injectionTemplate Injection occurs when user input is embedded in a template's code in an unsafe manner. An attacker can use native template syntax to inject a malicious payload into a template, which is then executed server-side. This permits the attacker to run arbitrary code in the server's context. RecommendationTo fix this, ensure that untrusted input is not used as part of a template's code. If the application requirements do not allow this, use a sandboxed environment where access to unsafe attributes and methods is prohibited. ExampleConsider the example given below, an untrusted HTTP parameter require 'erb'
require 'slim'
class BadERBController < ActionController::Base
def some_request_handler
name = params["name"]
html_text = "
<!DOCTYPE html><html><body>
<h2>Hello %s </h2></body></html>
" % name
template = ERB.new(html_text).result(binding)
end
end
class BadSlimController < ActionController::Base
def some_request_handler
name = params["name"]
html_text = "
<!DOCTYPE html><html><body>
<h2>Hello %s </h2></body></html>
" % name
Slim::Template.new{ html_text }.render
end
endHere we have fixed the problem by including ERB/Slim syntax in the string, then the user input will be rendered but not evaluated. require 'erb'
require 'slim'
class GoodController < ActionController::Base
def some_request_handler
name = params["name"]
html_text = "
<!DOCTYPE html><html><body>
<h2>Hello <%= name %> </h2></body></html>
"
template = ERB.new(html_text).result(binding)
end
end
class GoodController < ActionController::Base
def some_request_handler
name = params["name"]
html_text = "
<!DOCTYPE html>
html
body
h2 == name;
"
Slim::Template.new{ html_text }.render(Object.new, name: name)
end
endReferences
|
3 participants
This pull request adds a query for Server Side Template Injection covering ERB and Slim.
Looking forward to your suggestions.