Ruby: Add Improper LDAP Authentication query (CWE-287) by maikypedia · Pull Request #13313 · github/codeql

maikypedia · 2023-05-29T19:23:50Z

This pull request adds a query for Improper LDAP Authentication to prevent attackers use an empty password. This branch is created from maikypedia/ldap-injection branch (PR here) so maybe it is more convenient to review this one once the other is resolved.

Looking forward to your suggestions.

alexrford

Hi @maikypedia, I've given this an initial review - mostly stylistic suggestions. There's a minor merge conflict in NetLdapConnection, I think relating to its charpred. Would you mind merging in main and fixing this conflict?

alexrford · 2023-08-02T15:22:53Z

+  /** Holds if the binding process is anonymous. */
+  predicate isEmptyPassword() {
+    (
+      this.getPassword().getConstantValue().isStringlikeValue("")
+      or
+      this.getPassword().(DataFlow::ExprNode).getExprNode().getConstantValue().getValueType() =
+        "nil"
+    )
+  }


This predicate looks to be unused - can we remove it?

Would it be more convenient to declare nil and "" as sources?

Do we need this at all? I can't find anywhere that this predicate is used.

I forgot to finish it, it was to consider the empty password and nil as vulnerable

@maikypedia Okay, that makes sense to me. Would you mind either adding this as a source? I think the predicate itself probably belongs in ImproperLdapAuthCustomizations.qll rather than Concepts.qll.

As a tiny nitpick, we could replace getValueType() = "nil" with just isNil().

@alexrford Done! Although the output is a bit ugly, every step of the source node is considered a new source 🥲

Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>

github-actions · 2023-08-04T11:57:13Z

QHelp previews:

ruby/ql/src/experimental/ldap-improper-auth/ImproperLdapAuth.qhelp

Improper LDAP Authentication

If an LDAP connection uses user-supplied data as password, anonymous bind could be caused using an empty password to result in a successful authentication.

Recommendation

Don't use user-supplied data as password while establishing an LDAP connection.

Example

In the following Rails example, an ActionController class has a ldap_handler method to handle requests.

In the first example, the code builds a LDAP query whose authentication depends on user supplied data.

class FooController < ActionController::Base
  def some_request_handler
    pass = params[:pass]
    ldap = Net::LDAP.new(
        host: 'ldap.example.com',
        port: 636,
        encryption: :simple_tls,
        auth: {
            method: :simple,
            username: 'uid=admin,dc=example,dc=com',
            password: pass
        }
    )
    ldap.bind
  end
end

In the second example, the authentication is established using a default password.

class FooController < ActionController::Base
  def some_request_handler
    pass = params[:pass]
    ldap = Net::LDAP.new(
        host: 'ldap.example.com',
        port: 636,
        encryption: :simple_tls,
        auth: {
            method: :simple,
            username: 'uid=admin,dc=example,dc=com',
            password: '$uper$password123'
        }
    )
    ldap.bind
  end
end

References

MITRE: CWE-287: Improper Authentication.
Common Weakness Enumeration: CWE-287.

alexrford

Tiny note, and I left another comment on isEmptyPassword()

alexrford · 2023-08-25T11:44:51Z

@maikypedia sorry, just one last comment! I think we can probably just revert the last commit to remove EmptySource as a source. We have an issue where, because we're using taint tracking, any value that's "tainted" with an empty string or nil is considered to be empty. This is not necessarily the case, e.g. in:

    pass = get_a_secure_password
    pass += "" # this taints `pass` with an "empty password"

    ldap = Net::LDAP.new(
        host: 'ldap.example.com',
        port: 636,
        encryption: :simple_tls,
        auth: {
            method: :simple,
            username: 'uid=admin,dc=example,dc=com',
            password: pass # False positive: 
        }
    )
    ldap.bind

We can technically fix this by either using value only tracking for this case, but this feels like a potentially significant piece of work and I'm keen to get this merged so we can then iterate on this query. Would you mind reverting the last commit, then this should be fine to merge?

