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

Jump to bottom

Fix 4 problems in SASL kerberos #7008

Closed
wants to merge 4 commits into from
Closed

Fix 4 problems in SASL kerberos #7008

wants to merge 4 commits into from

Conversation

monnerat
Copy link
Contributor

@monnerat monnerat commented May 4, 2021

These are all in Curl_auth_create_gssapi_security_message, both in gssapi and sspi versions:

  1. Byte order was not handled correctly: was bad for a big endian architecture.
  2. Security layer negotiation flags were resent unchanged to the server: as we don't support such a layer (SASL is only used for authentication), mask off integrity and privacy layer flags.
  3. An authorization identity was always derived from the context's user name, which is useless and a possible source of error for the server. Replace it by the provided SASL authzid or nothing.
  4. The authorization identity was sent with a zero byte terminator, although RFC 4752 specifies it should not. This was justified by a comment but no reference to this need has been found. In addition, some servers include this extra byte in the real identity string causing failure of the authentication. As cyrus-sasl ang libgsasl do not include this byte, it is dropped here too.

@jay
Copy link
Member

jay commented May 5, 2021 •
edited

4. The authorization identity was sent with a zero byte terminator, although RFC 4752 specifies it should not. This was justified by a comment but no reference to this need has been found. In addition, some servers include this extra byte in the real identity string causing failure of the authentication. As cyrus-sasl ang libgsasl do not include this byte, it is dropped here too.

@captain-caveman2k

Ref: 7b29c28
Ref: https://curl.se/mail/lib-2021-04/0103.html

@bagder
Copy link
Member

bagder commented May 5, 2021

@monnerat have you thought anything about how we can add tests for this? We have too much gss/kerberos code that we can't test... 😢

@monnerat
Copy link
Contributor Author

monnerat commented May 5, 2021

@jay: I have bcc'd captain-caveman2k on https://curl.se/mail/lib-2021-04/0103.html without reaction. Is he still active?

@bagder: I've had some thinking about gss tests and gave up. I even doubt about an easy feasibility since it requires to have a valid ticket in the kerberos cache (obtained externally before use).

@bagder
Copy link
Member

bagder commented May 5, 2021

I even doubt about an easy feasibility since it requires to have a valid ticket in the kerberos cache (obtained externally before use).

If we'd have our own test server, such a ticket would be no problem. If we'd write stubs for all the gss functions to use when testing we could do other shortcuts.

@monnerat
Copy link
Contributor Author

monnerat commented May 6, 2021

If we'd write stubs for all the gss functions to use when testing we could do other shortcuts.

You mean something similar to libhostname.so, but for gss_* functions, right? In that case we probably wouln't need a kerberos dummy server: we can include fixed responses in http/smtp/etc server data like it's done for other authentication methods.
Writing the stubs is not a small job!
And it won't work for sspi builds.

@bagder
Copy link
Member

bagder commented May 6, 2021

Sure, it is. But having this much functionality in curl and not even the smallest way to test and verify it is... crazy.

I presume you've verified the functionality of your changes here by running curl code against some actual real servers?

@monnerat
Copy link
Contributor Author

monnerat commented May 6, 2021

I presume you've verified the functionality of your changes here by running curl code against some actual real servers?

Sure. As part of implementing SASL (binary) in openldap, I've started to test the available authentication methods.
I've set-up a Linux VM with MIT kerberos kdc, saslauthd and slapd servers, then tested the available authentication methods against it (EXTERNAL, PLAIN, LOGIN, NTLM, DIGEST-MD5, CRAM-MD5, GSSAPI: other curl featured SASL mechanisms are not supported by slapd). This led me finding the NTLM bug 9c1e1a6 and the problems fixed in the current PR. I spent several days tracking the later as I'm not familiar with GSSAPI/Kerberos.

@iboukris
Copy link
Contributor

iboukris commented May 8, 2021

Note that we already got some stubs at tests/libtest/stub_gssapi.c.
Also, see this script to setup a kdc with both Heimdal and MIT: https://github.com/openldap/openldap/blob/master/tests/scripts/setup_kdc.sh

@monnerat monnerat force-pushed the kerberos branch 2 times, most recently from e648e57 to 6b797e0 Compare May 18, 2021 22:16
@monnerat monnerat mentioned this pull request Jun 2, 2021
Closed
@bagder
Copy link
Member

bagder commented Jun 6, 2021

I think these changes should've been done as separate PRs so that we could review and discuss the changes separately. Consider splitting things up in the future. Is this series ready for merge in your opinion?

What does "auth: we do not support a security layer after kerberos authentication" mean for someone not into kerberos specifics?

@monnerat
Copy link
Contributor Author

monnerat commented Jun 6, 2021

What does "auth: we do not support a security layer after kerberos authentication" mean for someone not into kerberos specifics?

SASL GSSAPI features an optional transmission layer assuring data integrity/confidentiality. If negotiated, this layer is active after the negotiation succeeds, in the same way STARTTLS adds an encryption layer.
In curl, SASL is only used for authentication and there's no support (yet) in imap.c, pop3.c and smtp.c for such a layer.
As a consequence, the GSSAPI negotiation should clear the corresponding flags before returning them to the server.

There's a similar sequence in

curl/lib/socks_gssapi.c

Lines 339 to 350 in a0709f9

gss_enc = 0; /* no data protection */
/* do confidentiality protection if supported */
if(gss_ret_flags & GSS_C_CONF_FLAG)
gss_enc = 2;
/* else do integrity protection */
else if(gss_ret_flags & GSS_C_INTEG_FLAG)
gss_enc = 1;
infof(data, "SOCKS5 server supports GSS-API %s data protection.\n",
(gss_enc == 0)?"no":((gss_enc==1)?"integrity":"confidentiality"));
/* force for the moment to no data protection */
gss_enc = 0;

@monnerat
Copy link
Contributor Author

monnerat commented Jun 6, 2021

Is this series ready for merge in your opinion?

Unless someone gripes about this, yes. This works for me and I don't plan other changes around it.
I just wonder how it worked before: see #7156.

@monnerat monnerat force-pushed the kerberos branch 2 times, most recently from 7c9b63e to a62cf5d Compare July 1, 2021 14:06
@ghost
Copy link

ghost commented Jul 19, 2021 •
edited by ghost

Congratulations 🎉. DeepCode analyzed your code in 2.956 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

@monnerat
auth: properly handle byte order in kerberos security message
2e3f67e
@monnerat
auth: we do not support a security layer after kerberos authentication
5adb2e9
@monnerat
auth: use sasl authzid option in kerberos
635e55a 
... instead of deriving it from active ticket.
@monnerat
auth: do not append zero-terminator to authorisation id in kerberos
526e87b 
RFC4752 Section 3.1 states "The authorization identity is not terminated
with a zero-valued (%x00) octet". Although a comment in code said it may
be needed anyway, nothing confirms it. In addition, servers may consider
it as part of the identity, causing a failure.
@bagder bagder closed this in 3f9b1d0 Aug 16, 2021
bagder pushed a commit that referenced this pull request Aug 16, 2021
@monnerat @bagder
auth: we do not support a security layer after kerberos authentication
0a1c85e 
Closes #7008
bagder pushed a commit that referenced this pull request Aug 16, 2021
@monnerat @bagder
auth: use sasl authzid option in kerberos
396a2d7 
... instead of deriving it from active ticket.
Closes #7008
bagder pushed a commit that referenced this pull request Aug 16, 2021
@monnerat @bagder
auth: do not append zero-terminator to authorisation id in kerberos
7da2990 
RFC4752 Section 3.1 states "The authorization identity is not terminated
with a zero-valued (%x00) octet". Although a comment in code said it may
be needed anyway, nothing confirms it. In addition, servers may consider
it as part of the identity, causing a failure.

Closes #7008
@bagder
Copy link
Member

bagder commented Aug 16, 2021

Thanks!

@monnerat
Copy link
Contributor Author

Thanks for pulling!

@monnerat monnerat deleted the kerberos branch August 16, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
authentication
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@monnerat @jay @bagder @iboukris