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

New version of Enable PAM_RHOST patch #6

Merged
merged 4 commits into from Mar 2, 2020
Merged

Conversation

lmctv
Copy link
Contributor

@lmctv lmctv commented Oct 25, 2016

Originally submitted as

https://bugzilla.cyrusimap.org/show_bug.cgi?id=3468

The patch changes saslauthd's protocol ABI, I think this should lead to an increase in SONAME's ABI_VERSION field.

Contrary to what I said in the original bugzilla submission, the patch have been in production use since 5 years, on some of the systems I'm managing, in particular, on a frontend system getting more than 100 thousand login attempts each day, which makes for more than 200 million SASL login operations till now.

The protocol as implemented in testsaslauthd has been updated too, this solves
the issue Petr Lautrbach wrote about in

https://bugzilla.redhat.com/show_bug.cgi?id=683797#c12

While the changes are really limited, I'm hereby expressly authorizing
redistribution and use under the original file's license.

In detail:

  1. lib/checkpw.c: 4 clause bsd-like license *cmu
  2. saslauthd/auth_dce.{c,h}: 2 clause bsd-like license - *md
  3. saslauthd/auth_getpwent.{c,h}: 2 clause bsd-like license - *md
  4. saslauthd/auth_httpform.{c,h}: 2 clause bsd-like license - *pe
  5. saslauthd/auth_krb4.{c,h}: 2 clause bsd-like license - *md
  6. saslauthd/auth_krb5.{c,h}: 2 clause bsd-like license - *md
  7. saslauthd/auth_ldap.{c,h}: 2 clause bsd-like license - *ib
  8. saslauthd/auth_pam.{c,h}: 2 clause bsd-like license - *fk
  9. saslauthd/auth_rimap.{c,h}: 2 clause bsd-like license - *md
  10. saslauthd/auth_sasldb.{c,h}: 2 clause bsd-like license - *md
  11. saslauthd/auth_shadow.{c,h}: 2 clause bsd-like license - *md
  12. saslauthd/auth_sia.{c,h}: 2 clause bsd-like license - *md
  13. saslauthd/ipc_doors.c: 2 clause bsd-like license - *md
  14. saslauthd/ipc_unix.c: 2 clause bsd-like license - *md
  15. saslauthd/mechanisms.h: 2 clause bsd-like license - *md
  16. saslauthd/saslauthd-main.{c,h}: 2 clause bsd-like license - *md

*cmu - Carnegie Mellon University.
*md - Messaging Direct Ltd.
*pe - Pyx Engineering AG
*ib - Igor Brezac
*fk - Fabian Knittel

As for the copyright, I would be equally satisfied, at sasl project's
representative's sole option, either if it would be assigned to the respective
file's copyright owner, or if a statement was added in each of the modified .c
files declaring my own copyright on the changed portions.

JanParcel and others added 4 commits October 18, 2016 10:42
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
Originally submitted as

    https://bugzilla.cyrusimap.org/show_bug.cgi?id=3468

The patch changes saslauthd's protocol ABI, and will require a soname change.

The protocol as implemented in testsaslauthd has been updated too, this solves
the issue Petr Lautrbach wrote about in

    https://bugzilla.redhat.com/show_bug.cgi?id=683797#c12
@ksmurchison
Copy link
Contributor

I think I'd like to hold off on this until a 2.2 release. I don't want to change the wire protocol in a patch version

@lmctv
Copy link
Contributor Author

lmctv commented Nov 23, 2016

@ksmurchison: Fair enough; this is perfectly in-line with my suggestion to defer the integration to a soname change time.

In the meanwhile, we've been testing the patched code on the following non-linux systems:

  • solaris 11
  • solaris 10
  • freebsd 11

and I can confirm the patched code worked correctly throughout 150,000 failed and good logins on each of the tested systems.

@cepheid666
Copy link

