Skip to content

Ruby: Add LDAP Injection query #13309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Aug 1, 2023

Conversation

maikypedia
Copy link
Contributor

This pull request adds a query for LDAP Injection covering net-ldap.

Looking forward to your suggestions.

@maikypedia maikypedia requested a review from a team as a code owner May 28, 2023 15:44
@maikypedia maikypedia changed the title Ruby: Add LDAP Injection query #12311 Ruby: Add LDAP Injection query May 28, 2023
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the query! I'm overall happy with this, just some minor requests.

Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should fix the deprecation warnings.

maikypedia and others added 4 commits July 11, 2023 19:20
Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>
Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>
@github-actions
Copy link
Contributor

QHelp previews:

ruby/ql/src/experimental/ldap-injection/LdapInjection.qhelp

LDAP Injection

If an LDAP query or DN is built using string concatenation or string formatting, and the components of the concatenation include user input without any proper sanitization, a user is likely to be able to run malicious LDAP queries.

Recommendation

If user input must be included in an LDAP query or DN, it should be escaped to avoid a malicious user providing special characters that change the meaning of the query.

Example

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

The user and dc is specified using a parameter, user_name and dc provided by the client which it then uses to build a LDAP query and DN. This value is accessible using the params method.

The first example uses the unsanitized user input directly in the search filter and DN for the LDAP query. A malicious user could provide special characters to change the meaning of these components, and search for a completely different set of values.

require 'net/ldap'

class BadLdapController < ActionController::Base
  def ldap_handler
    name = params[:user_name]
    dc = params[:dc]
    ldap = Net::LDAP.new(
        host: 'ldap.example.com',
        port: 636,
        encryption: :simple_tls,
        auth: {
            method: :simple,
            username: 'uid=admin,dc=example,dc=com',
            password: 'adminpassword'
        }
    )
    filter = Net::LDAP::Filter.eq('foo', name)
    attrs = [name]
    result = ldap.search(base: "ou=people,dc=#{dc},dc=com", filter: filter, attributes: attrs)
  end
end

In the second example, the input provided by the user is sanitized before it is included in the search filter or DN. This ensures the meaning of the query cannot be changed by a malicious user.

require 'net/ldap'

class GoodLdapController < ActionController::Base
  def ldap_handler
    name = params[:user_name]
    ldap = Net::LDAP.new(
        host: 'ldap.example.com',
        port: 636,
        encryption: :simple_tls,
        auth: {
            method: :simple,
            username: 'uid=admin,dc=example,dc=com',
            password: 'adminpassword'
        }
    )
    
    name = if ["admin", "guest"].include? name
      name
    else 
      name = "none"
    end

    filter = Net::LDAP::Filter.eq('foo', name)
    attrs = ['dn']
    result = ldap.search(base: 'ou=people,dc=example,dc=com', filter: filter, attributes: attrs)
  end
end

References

@alexrford
Copy link
Contributor

Hey @maikypedia, sorry for the delay on this PR. I had another look through this PR and there are a few comments from my initial review that got lost in the changes. Would you mind looking through the comments in that review again?

@maikypedia
Copy link
Contributor Author

Hey @maikypedia, sorry for the delay on this PR. I had another look through this PR and there are a few comments from my initial review that got lost in the changes. Would you mind looking through the comments in that review again?

Sorry, I completely overlooked it 😅. I think with this, everything should be ready now. Is there anything I might be missing?

@alexrford alexrford self-requested a review July 31, 2023 13:59
alexrford
alexrford previously approved these changes Jul 31, 2023
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a few tiny syntax fixes, I'll merge this when the checks pass.

@alexrford alexrford merged commit 2b74144 into github:main Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants