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 a ldapsimple pwcheck plugin #468

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

Conversation

comicfans
Copy link

this mainly intend to be used under windows without permission to config
ldap server . for example setting up a svnserve on windows, but don't have
admin account for domain controller. no config needs to be done on ldap server side,
but have following limitations:

all user must under same DN
user DN must be written in config
not support admin bind-search then bind process

sample config:
pwcheck_method: ldapsimple
mech_list: PLAIN
ldapsimple_servers: ldap.forumsys.com
ldapsimple_dn: cn=%u,dc=example,dc=com
ldapsimple_port:389

currently this prototype only written and tested on windows, on other system you can use saslauthd to support full ldap authorization.

nacho and others added 30 commits June 24, 2016 15:18
saslmd5.h is already generated somewhere else so there is no need
to point to it here.
This reverts commit 1518b84.

The patch broke the msvc builds using nmake
This change prevent logging "no user in db" error messages
but set detailed error message for the case using ndbm as a
sasldb backend.
We have found some issues in saslauthd. We contacted various members who
maintain saslauthd and they suggested we report this to you so that
you can help to coordinate this issue with the saslauthd and other
consumers using this library. The details are below.

TLP:  AMBER

Libsasl2:  DOS attack possible on saslauthd deamon if configured with doors

Problem Description:

If "--with-ipctype=doors" is used, there are 2 types or stages of DOS
possible for saslauthd.  It may result in Segmentation Fault(core dump)
led by SIGSEGV, also it may result in leaking reference count under
certain circumstances.

CONFIGURE_OPTIONS += --with-ipctype=doors

The SIGSEGV can be triggered by running the PoC code as an unprivileged
user. There might be a similar case that could be created for the other
ipctype, this PoC only applies to the doors type.

When reference count is leaked but the SIGSEGV is prevented, it is
possible to lock up saslauthd daemon instead of killing it, which in a
way is worse, if there is a re-starter available.

The suggested fix addresses (1)the SIGSEGV, (2)the reference count
problem, and also (3) a mis-declaration issue which prevented compiling
the code on Solaris.

Using the PoC code (open-close-w-null.c) it was possible to prove that:

1.  The saslauthd can be DoS'ed by SIGSEGV.

2.  If SIGSEGV is prevented but the reference count link issue is not
fixed, it's possible to freeze saslauthd in a state where it has 1 or 0
threads and won't create any more because it thinks it already has the
maximum number of threads (usually 5).

There are a couple of curious things about the "simple-ugly" fix.  The
nice-complex fix keeps the thread count at maximum reference count,
which probably means it is a superior solution. However, that is for
upstream to ultimately decide.

A.  It's not clear why, with both fixes in (reference count and checking
the arguments) the thread gets killed, but it does NOT get killed if any
of the OTHER parameters that can be checked are
done wrong. For those cases, the thread appears to simply loop.

B. Sometimes saslauthd gets 1-off on the thread count, it shows 0
but still has 1 thread. But testing has not shown a way to get off by 2.

Both of these seem mild compared to the other 2 problems, but the
debugger didn't show exactly what is going on.

Both the simple-ugly fix and the nice-complex fix have been tested for
the core dump issue, and only the simple-ugly fix allows us to see the
reference-count issue, because it does not create a new thread until
there is a new connection request.

Our preference is for the project to release the fix by September 19, 2016.

TLP:  AMBER
Revert "Remove LIBSASL_API to sasldb"
saslauthd: Fix indentation to silent GCC6 warnings [-Wmisleading-indentation]
Add OpenSSL 1.1.0 support in saslauthd
ksmurchison and others added 16 commits July 13, 2017 10:19
…urns zero maxbufsize (patch from Sergio Gelato)
…corrupted authz name (using modified patch from wbclay)
this mainly intend to be used under windows without permission to config
ldap server . for example setting up a svnserve on windows, but don't have
 admin account for domain controller. no config needs to be done on ldap server side,
but have following limitations:

all user must under same DN
user DN must be written in config
not support admin bind-search then bind process

sample config:
pwcheck_method: ldapsimple
mech_list: PLAIN
ldapsimple_servers:  ldap.forumsys.com
ldapsimple_dn: cn=%u,dc=example,dc=com
ldapsimple_port:389
lib/checkpw.c Outdated
@@ -95,6 +95,7 @@
# endif
#endif

# include <winldap.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This include clearly breaks Unix platforms. So you need to wrap it in some form of #ifdef

Copy link
Author

Choose a reason for hiding this comment

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

should _WIN32 macro test enough ? if so I'll update this

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, ifdef _WIN32 should be enough.

@brong brong force-pushed the master branch 2 times, most recently from 3f420d3 to 64eda6e Compare January 30, 2018 09:21
@quanah
Copy link
Contributor

quanah commented Oct 5, 2021

@comicfans Unfortunately someone pushed master into your merge request, making it a complete mess. Can you either update this MR so it's correct, or create a new one with just your changes? Thanks!

It will need to be signed off on to comply with the DCO check as well.

@quanah quanah added the reviewed reviewed in triage label Oct 5, 2021
@Neustradamus
Copy link
Contributor

@comicfans: Have you seen the @quanah comment?

@comicfans
Copy link
Author

@comicfans: Have you seen the @quanah comment?
Hi @quanah @Neustradamus sorry for the late reply, this MR opened at 2017, because my working environment needs exactly this function, and I tested/used this function for a while. but currently I've changed my job (at 2018) so I don't have such testing environment, also lose the usage intention. so if possible, please close this. unfortunately, I don't have too much sparse time for this.

@williamdes
Copy link
Contributor

Can this PR be rebased so some testing can be done ?
I am interested in this plugin

@hyc
Copy link
Contributor

hyc commented Oct 8, 2022

You can examine the code without all the extraneous commits on the original branch https://github.com/comicfans/cyrus-sasl/tree/fix-msc-1900

I note that the ldap_init / ldap_connect APIs are deprecated, it should be using ldap_initialize() instead.

@Pfiver
Copy link
Contributor

Pfiver commented May 29, 2023

I'd love to have this kind of plugin available!

But I see a potential security issue in this implementation. (See comicfans@639f983#r115512817)

I also found a similar Idea (patch) from a mailing list contribution in 2001 (!) and am really wondering why that never made it to the trunk?

Use case: I have an openLDAP instance where I store hashed passwords - and thus cannot use the "auxprop" plugins. Or am I missing some other obvious solution?

@quanah
Copy link
Contributor

quanah commented Jul 23, 2024

Needs DCO and merge conflict updating. May be easier to close this PR and create a new one.

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

Successfully merging this pull request may close these issues.