@ksmurchison Do you know when 2.2 might come out? The underlying problem (#346) is still an issue and it'd be great to have saslauthd log rhost in pam like other services do. Would this patch also require updates to various saslauthd-client software like sendmail, or would saslauthd pick up the rhost automatically?

Given the age of this issue (back to at least 2011), and the last comment in this thread being over 2 years old, might you reconsider implementing this in a patch version?

In any case, it would be amazing to have this fixed so blocking software like fail2ban can operate properly for things like SMTP brute-force auth attack. (I know there's a LogLevel workaround, but that comes with other prices...)

Thanks!

@lmctv
Copy link
Contributor Author

lmctv commented Mar 21, 2019

[...] Would this patch also require updates to various saslauthd-client software like sendmail, or
would saslauthd pick up the rhost automatically?

With this patch in, the connection parameters are correctly inserted in the pam environment without any further configuration action.

@cepheid666
Copy link

With this patch in, the connection parameters are correctly inserted in the pam environment without any further configuration action.

Excellent news. So then the question is whether we might be able to get this released prior to 2.2, given how old this issue is? Or, is there an easy way to implement it without an official release? (e.g., is there a branch with this applied already, or a pre-release SVN?)

Thanks!

@quanah
Copy link
Contributor

quanah commented Feb 27, 2020

@ksmurchison any objection to this getting merged into mainline, since the 2.1 release branch is separate?

@ksmurchison
Copy link
Contributor

Go for it

@quanah quanah merged commit f769dde into cyrusimap:master Mar 2, 2020
@cepheid666
Copy link

Can package maintainers backport this fix into RHEL/CentOS 7 and 8?

@quanah
Copy link
Contributor

quanah commented Mar 2, 2020

@cepheid666 Should probably file a bug in the RHEL bugzilla for that.

@cepheid666
Copy link

cepheid666 commented Mar 2, 2020

Right, will do, didn't know if the package maintainers are here also. But will do. Thanks!

aiobofh pushed a commit to aiobofh/cyrus-sasl that referenced this pull request Mar 4, 2020
New version of Enable PAM_RHOST patch
azat added a commit to azat-ch/cyrus-sasl that referenced this pull request Apr 27, 2021
MSan reports [1]:

    Uninitialized bytes in __interceptor_getaddrinfo at offset 20 inside [0x7febe03d1ec0, 48)
    ==7==WARNING: MemorySanitizer: use-of-uninitialized-value
        #0 0x3bcc351d in get_fqhostname obj-x86_64-linux-gnu/../contrib/cyrus-sasl/lib/saslutil.c:548:9
        ClickHouse#1 0x3bcf6e7f in sasl_client_new obj-x86_64-linux-gnu/../contrib/cyrus-sasl/lib/client.c:515:7
        cyrusimap#2 0x3bb91c19 in rd_kafka_sasl_cyrus_client_new obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_sasl_cyrus.c:500:13
        cyrusimap#3 0x3bacfd9d in rd_kafka_sasl_client_new obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_sasl.c:265:13
        cyrusimap#4 0x3b689f83 in rd_kafka_broker_connect_auth obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:2263:8
        cyrusimap#5 0x3b6a4075 in rd_kafka_broker_handle_SaslHandshake obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:2199:2
        cyrusimap#6 0x3b70c8e3 in rd_kafka_buf_callback obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_buf.c:480:17
        cyrusimap#7 0x3b681189 in rd_kafka_req_response obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:1775:9
        cyrusimap#8 0x3b681189 in rd_kafka_recv obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:1893:3
        cyrusimap#9 0x3bb18ff2 in rd_kafka_transport_io_event obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_transport.c:742:11
        cyrusimap#10 0x3bb18ff2 in rd_kafka_transport_io_serve obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_transport.c:801:17
        cyrusimap#11 0x3b6bd7b3 in rd_kafka_broker_ops_io_serve obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:3380:21
        cyrusimap#12 0x3b6adb2d in rd_kafka_broker_consumer_serve obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:4961:17
        cyrusimap#13 0x3b6adb2d in rd_kafka_broker_serve obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:5066:17
        cyrusimap#14 0x3b694799 in rd_kafka_broker_thread_main obj-x86_64-linux-gnu/../contrib/librdkafka/src/rdkafka_broker.c:5208:25
        cyrusimap#15 0x3bb86676 in _thrd_wrapper_function obj-x86_64-linux-gnu/../contrib/librdkafka/src/tinycthread.c:576:9
        cyrusimap#16 0x7fecf4eaf608 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)
        cyrusimap#17 0x7fecf4dd6292 in clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292)

      Uninitialized value was created by an allocation of 'hints' in the stack frame of function 'get_fqhostname'
        #0 0x3bcc31b0 in get_fqhostname obj-x86_64-linux-gnu/../contrib/cyrus-sasl/lib/saslutil.c:523

    SUMMARY: MemorySanitizer: use-of-uninitialized-value obj-x86_64-linux-gnu/../contrib/cyrus-sasl/lib/saslutil.c:548:9 in get_fqhostname

  [1]: https://clickhouse-test-reports.s3.yandex.net/0/f7edcdf7c847f7d45ceede4a6073400ec2c985b7/integration_tests_(memory).html

Note that we can use __msan_unpoison() instead, but this function does
not looks hot, so it does not worth the complexity.
@quanah quanah added this to the 2.2.0 milestone Sep 22, 2021
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.

None yet

5 participants