Incomplete url string sanitization by aibaars · Pull Request #8354 · github/codeql

aibaars · 2022-03-07T15:18:24Z

Added a new query, rb/incomplete-url-substring-sanitization. The query finds instances where a URL is incompletely sanitized due to insufficient checks.

github-actions · 2022-03-08T11:48:55Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

…uery

github-actions · 2022-03-09T11:12:47Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qll

erik-krogh

JS 👍

ruby/ql/lib/codeql/ruby/InclusionTests.qll

nickrolfe · 2022-03-11T15:39:48Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+  }
+
+  /**
+   * A check of form `A.index(B) != -1`, `A.index(B) >= 0`, or similar.


String#index and Array#index return nil on failure, not -1.

Perhaps we should extend this to cover checks of the form A.index(B) != nil and !A.index(B).nil?.

ruby/ql/lib/codeql/ruby/StringOps.qll

ruby/ql/lib/codeql/ruby/security/performance/RegExpTreeView.qll

ruby/ql/src/queries/security/cwe-020/examples/IncompleteUrlSubstringSanitization_BAD1.rb

asgerf · 2022-03-15T11:50:23Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+        or
+        value = -1 and polarity = true and comparison.getExpr() instanceof NEExpr
+        or
+        value = 0 and polarity = false and comparison.getExpr() instanceof NEExpr


Why this case? I'd say that value = 0 is only relevant for relational comparisons.

hvitved · 2022-03-16T12:11:35Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+ * A.index(B) >= 0
+ * ```
+ */
+class InclusionTest extends DataFlow::Node instanceof InclusionTest::Range {


Why is this using data-flow nodes instead of CFG nodes?

The JavaScript library does the same, so keeping it this way makes it easier to share the implementation of the query.

ruby/ql/lib/codeql/ruby/StringOps.qll

Co-authored-by: Nick Rolfe <nickrolfe@github.com>

github-actions · 2022-03-16T13:50:32Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
        end
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

github-actions · 2022-03-16T16:10:57Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
        end
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

…ng-sanitization Conflicts: config/identical-files.json javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qll ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qll

github-actions · 2022-03-18T15:10:41Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
        end
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

github-actions · 2022-03-24T10:28:29Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
        end
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

hvitved · 2022-03-28T10:54:04Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+        (
+          value = index.getConstantValue().getInt() and value = 0
+          or
+          index.getExpr() instanceof NilLiteral and value = -1


Can use index.getConstantValue().isNil() instead.

hvitved · 2022-03-28T10:55:05Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+ */
+class InclusionTest extends DataFlow::Node instanceof InclusionTest::Range {
+  /** Gets the `A` in `A.include?(B)`. */
+  DataFlow::Node getContainerNode() { result = super.getContainerNode() }


hvitved · 2022-03-28T10:55:09Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+  DataFlow::Node getContainerNode() { result = super.getContainerNode() }
+
+  /** Gets the `B` in `A.include?(B)`. */
+  DataFlow::Node getContainedNode() { result = super.getContainedNode() }


hvitved · 2022-03-28T10:55:13Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+   * If the polarity is `false` the check returns `true` if the container does not contain
+   * the given element.
+   */
+  boolean getPolarity() { result = super.getPolarity() }


hvitved · 2022-03-28T10:56:03Z

ruby/ql/lib/codeql/ruby/InclusionTests.qll

+  private class Includes_Native extends Range, DataFlow::CallNode {
+    Includes_Native() {
+      this.getMethodName() = "include?" and
+      count(this.getArgument(_)) = 1


may as well use strictcount

hvitved · 2022-03-28T10:57:42Z

ruby/ql/lib/codeql/ruby/StringOps.qll

+        exists(ExprNodes::ComparisonOperationCfgNode comparison |
+          this.asExpr() = comparison and
+          indexOf.getMethodName() = "index" and
+          count(indexOf.getArgument(_)) = 1 and


strictcount

hvitved · 2022-03-28T10:59:13Z

ruby/ql/lib/codeql/ruby/StringOps.qll

+    /**
+     * Gets the `A` in `A.start_with?(B)`.
+     */
+    DataFlow::Node getBaseString() { result = super.getBaseString() }


hvitved · 2022-03-28T10:59:17Z

ruby/ql/lib/codeql/ruby/StringOps.qll

+    /**
+     * Gets the `B` in `A.start_with?(B)`.
+     */
+    DataFlow::Node getSubstring() { result = super.getSubstring() }


hvitved · 2022-03-28T10:59:20Z

ruby/ql/lib/codeql/ruby/StringOps.qll

+     * If the polarity is `false` the check returns `true` if the string does not end
+     * with the given substring.
+     */
+    boolean getPolarity() { result = super.getPolarity() }


hvitved · 2022-03-28T11:00:23Z

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitizationSpecific.qll

+
+/** Holds if `node` may evaluate to `value` */
+predicate mayHaveStringValue(DataFlow::Node node, string value) {
+  node.asExpr().getExpr().getConstantValue().getString() = value


You can remove getExpr()

github-actions · 2022-03-28T11:17:13Z

QHelp previews:

ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qhelp

Incomplete URL substring sanitization

Sanitizing untrusted URLs is an important technique for preventing attacks such as request forgeries and malicious redirections. Usually, this is done by checking that the host of a URL is in a set of allowed hosts.

However, treating the URL as a string and checking if one of the allowed hosts is a substring of the URL is very prone to errors. Malicious URLs can bypass such security checks by embedding one of the allowed hosts in an unexpected location.

Even if the substring check is not used in a security-critical context, the incomplete check may still cause undesirable behaviors when the check succeeds accidentally.

Recommendation

Parse a URL before performing a check on its host value, and ensure that the check handles arbitrary subdomain sequences correctly.

Example

The following example code checks that a URL redirection will reach the example.com domain, or one of its subdomains, and not some malicious site.

class AppController < ApplicationController
    def index
        url = params[:url]
        # BAD: the host of `url` may be controlled by an attacker
        if url.include?("example.com")
            redirect_to url
        end
    end
end

The substring check is, however, easy to bypass. For example by embedding example.com in the path component: http://evil-example.net/example.com, or in the query string component: http://evil-example.net/?x=example.com. Address these shortcomings by checking the host of the parsed URL instead:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # BAD: the host of `url` may be controlled by an attacker
        if host.include?("example.com")
            redirect_to url
        end
    end
end

This is still not a sufficient check as the following URLs bypass it: http://evil-example.com http://example.com.evil-example.net. Instead, use an explicit whitelist of allowed hosts to make the redirect secure:

class AppController < ApplicationController
    def index
        url = params[:url]
        host = URI(url).host
        # GOOD: the host of `url` can not be controlled by an attacker
        allowedHosts = [
            'example.com',
            'beta.example.com',
            'www.example.com'
        ]
        if allowedHosts.include?(host)
            redirect_to url
        end
    end
end

References

OWASP: SSRF
OWASP: XSS Unvalidated Redirects and Forwards Cheat Sheet.
Common Weakness Enumeration: CWE-20.

aibaars added 3 commits March 7, 2022 16:09

JavaScript: fix typos in documentation

eeb9a1d

Ruby: copy JS version of IncompleteUrlSubstringSanitization.ql

c9fa1fb

Ruby: IncompleteUrlSubstringSanitization.ql

a1873cc

github-actions bot added documentation JS Ruby labels Mar 7, 2022

Add missing QLdoc

49b4fe7

aibaars force-pushed the incomplete-url-string-sanitization branch 15 times, most recently from 5d9ee09 to 49b4fe7 Compare March 8, 2022 09:47

github deleted a comment from github-actions bot Mar 8, 2022

aibaars marked this pull request as ready for review March 8, 2022 16:41

aibaars requested review from a team as code owners March 8, 2022 16:41

JS/Ruby: share implementation of IncompleteUrlSubstringSanitization q…

747c7f6

…uery

erik-krogh reviewed Mar 9, 2022

View reviewed changes

javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qll Show resolved Hide resolved

erik-krogh previously approved these changes Mar 9, 2022

View reviewed changes

nickrolfe reviewed Mar 11, 2022

View reviewed changes

asgerf reviewed Mar 15, 2022

View reviewed changes

hvitved reviewed Mar 16, 2022

View reviewed changes

Apply suggestions from code review

f2ec513

Co-authored-by: Nick Rolfe <nickrolfe@github.com>

aibaars dismissed erik-krogh’s stale review via f2ec513 March 16, 2022 13:46

aibaars added 2 commits March 16, 2022 16:18

Ruby: String.index method returns 'nil', not '-1'

fb8cc6e

Ruby: remove wrong clause

f95e1ef

Merge branch 'main' into incomplete-url-string-sanitization

65f8f56

hvitved requested changes Mar 28, 2022

View reviewed changes

Address comments

85c4daa

hvitved approved these changes Mar 28, 2022

View reviewed changes

aibaars merged commit 15c54f6 into github:main Mar 31, 2022

hvitved mentioned this pull request Mar 31, 2022

Ruby: Fix broken import #8624

Merged

Conversation

aibaars commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 8, 2022

Incomplete URL substring sanitization

Recommendation

Example

References

Uh oh!

github-actions bot commented Mar 9, 2022

Incomplete URL substring sanitization

Recommendation

Example

References

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 16, 2022

Incomplete URL substring sanitization

Recommendation

Example

References

Uh oh!

github-actions bot commented Mar 16, 2022

Incomplete URL substring sanitization

Recommendation

Example

References

Uh oh!

github-actions bot commented Mar 18, 2022

Incomplete URL substring sanitization

Recommendation

Example

References

Uh oh!

github-actions bot commented Mar 24, 2022

Incomplete URL substring sanitization

Recommendation

Example

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 28, 2022

Incomplete URL substring sanitization

aibaars commented Mar 7, 2022 •

edited

Loading