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

spnego_gssapi: implement TLS channel bindings for openssl #7196

Conversation

steffen-kiess
Copy link

This requires krb5 >= 1.19 because otherwise channel bindings won't be
forwarded through SPNEGO.

@bagder
Copy link
Member

bagder commented Jun 4, 2021

Run make checksrc locally first to make sure the code follows our style!

@bagder bagder added the TLS label Jun 4, 2021
@steffen-kiess steffen-kiess force-pushed the openssl-spnego-channel-binding branch from ce3cde8 to 7bf94d3 Compare June 7, 2021 10:29
@steffen-kiess
Copy link
Author

Run make checksrc locally first to make sure the code follows our style!

I've fixed the style problems and also fixed the -Werror=missing-field-initializers errors.

@bagder
Copy link
Member

bagder commented Nov 20, 2021

This has been lingering for much too long. Can you explain, in layman terms, exactly what this PR brings and why?

@steffen-kiess
Copy link
Author

Without channel binding SPNEGO can be attacked by reusing an authentication happening over http (in cleartext) to authenticate over https.

For example if there is a git client doing git clone http://[email protected]/repo git will (using libcurl) connect to example.org:80 and send the GSSAPI authentication data over the unecrypted channel (assuming that the server supports GSSAPI authentication and the client can find a kerberos service principal for HTTP/example.org). If an attacker can intercept this communication, he can connect to https://example.org/repo and reuse the captured authentication data. Without channel binding there is no indication for the server that the authentication data was originally intended for an unencrypted channel.

When the clients support channel binding the server can simple refuse GSSAPI authentication without channel binding (and refuse GSSAPI authentication over unencrypted http). The server can now distinguish between authentication data for http (which will be without channel binding data) and https (which will be with channel binding data).

In addition, the server will notice if the client's idea of the server's public key is incorrect (e.g. because the client is using curl -k) but this is probably not as important in most cases as distinguishing between http and https.

This way of using channel binding data for SPNEGO/GSSAPI is what e.g. Microsoft Edge will do also when connecting to a https server and sending GSSAPI authentication data.

@steffen-kiess
Copy link
Author

Note: Here is a bug report regarding implementing something similar in firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1179722 and the implementation in chromium https://codereview.chromium.org/1408433006/.

@Foorack
Copy link

Foorack commented Dec 14, 2021

Hello @steffen-kiess !

This requires krb5 >= 1.19 because otherwise channel bindings won't be forwarded through SPNEGO.

Apologies for the beginner question, but which package exactly do I need verify version of to test this patch?

$ apt search krb5 | grep krb5 | grep installed

krb5-config/focal,focal,now 2.6ubuntu1 all [installed]
krb5-doc/focal-updates,focal-updates,focal-security,focal-security,now 1.17-6ubuntu4.1 all [installed]
krb5-locales/focal-updates,focal-updates,focal-security,focal-security,now 1.17-6ubuntu4.1 all [installed,automatic]
krb5-multidev/focal-updates,focal-security,now 1.17-6ubuntu4.1 amd64 [installed]
krb5-user/focal-updates,focal-security,now 1.17-6ubuntu4.1 amd64 [installed]
libgssapi-krb5-2/focal-updates,focal-security,now 1.17-6ubuntu4.1 amd64 [installed,automatic]
libkrb5-26-heimdal/focal,now 7.7.0+dfsg-1ubuntu1 amd64 [installed,automatic]
libkrb5-3/focal-updates,focal-security,now 1.17-6ubuntu4.1 amd64 [installed,automatic]
libkrb5-dev/focal-updates,focal-security,now 1.17-6ubuntu4.1 amd64 [installed]
libkrb5support0/focal-updates,focal-security,now 1.17-6ubuntu4.1 amd64 [installed,automatic]
sssd-krb5/focal-updates,focal-security,now 2.2.3-3ubuntu0.8 amd64 [installed]
sssd-krb5-common/focal-updates,focal-security,now 2.2.3-3ubuntu0.8 amd64 [installed]

It looks like I am primarily running 1.17. Will upgrading a specific package like libkrb5-dev be enough or do I need to compile this in jammy?

