Prevent accidental anonymous binds #4784

Merged
merged 2 commits into from Oct 13, 2016

Projects

None yet

4 participants

@jonathon-k
Contributor

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

By design, or by bug, PHP's ldap_bind function will automatically attempt an anonymous bind if either the bind_rdn (username) or bind_password arguments are empty. This behavior may allow a non-existent user to authenticate to librenms with minimal (Level 1?) privileges.

Proposed fix -- Enforce a non-blank password when authenticating via ldap or active directory.

Jonathon Koyle added some commits Oct 12, 2016
@scrutinizer-notifier

The inspection completed: No new issues

@murrant
Contributor
murrant commented Oct 13, 2016

How are you triggering this? I can't reproduce.

@jonathon-k
Contributor

Does your AD reject anonymous binds?

Both @Gorian and and myself were able to put in a user name (real or fake)
with an empty password and it authenticated.

On Oct 13, 2016 8:33 AM, "Tony Murray" notifications@github.com wrote:

How are you triggering this? I can't reproduce.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4784 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUYaP2tjs0HEI4kaGC-c45ynzycPjdimks5qzkE3gaJpZM4KVQ2O
.

@murrant
Contributor
murrant commented Oct 13, 2016 edited

It seems to. My AD controllers are Win 2012 and Win 2012 R2. I thought blocking anonymous binds was standard AD behavior...

@jonathon-k
Contributor

It looks like it should be, I don't have permissions to our AD instance to
check its configuration... If we want to wait on this pull, I can ask one
of my domain admins when they get back from a conference...

On Oct 13, 2016 8:57 AM, "Tony Murray" notifications@github.com wrote:

It seems to. My AD controllers are Win 2012 and Win 2012 R2... I thought
blocking anonymous binds was standard AD behavior...


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4784 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUYaPz21gIEYmVraXgLMukx2YOMhWN47ks5qzkbSgaJpZM4KVQ2O
.

@jonathon-k
Contributor

Or if you are feeling particularly adventurous, and have the option, you could enable anonymous binds to test it. ;)

@murrant
Contributor
murrant commented Oct 13, 2016

Uh, looks like this setting has the issue:

$config['auth_ad_require_groupmembership'] = 0;

I have that set to 1 on my install.

@murrant murrant merged commit ba9672b into librenms:master Oct 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Gorian
Contributor
Gorian commented Oct 13, 2016

So, I set that to 0 on purpose - but the assumption is, if someone can successfully authenticate to the domain with a valid username and password, they can login. Not that a random string of characters in the "username" box would allow access. If people don't have an account on this particular AD, they shouldn't be able to login. I'm not sure if that's the expected behaviour and it's not communicated well in documentation, or if it's a bug in that particular feature...

@jonathon-k
Contributor

I believe @murrant meant that having that set to 1 is why he couldn't
reproduce it. He did merge the change.

On Oct 13, 2016 11:49 AM, "Layne" notifications@github.com wrote:

So, I set that to 0 on purpose - but the assumption is, if someone can
successfully authenticate to the domain with a valid username and password,
they can login. Not that a random string of characters in the "username"
box would allow access. If people don't have an account on this particular
AD, they shouldn't be able to login. I'm not sure if that's the expected
behaviour and it's not communicated well in documentation, or if it's a bug
in that particular feature...


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4784 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUYaP8OQaDjb0NkHir1_iFrOfjsqP-XZks5qzm8egaJpZM4KVQ2O
.

@Gorian
Contributor
Gorian commented Oct 13, 2016

ah, okay. Cool. As long as it's fixed ^.^

@murrant
Contributor
murrant commented Oct 13, 2016

Yes, having that set to 1 prevented the bug. Once I was able to reproduce, I had no problem merging. Thanks for the fix :)

@Gorian
Contributor
Gorian commented Oct 13, 2016

@murrant you rock. It's awesome to see you guys take care of things so fast :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment