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

Merged
merged 6 commits into from
Nov 24, 2024

Conversation

zeroSteiner
Copy link
Collaborator

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

@zeroSteiner zeroSteiner force-pushed the rfc4532 branch 2 times, most recently from 55f550f to 7a3dbc3 Compare November 22, 2024 23:42
Per RFC4511 section 4.12, the responseValue field of an ExtendedResponse object is an optional string.
Per RFC3062 section 2, the response to a passsword modify request is a sequence.
This means the extended response must be parsed.
@zeroSteiner
Copy link
Collaborator Author

@HarlemSquirrel tests are passing now! There was an issue with parsing the response from password-modify requests that deviated from the RFC which defines that it's in it's own format. The #extended_response attribute should be the unparsed data to closely adhere to the RFC spec, and extracting the password from it should require parsing it as per the other RFC spec.

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 working on this!

@HarlemSquirrel HarlemSquirrel merged commit a562790 into ruby-ldap:master Nov 24, 2024
6 checks passed
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