Ruby: Add Improper LDAP Authentication query (CWE-287)

[maikypedia]

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]

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

[maikypedia]

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

[alexrford]

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

[maikypedia]

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

[alexrford]

@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().

[maikypedia]

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

[github-actions]

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]

@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?

[maikypedia]

@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]

Sure, reverted 😄

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