Skip to content
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

Add support for ldapwhoami (RFC4532) (now with tests) #425

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zeroSteiner
Copy link
Contributor

@zeroSteiner zeroSteiner commented Feb 23, 2024

I'm hoping to see #396 completed by adding the tests that were requested. In doing so I also fixed a problem I noticed where it was not always returning the response string.

Tested with Active Directory and OpenLDAP.

@zeroSteiner
Copy link
Contributor Author

I was just wondering if there was anything I could do to help this along?

zeroSteiner added a commit to zeroSteiner/metasploit-framework that referenced this pull request Oct 9, 2024
zeroSteiner added a commit to zeroSteiner/metasploit-framework that referenced this pull request Oct 9, 2024
zeroSteiner added a commit to zeroSteiner/metasploit-framework that referenced this pull request Oct 17, 2024
Copy link
Member

@HarlemSquirrel HarlemSquirrel 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 tackling this!

Looks like tests are failing and I have a few questions

@@ -325,7 +325,7 @@ class Net::LDAP

universal = {
constructed: {
107 => :array, #ExtendedResponse (PasswdModifyResponseValue)
107 => :string, # ExtendedResponse
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change in place, the reply would cause an exception to be raised when it was being parsed.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is fine if it doesn't break other queries

@ldap_result = {
:resultCode => sequence[0],
:matchedDN => sequence[1],
:errorMessage => sequence[2],
}
@extended_response = sequence[3]
@extended_response = sequence.last
Copy link
Member

Choose a reason for hiding this comment

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

What's the idea here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an optional field that's included when the target is Active Directory that shifts the index. This accounts for both cases by accessing it as the last member.

Copy link
Member

Choose a reason for hiding this comment

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

OK, as long as it works with other LDAP directories

@zeroSteiner
Copy link
Contributor Author

zeroSteiner commented Oct 29, 2024

Let me add the scripts I used for testing. IIRC I tested both with OpenLDAP and Active Directory.

test_password_change.rb
$LOAD_PATH.unshift 'lib'

require 'rubygems'
require 'net/ldap'
require 'pry'

openldap = {
     :host => '127.0.0.1',
     :port => 389,
     :auth => {
           :method => :simple,
           :username => "cn=admin,dc=example,dc=org",
           :password => "adminpassword"
     }
}


ldap = Net::LDAP.new openldap
ldap.bind
result = ldap.password_modify(
  dn: 'uid=johndoe,ou=users,dc=example,dc=org',
  #old_password: 'admin',
  new_password: 'Password1!'
)
puts "password_modify: #{result}"
test_whoami.rb
$LOAD_PATH.unshift 'lib'

%Q{
podman run --rm --name openldap \
  -p 1389:1389 \
  --env LDAP_ADMIN_USERNAME=admin \
  --env LDAP_ADMIN_PASSWORD=adminpassword \
  --env LDAP_USERS=customuser \
  --env LDAP_PASSWORDS=custompassword \
  --env LDAP_ROOT=dc=example,dc=org \
  --env LDAP_ADMIN_DN=cn=admin,dc=example,dc=org \
  bitnami/openldap:2.5.17
}

require 'rubygems'
require 'net/ldap'
require 'pry'

openldap = {
     :host => '127.0.0.1',
     :port => 1389,
     :auth => {
           :method => :simple,
           :username => "cn=admin,dc=example,dc=org",
           :password => "adminpassword"
     }
}

openldap2 = {
     :host => '127.0.0.1',
     :port => 389,
     :auth => {
           :method => :simple,
           :username => "uid=johndoe,ou=users,dc=example,dc=org",
           :password => "password123"
     }
}

adds = {
     :host => '192.168.159.10',
     :port => 389,
     :auth => {
           :method => :simple,
           :username => "CN=Alice Liddle,CN=Users,DC=msflab,DC=local",
           :password => "Password1!"
     }
}

ldap = Net::LDAP.new openldap
ldap.bind
puts "[openldap]  whoami: #{ldap.whoami}"

#ldap = Net::LDAP.new openldap2
#ldap.bind
#puts "[openldap2] whoami: #{ldap.whoami}"

ldap = Net::LDAP.new adds
ldap.bind
puts "[adds]      whoami: #{ldap.whoami}"

@HarlemSquirrel
Copy link
Member

Thanks for sharing the manual test results. Do you think you can get these tests passing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants