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

Handle mutual authentication #36

Merged
merged 1 commit into from
Nov 4, 2014
Merged

Conversation

mkomitee
Copy link
Collaborator

Make certain that responses always pass through handle_other() to provide mutual
authentication before returning them to the user.

This fixes #35.

Since this is security related, once we get a few people to verify it doesn't break anything, we should cut a new release.

Note: It may cause requests which were succeeding to fail if users were requiring mutual authentication but servers weren't providing it.

Make certain that responses always pass through handle_other() to provide mutual
authentication before returning them to the user.
@mkomitee
Copy link
Collaborator Author

mkomitee commented Nov 4, 2014

If it's not clear, I don't have permission to merge this and it's a pretty bad security flaw. Can someone take a peek & merge if it's acceptable?

sigmavirus24 added a commit that referenced this pull request Nov 4, 2014
Handle mutual authentication
@sigmavirus24 sigmavirus24 merged commit 208bebb into requests:master Nov 4, 2014
@sigmavirus24
Copy link
Contributor

@mkomitee should we get a CVE identifier for this?

@mkomitee
Copy link
Collaborator Author

mkomitee commented Nov 4, 2014

I have absolutely no idea what the process would be around that, or what kind of issues warrant one.

@sigmavirus24
Copy link
Contributor

You said this is a security flaw. Can you explain what kind of security flaw it is?

@mkomitee
Copy link
Collaborator Author

mkomitee commented Nov 4, 2014

It was explained in #35:

I noticed today that we're not actually authenticating the server responses. In handle_401 we call authenticate_user and return the resulting response to the caller, handle_response, which passes it back to the user. Under normal circumstances, we never call handle_other, which is where mutual authentication occurs.

I'm a bad person and I feel bad.

Users typically use kerberos w/ HTTP to authenticate a client to a server, but it also allows the client to verify that the server is who it claims to be, much like SSL/TLS, but without encrypting the request in transit.

This is called mutual authentication.

In designing this library, I made sure that the client could declare its intention to require the server to authenticate successfully, and defaulted it to requiring it.

This bug, however, prevented the mutual authentication code from being executed, so it's possible that users think they're talking to a trusted server, but they're not.

It's similar to bugs that prevent clients from verifying the trust of a servers SSL certificate.

... with this fix in place we begin to behave as I originally intended: by default, the caller won't even receive a response from a server that fails mutual authentication. It's rather strict, so the fix may "break" "working code".

@sigmavirus24
Copy link
Contributor

So I'm going to request a CVE. I'm just waiting to hear if I should wait for the identifier before cutting a new release or if I should just cut the release and request the identifier at the same time.

@sigmavirus24
Copy link
Contributor

We were assigned CVE-2014-8650

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.

Not providing mutual authentication
3 participants