@steffen-kiess
Copy link
Author

It looks like I am primarily running 1.17. Will upgrading a specific package like libkrb5-dev be enough or do I need to compile this in jammy?

I think upgrading only a single package from jammy will be difficult (all the krb5 packages depend on each other and they also depend on a new version of libc, I was unable to install versions from jammy in focal).

What I did (on focal) was to download and compile krb5:

wget https://web.mit.edu/kerberos/dist/krb5/1.19/krb5-1.19.2.tar.gz
tar xf krb5-1.19.2.tar.gz
cd krb5-1.19.2/src
./configure
make -j8

Then it should be possible to run the compiled curl version by setting LD_LIBRARY_PATH:

env LD_LIBRARY_PATH=path/to/krb5-1.19.2/src/lib ./src/curl -k --negotiate 'https://[email protected]/path'

@Foorack
Copy link

Foorack commented Dec 17, 2021

Hello @steffen-kiess ,

I just wanted to let you know that I've been able to verify this patch is working and solving the problem.

Unfortunately in my scenario it required obscene amount of debugging, but that was rather an underlying issue with the computer apparently not having joined the AD network correctly, and some SSSD and Kerberos configuration that required changing.

Also I ended up having to do:

./curl --negotiate --user : https://protectedomain.example.com/

Without --user : it would not authenticate.

I will now try start fresh with a new VM and see exactly which of these configuration changes are required and what the minimum-level amount of configuration is needed to get it working within the network (for example I ended up installing krb5 1.19 as system lib instead of linking against it via ENV var, I wanna see if it's possible without doing that), but that is not really related to this PR.

Thank you so much for the work on this PR! ❤️ Trying to google about information on "Ubuntu SSSD secure context curl" gives very limited information, debugging in this area is hard, and software support in Linux-world has been lacking. It took me several days to even find this PR, so would love to see this merged in.

@@ -4570,6 +4570,72 @@ static void ossl_disassociate_connection(struct Curl_easy *data,
}
}