This reverts commit 664c1eb.

maikypedia · 2023-08-25T13:27:24Z

@maikypedia sorry, just one last comment! I think we can probably just revert the last commit to remove EmptySource as a source. We have an issue where, because we're using taint tracking, any value that's "tainted" with an empty string or nil is considered to be empty. This is not necessarily the case, e.g. in:
    pass = get_a_secure_password
    pass += "" # this taints `pass` with an "empty password"

    ldap = Net::LDAP.new(
        host: 'ldap.example.com',
        port: 636,
        encryption: :simple_tls,
        auth: {
            method: :simple,
            username: 'uid=admin,dc=example,dc=com',
            password: pass # False positive: 
        }
    )
    ldap.bind
We can technically fix this by either using value only tracking for this case, but this feels like a potentially significant piece of work and I'm keen to get this merged so we can then iterate on this query. Would you mind reverting the last commit, then this should be fine to merge?

Sure, reverted 😄

alexrford · 2023-08-25T16:12:32Z

Sure, reverted 😄

Thanks for your patience on the review 🙇, just ran the formatter and waiting on checks to complete before merging.

maikypedia added 2 commits May 29, 2023 21:16

Add Improper LDAP Authentication query (CWE-287)

6235312

fix concepts

345f43f

maikypedia requested a review from a team as a code owner May 29, 2023 19:23

github-actions bot added documentation Ruby labels May 29, 2023

maikypedia mentioned this pull request May 29, 2023

[Ruby]: Add Improper LDAP Authentication query github/securitylab#761

Closed

2 tasks

calumgrant requested a review from alexrford June 6, 2023 08:18

alexrford suggested changes Aug 3, 2023

View reviewed changes

maikypedia and others added 8 commits August 3, 2023 16:18

change useSsl() to usesSsl()

2d87489

simplify usesSsl()

f7a6628

Fix getHost() (ldap.host = ip is not covered)

fd649c1

Merge branch 'main' into maikypedia/ldap-improper-auth

c54561e

typo

0237f37

Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>

typo

6f1b406

Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>

Replace cast to DataFlow::CallNode

0dec487

Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>

Make private

0cd1c1e

Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>

github-advanced-security AI found potential problems Aug 4, 2023

View reviewed changes

Comment thread ruby/ql/lib/codeql/ruby/frameworks/Ldap.qll Fixed

maikypedia added 3 commits August 10, 2023 00:13

fix Singleton set literal

c5c2df4

Remove unnecesary }

e60fa6f

add getValue() method

97f8d1f

alexrford reviewed Aug 10, 2023

View reviewed changes

Comment thread ruby/ql/lib/codeql/ruby/frameworks/Ldap.qll Outdated

alexrford suggested changes Aug 10, 2023

View reviewed changes

Comment thread ruby/ql/src/change-notes/2023-29-05-improper-ldap-auth-query.md

maikypedia added 3 commits August 16, 2023 15:32

rename change-note

5f502ec

Remove isEmptyPassword predicate

f301e46

Add "" and nil as sources

664c1eb

alexrford closed this Aug 25, 2023

alexrford reopened this Aug 25, 2023

Revert "Add "" and nil as sources"

ffd618d

This reverts commit 664c1eb.

Ruby: autoformat

ae635c6

alexrford approved these changes Aug 25, 2023

View reviewed changes

alexrford merged commit 9957e26 into github:main Aug 25, 2023

Conversation

maikypedia commented May 29, 2023

Uh oh!

alexrford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexrford Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

maikypedia Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

alexrford Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

maikypedia Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

alexrford Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

maikypedia Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 4, 2023

Improper LDAP Authentication

Recommendation

Example

References

Uh oh!

Uh oh!

Uh oh!

alexrford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexrford commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maikypedia commented Aug 25, 2023

Uh oh!

alexrford commented Aug 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexrford commented Aug 25, 2023 •

edited

Loading