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

Make channel binding data per-host #180

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

geofft
Copy link
Contributor

@geofft geofft commented Feb 8, 2023

Previously, once you successfully generated channel binding data for one host, it would be stored in self.cbt_struct, and self.cbt_binding_tried would be set to True, preventing ever recalculating self.cbt_struct. This means that once you used an HTTPKerberosAuth object to talk to one host, it would keep sending that channel binding data to other hosts, resulting in failures for servers that enforce channel binding.

Instead, store CBTs in a per-host dictionary as we do with GSSAPI contexts.

@geofft
Copy link
Contributor Author

geofft commented Feb 8, 2023

Note that this is arguably a privacy vulnerability - you're sending the hash of the certificate of one server to all the other servers you talk to, and on the public internet those are indexed and easy to map back to a site (e.g., I bet you can just throw the hash into https://crt.sh). But it only gets leaked from one server you can get a Kerberos ticket for to another server you can also get a Kerberos ticket for, and most of the time your Kerberos-enabled servers are in a single trust domain, so I'm not sure it's a vulnerability for any real-world use cases.

Also, come to think of it, it might actually be better to compute the token right when it's needed.

@@ -214,7 +213,7 @@ def generate_request_header(self, response, host, is_preemptive=False):
username=self.principal,
hostname=kerb_host,
service=self.service,
channel_bindings=self.cbt_struct,
channel_bindings=self._cbts.get(host),
Copy link
Contributor

Choose a reason for hiding this comment

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

While it does this by default it would be good to specify the default to show we know that this could be None when it's not set

Suggested change
channel_bindings=self._cbts.get(host),
channel_bindings=self._cbts.get(host, None),

It can not be populated in the case of force_preemptive as that skips the handle_response branch.

@jborean93
Copy link
Contributor

Note that this is arguably a privacy vulnerability

You are providing a hash that is at least a sha256 of the public portion of the certificate which Kerberos then creates an MD4 hash of the CBT struct and embeds that inside the AP-REQ (Kerberos ticket) sent that is encrypted by the unique session key known only by the user and principal specified by the SPN. Putting aside that the public cert is publicly accessible you would really need a special case of scenarios for the actual recpipient to be able to decrypt that data and then reverse that hash (twice) into something that's actually useful.

Previously, once you successfully generated channel binding data for one
host, it would be stored in self.cbt_struct, and self.cbt_binding_tried
would be set to True, preventing ever recalculating self.cbt_struct.
This means that once you used an HTTPKerberosAuth object to talk to one
host, it would keep sending that channel binding data to other hosts,
resulting in failures for servers that enforce channel binding.

Instead, store CBTs in a per-host dictionary as we do with GSSAPI
contexts.
@geofft geofft force-pushed the fix-channel-binding branch from 8817e8d to 1ede418 Compare February 11, 2023 18:57
@geofft
Copy link
Contributor Author

geofft commented Feb 11, 2023

Updated with , None, thanks for the review! And yes, the only potential vulnerability here is that some Kerberized server (or someone who has compromised it) can figure out the fact that you talked to some other Kerberized server, which should be unlikely to be a problem in practice. (But it doesn't help that public certs are public - in fact it makes it easier to pre-compute the hashes and then compare the received hash against those.)

@jborean93 jborean93 merged commit 3f672cf into requests:master Feb 27, 2023
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.

2 participants