static CURLcode ossl_get_tls_server_end_point(struct Curl_easy *data,
int sockindex, char **binding,
Copy link

Choose a reason for hiding this comment

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

C:\projects\curl\lib\vtls\openssl.c(4574,51): error C2220: the following warning is treated as an error [C:\projects\curl\lib\libcurl.vcxproj]
C:\projects\curl\lib\vtls\openssl.c(4574,51): warning C4100: 'sockindex': unreferenced formal parameter [C:\projects\curl\lib\libcurl.vcxproj]
C:\projects\curl\lib\vtls\openssl.c(4573,65): warning C4100: 'data': unreferenced formal parameter [C:\projects\curl\lib\libcurl.vcxproj]

AppVeyor is complaining about unused variables.

Copy link
Author

Choose a reason for hiding this comment

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

I think this should only happen if #if OPENSSL_VERSION_NUMBER > 0x10100000L is not satisfied. I can add (void) casts to ignore the parameters in the #else branch.

@Foorack
Copy link

Foorack commented Dec 17, 2021

It seems like it is no longer possible to view the Build log from Azure on why it is failing, probably because the build was over 6 months ago.

@steffen-kiess
Copy link
Author

It seems like it is no longer possible to view the Build log from Azure on why it is failing, probably because the build was over 6 months ago.

I can try rebasing the branch, then there should be a new build.

@steffen-kiess steffen-kiess force-pushed the openssl-spnego-channel-binding branch 2 times, most recently from 7a883aa to 0ad6f58 Compare December 17, 2021 18:27
@steffen-kiess
Copy link
Author

Ok, I've rebased to the current master, fixed the unreferenced formal parameter problem and also fixed a problem https://github.com/curl/curl/runs/4563490112?check_suite_focus=true (vtls/sectransp.c:3504:1: error: missing field 'get_tls_server_end_point' initializer [-Werror,-Wmissing-field-initializers]).

@Foorack
Copy link

Foorack commented Feb 4, 2022

This ended up being so big of a pain; it had consumed so much support time that IT eventually chose to simply disable the requirement for Secure Channel Binding...

Although, they are likely to want to turn this back on in a few years time. It would therefore be great if this PR could be merged so it would eventually start trickle down into a release, hopefully in time to make it into Ubuntu 22 if possible.

@bagder bagder added the needs-votes Pull-request in need of thumbs-ups to make progress label Jun 2, 2022
@DemiMarie
Copy link

@Foorack upvote it, or try to resolve the merge conflicts

@Foorack
Copy link

Foorack commented Oct 26, 2022

Hello @DemiMarie

Wow, it's been so long that I had forgotten this issue. Altough while they temporarily disabled the requirement for CSB, they will likely want to enable it again in the future, so it would still be appreciated for this to be merged as Channel Binding becomes more industry adopted.

Where do you want me to vote? Simple thumbs up on the main post?

@Foorack
Copy link

Foorack commented Oct 26, 2022

@steffen-kiess Can you please try rebase when you have time?

This requires krb5 >= 1.19 because otherwise channel bindings won't be
forwarded through SPNEGO.
unsigned int length;
u_char buf[EVP_MAX_MD_SIZE];

cert = SSL_get_peer_certificate(backend->handle);

Check failure

Code scanning / CodeQL

Certificate not checked

This call to SSL_get_peer_certificate is not followed by a call to SSL_get_verify_result.
@steffen-kiess
Copy link
Author

@steffen-kiess Can you please try rebase when you have time?

I've rebased the PR. (Basically no change, except that mesalink support was dropped.)

@Neustradamus
Copy link

Hi all,

Have you progressed on it?

@DemiMarie
Copy link

@Neustradamus I recommend reacting with thumbs-up to the PR

@SGA-max-faxalv
Copy link

@Neustradamus I recommend reacting with thumbs-up to the PR

Actually I am thankful for the email notification, as it reminded me of this project.

This issue temporarily stopped being a priority for me as IT disabled Enforced Secure Channel Binding (SCB) on the specific server, although this is an issue that will come back in the future. I am currently overloaded right now, but could possibly start look at this again next month.

The test environment is to set up two identical servers, one with SCB and one without, and then try reproduce the issue both on this PR-branch and on master branch.

@Neustradamus
Copy link

@SGA-max-faxalv, @Foorack: Thanks for your comment! :)

I have originally done a ticket here: #9226 about the RFC 9266: Channel Bindings for TLS 1.3 support.

TLS Channel Bindings uses:

  • tls-unique for TLS =< 1.2
  • tls-exporter for TLS = 1.3

About GNU SASL and SCRAM:

cc: @jas4711, @vszakats, @bagder.

@bagder
Copy link
Member

bagder commented Aug 6, 2023

I'm sorry but I see very little desire from users for this and the PR has now fallen behind and has lots of merge conflicts. Closing.

@bagder bagder closed this Aug 6, 2023
@Neustradamus
Copy link

@bagder: Little desire? It is sure if you do not want, it will not help!

TLS Channel Binding is important for security!

Have you seen the list here?

TLS Channel Binding is supported for example by GNU SASL, GnuTLS, Cyrus SASL/IMAPD, PostgreSQL, Exim, ...

What do you think about this feature and this PR?
@jas4711: The dev of GNU SASL (gsasl) who has done a good job for SCRAM and TLS Channel Binding.
@rufferson, @ZoltanFridrich, @ueno, jas4711 for GnuTLS.
@michaelpq, @danielgustafsson, @hlinnaka for @postgres.

Thanks in advance.

@Foorack
Copy link

Foorack commented Aug 7, 2023

This feature is important if curl should be able to talk to a web server which enforces secure channel binding on Kerberos auth (recommended by Microsoft!). I will start priorizing setting up a dev environment.

@Foorack
Copy link

Foorack commented Mar 11, 2024

@Neustradamus @steffen-kiess This has been re-based (did not apply cleanly due to refactoring in the last 2 years) and proposed as a new PR. #13098

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-votes Pull-request in need of thumbs-ups to make progress TLS
Development

Successfully merging this pull request may close these issues.

None yet

6 participants