Bug 260017 - security/cyrus-sasl2: Review and skim/remove patch-plugins_gssapi.c
Summary: security/cyrus-sasl2: Review and skim/remove patch-plugins_gssapi.c
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Hajimu UMEMOTO
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-24 14:35 UTC by Michael Osipov
Modified: 2021-11-28 04:28 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (ume)
michael.osipov: merge-quarterly?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2021-11-24 14:35:38 UTC
During an evaluation of a new server we have discovered (severe) issues with that custom patch:

> +#if 0
>     params->utils->log(params->utils->conn, SASL_LOG_DEBUG,
> 		       "GSSAPI client step %d", text->state);
> +#endif

Why are you taking away this debug output from me? I should have access to this as a developer.

> +	/*
> +	 * If caller didn't provide creds already.
> +	 *
> +	 * In the case of Kerberos, a client typically wants to use
> +	 * a credential in either a keytab file or the credentials cache
> +	 * of the current process context.  This code path will try to
> +	 * find a credential in the specified keytab file,  then the
> +	 * credentials cache.  The keytab file can be specified by
> +	 * "keytab" option, and it is configured by using
> +	 * gsskrb5_register_acceptor_identity() API when available.
> +	 */
> +	if (client_creds == GSS_C_NO_CREDENTIAL) {
> +	    GSS_LOCK_MUTEX_CTX(params->utils, text);
> +	    maj_stat = gss_acquire_cred(&min_stat,
> +					text->server_name,
> +					GSS_C_INDEFINITE,
> +					GSS_C_NO_OID_SET,
> +					GSS_C_INITIATE,
> +					&text->client_creds, 
> +					NULL, 
> +					NULL);
> +	    GSS_UNLOCK_MUTEX_CTX(params->utils, text);
> +
> +	    /*
> +	     * Ignore the error intentionally.  The credential was
> +	     * not found in the specified keytab file.
> +	     */
> +	    if (GSS_ERROR(maj_stat) == 0) {
> +		client_creds = text->client_creds;
> +	    }
> +	}
> +
> +	/* Try the credentials cache. */

This is problematic as a whole. I understand the reason why it was imported, but it is broken:

It acquires a client credential (GSS_C_INITIATE), but supplies the server name (text->server_name) for the target cred. That struct is likely memory garbage and will lead to a SIGSEGV. It has to be NULL. If you check the log of that file you see that it has been reverted in https://github.com/cyrusimap/cyrus-sasl/commit/26835b156ec9b69f22c3f15da9c3ab671b2d22dc for the obvious reasons also described in: https://github.com/cyrusimap/cyrus-sasl/pull/593

The next hunks suffer from the same broken logical approach and have been reverted ih the commit from above.

Ideally, the entire file is removed.
Comment 1 Michael Osipov 2021-11-24 14:36:40 UTC
This is also be applied to quarterly.
Comment 2 Michael Osipov 2021-11-24 14:37:15 UTC
Sample crash:
> debchaf047% lldb --core msktutil.core msktutil
> (lldb) target create "msktutil" --core "msktutil.core"
> Core file '/home/johannes.kunde/msktutil.core' (x86_64) was loaded.
> 
> 
> 
> (lldb) bt all
> * thread #1, name = 'msktutil', stop reason = signal SIGSEGV
> * frame #0: 0x00000008006ae62f libc.so.7`memcpy + 607
> frame #1: 0x00000008003831af libkrb5.so.3.3`___lldb_unnamed_symbol302$$libkrb5.so.3.3 + 127
> frame #2: 0x00000008017223af libkrb5.so.11`krb5_make_principal + 175
> frame #3: 0x00000008016a1db6 libgssapi_krb5.so.10`_gsskrb5_import_name + 278
> frame #4: 0x0000000801687c15 libgssapi.so.10`___lldb_unnamed_symbol4$$libgssapi.so.10 + 149
> frame #5: 0x0000000801684020 libgssapi.so.10`gss_acquire_cred + 448
> frame #6: 0x0000000801679b26 libgssapiv2.so`gssapi_client_mech_step + 1142
> frame #7: 0x0000000800974212 libsasl2.so.3`sasl_client_step + 226
> frame #8: 0x0000000800973b02 libsasl2.so.3`sasl_client_start + 146
> frame #9: 0x00000008002a8347 libldap_r-2.4.so.2`ldap_int_sasl_bind + 1575
> frame #10: 0x00000008002ab85b libldap_r-2.4.so.2`ldap_sasl_interactive_bind + 91
> frame #11: 0x00000008002aba0f libldap_r-2.4.so.2`ldap_sasl_interactive_bind_s + 127
> frame #12: 0x0000000000228599 msktutil`___lldb_unnamed_symbol129$$msktutil + 505
> frame #13: 0x0000000000212489 msktutil`___lldb_unnamed_symbol32$$msktutil + 1977
> frame #14: 0x0000000000213907 msktutil`___lldb_unnamed_symbol36$$msktutil + 247
> frame #15: 0x000000000021526a msktutil`___lldb_unnamed_symbol39$$msktutil + 4026
> frame #16: 0x000000000020f660 msktutil`___lldb_unnamed_symbol168$$msktutil + 256
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-11-28 04:15:16 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=17b54ce763286be358fae69961f5fb1a670c614c

commit 17b54ce763286be358fae69961f5fb1a670c614c
Author:     Hajimu UMEMOTO <ume@FreeBSD.org>
AuthorDate: 2021-11-28 04:08:48 +0000
Commit:     Hajimu UMEMOTO <ume@FreeBSD.org>
CommitDate: 2021-11-28 04:08:48 +0000

    security/cyrus-sasl2-gssapi: remove patch-plugins_gssapi.c

    PR:             260017
    Reported by:    Michael Osipov
    Discussed with: hrs
    MFH:            2021Q4

 security/cyrus-sasl2-gssapi/Makefile               |   2 +-
 .../files/patch-plugins_gssapi.c (gone)            | 117 ---------------------
 2 files changed, 1 insertion(+), 118 deletions(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-11-28 04:25:18 UTC
A commit in branch 2021Q4 references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=a8442f059c6bbdfaf43aa0f5f7f229393e49bc7c

commit a8442f059c6bbdfaf43aa0f5f7f229393e49bc7c
Author:     Hajimu UMEMOTO <ume@FreeBSD.org>
AuthorDate: 2021-11-28 04:08:48 +0000
Commit:     Hajimu UMEMOTO <ume@FreeBSD.org>
CommitDate: 2021-11-28 04:24:23 +0000

    security/cyrus-sasl2-gssapi: remove patch-plugins_gssapi.c

    PR:             260017
    Reported by:    Michael Osipov
    Discussed with: hrs
    MFH:            2021Q4

    (cherry picked from commit 17b54ce763286be358fae69961f5fb1a670c614c)

 security/cyrus-sasl2-gssapi/Makefile               |   2 +-
 .../files/patch-plugins_gssapi.c (gone)            | 117 ---------------------
 2 files changed, 1 insertion(+), 118 deletions(-)
Comment 5 Hajimu UMEMOTO freebsd_committer freebsd_triage 2021-11-28 04:28:23 UTC
Thanks committed.