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

New option: Add server certs to client chain #1014

Merged
merged 9 commits into from Mar 17, 2016

Conversation

Projects
None yet
2 participants
@ikoz
Contributor

ikoz commented Mar 8, 2016

If enabled, append all server certificates to the certificate chain
served to the client, as extras. Can be used to test/bypass certain
certificate pinning impementations.

New option: Add server certs to client chain
If enabled, append all server certificates to the certificate chain
served to the client, as extras. Can be used to bypass certain
certificate pinning impementations.
@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Mar 9, 2016

Member

Thanks for the great PR! 😃
Could you add a test which verifies that the feature is working as intended?

Member

mhils commented Mar 9, 2016

Thanks for the great PR! 😃
Could you add a test which verifies that the feature is working as intended?

@ikoz

This comment has been minimized.

Show comment
Hide comment
@ikoz

ikoz Mar 10, 2016

Contributor

Some more context on this was just made public:

https://www.cigital.com/blog/ineffective-certificate-pinning-implementations/
related: CVE-2016-2402

@mhils What we need for a test is to implement the following scenario:

  1. start a web server (443, certificate signed by some self-signed CA)
  2. start mitmproxy using the new option
  3. start a 'client' and configure it to use the mitmproxy
  4. use the 'client' to connect to the web server through mitmproxy
  5. use the 'client' to retrieve the certificates mitmproxy sends
  6. if this list includes the certificates the real server is using, then the test succeeds.

Unfortunately I'm not very familiar with the mitmproxy test suite, so I am not sure where to start to implement this. Is the a similar test case already?

Contributor

ikoz commented Mar 10, 2016

Some more context on this was just made public:

https://www.cigital.com/blog/ineffective-certificate-pinning-implementations/
related: CVE-2016-2402

@mhils What we need for a test is to implement the following scenario:

  1. start a web server (443, certificate signed by some self-signed CA)
  2. start mitmproxy using the new option
  3. start a 'client' and configure it to use the mitmproxy
  4. use the 'client' to connect to the web server through mitmproxy
  5. use the 'client' to retrieve the certificates mitmproxy sends
  6. if this list includes the certificates the real server is using, then the test succeeds.

Unfortunately I'm not very familiar with the mitmproxy test suite, so I am not sure where to start to implement this. Is the a similar test case already?

@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Mar 10, 2016

Member

Really nice find, congratulations! 😃

test/mitmproxy/test_server.py#L419 is the closest we have right now I think. ssloptions configures the server, f.sslinfo is what we get on the client. You can pass additional arguments to the proxy by implementing get_proxy_config (just grep for it in the tests). Let me know if you run into any issues.

Member

mhils commented Mar 10, 2016

Really nice find, congratulations! 😃

test/mitmproxy/test_server.py#L419 is the closest we have right now I think. ssloptions configures the server, f.sslinfo is what we get on the client. You can pass additional arguments to the proxy by implementing get_proxy_config (just grep for it in the tests). Let me know if you run into any issues.

@ikoz

This comment has been minimized.

Show comment
Hide comment
@ikoz

ikoz Mar 16, 2016

Contributor

I finally got around to create two tests for this, please review.

Also found a bug in the (unrelated) cert chain printing code which I was using for debugging, fixed it as well. Could have been a different pull but got mixed up in here.

The only thing missing, which I noticed fairly late, is that this option doesn't work when verify-upstream-cert is also in use. I'm not sure how to make these two options mutually exclusive - argparse groups?

Finally, I now also realize that I could rename 'server' and 'client' to 'upstream' and 'downstream' if that's better for consistency, let me know.

Contributor

ikoz commented Mar 16, 2016

I finally got around to create two tests for this, please review.

Also found a bug in the (unrelated) cert chain printing code which I was using for debugging, fixed it as well. Could have been a different pull but got mixed up in here.

The only thing missing, which I noticed fairly late, is that this option doesn't work when verify-upstream-cert is also in use. I'm not sure how to make these two options mutually exclusive - argparse groups?

Finally, I now also realize that I could rename 'server' and 'client' to 'upstream' and 'downstream' if that's better for consistency, let me know.

@@ -69,7 +70,7 @@ def __str__(self):
s = certutils.SSLCert(i)
if s.altnames:
parts.append("\tSANs: %s" % " ".join(s.altnames))
return "\n".join(parts)
return "\n".join(parts)

This comment has been minimized.

@mhils

mhils Mar 16, 2016

Member

Very good find, thanks! 😃

@mhils

mhils Mar 16, 2016

Member

Very good find, thanks! 😃

Show outdated Hide outdated test/mitmproxy/test_server.py
c1 = SSLCert.from_pem(d)
p = self.pathoc()
print("digest of p.cert[1]: %s"%p.server_certs[1].digest('sha256'))
print("digest of c1.cert[1]: %s"%c1.digest('sha256'))

This comment has been minimized.

@mhils

mhils Mar 16, 2016

Member

Can we remove the print statements from the tests please?

@mhils

mhils Mar 16, 2016

Member

Can we remove the print statements from the tests please?

This comment has been minimized.

@ikoz

ikoz Mar 16, 2016

Contributor

not sure how I missed that, will fix.

@ikoz

ikoz Mar 16, 2016

Contributor

not sure how I missed that, will fix.

Show outdated Hide outdated test/mitmproxy/test_server.py
server_cert_found_in_client_chain = True
break
assert(server_cert_found_in_client_chain == False)

This comment has been minimized.

@mhils

mhils Mar 16, 2016

Member

These look really good - thanks. How about replacing the last assert with == self.add_server_certs_to_client_chain and then moving all the common code in a mixin? (happy to help here if you are not sure how to do this)

@mhils

mhils Mar 16, 2016

Member

These look really good - thanks. How about replacing the last assert with == self.add_server_certs_to_client_chain and then moving all the common code in a mixin? (happy to help here if you are not sure how to do this)

This comment has been minimized.

@ikoz

ikoz Mar 16, 2016

Contributor

That seems like a good idea. Not sure what how mixin works, will try to understand.

@ikoz

ikoz Mar 16, 2016

Contributor

That seems like a good idea. Not sure what how mixin works, will try to understand.

@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Mar 16, 2016

Member

Good job - thanks for the high-quality PR!

The only thing missing, which I noticed fairly late, is that this option doesn't work when verify-upstream-cert is also in use.

Why is that the case? Any idea?

Finally, I now also realize that I could rename 'server' and 'client' to 'upstream' and 'downstream' if that's better for consistency, let me know.

I think it's just fine, thanks. ☺️

Member

mhils commented Mar 16, 2016

Good job - thanks for the high-quality PR!

The only thing missing, which I noticed fairly late, is that this option doesn't work when verify-upstream-cert is also in use.

Why is that the case? Any idea?

Finally, I now also realize that I could rename 'server' and 'client' to 'upstream' and 'downstream' if that's better for consistency, let me know.

I think it's just fine, thanks. ☺️

@ikoz

This comment has been minimized.

Show comment
Hide comment
@ikoz

ikoz Mar 16, 2016

Contributor

Why is that the case? Any idea?

Have to test this further, but the reasoning seems to be this: If the upstream chain is verified, then the API that retrieves the server's cert chain only retrieves the 'validated trusted path' (e.g. 1 or 2 certificates) - instead of 'all' the certificates that the server really sends down to the proxy (which what we need for this feature).

Contributor

ikoz commented Mar 16, 2016

Why is that the case? Any idea?

Have to test this further, but the reasoning seems to be this: If the upstream chain is verified, then the API that retrieves the server's cert chain only retrieves the 'validated trusted path' (e.g. 1 or 2 certificates) - instead of 'all' the certificates that the server really sends down to the proxy (which what we need for this feature).

Restructuring of the AddServerCertsToClientChain test so that it uses…
… a Mixin - also removed some extra printf statements
Show outdated Hide outdated test/mitmproxy/test_server.py
certs=[
("trusted-cert", servercert)
]
)

This comment has been minimized.

@mhils

mhils Mar 16, 2016

Member

Excellent. I think we can even move ssl, servercert and ssloptions into AddServerCertsToClientChainMixin ? 😃

@mhils

mhils Mar 16, 2016

Member

Excellent. I think we can even move ssl, servercert and ssloptions into AddServerCertsToClientChainMixin ? 😃

@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Mar 16, 2016

Member

Have to test this further, but the reasoning seems to be this: If the upstream chain is verified, then the API that retrieves the server's cert chain only retrieves the 'validated trusted path' (e.g. 1 or 2 certificates) - instead of 'all' the certificates that the server really sends down to the proxy (which what we need for this feature).

OpenSSL ¯_(ツ)_/¯

Let's just make it a mutually exclusive group then.

(We don't get a GitHub notification for pushed commits. Please leave a quick comment if things are ready. 😃)

Member

mhils commented Mar 16, 2016

Have to test this further, but the reasoning seems to be this: If the upstream chain is verified, then the API that retrieves the server's cert chain only retrieves the 'validated trusted path' (e.g. 1 or 2 certificates) - instead of 'all' the certificates that the server really sends down to the proxy (which what we need for this feature).

OpenSSL ¯_(ツ)_/¯

Let's just make it a mutually exclusive group then.

(We don't get a GitHub notification for pushed commits. Please leave a quick comment if things are ready. 😃)

ikoz added some commits Mar 16, 2016

@ikoz

This comment has been minimized.

Show comment
Hide comment
@ikoz

ikoz Mar 16, 2016

Contributor

Here we go.

One last thing: The no-upstream-cert feature is also obviously exclusive to this new feature, since the _establish_tls_with_client_and_server little dance doesn't happen if it's enabled.

Do you think no-upstream-cert should be moved into the mutually exclusive group?

Is no-upstream-cert mutually exclusive to verify-upstream-cert ?

It sounds like it is, but the underlying verify_options inside _establish_tls_with_server is reachable even if no-upstream-cert is enabled.

Contributor

ikoz commented Mar 16, 2016

Here we go.

One last thing: The no-upstream-cert feature is also obviously exclusive to this new feature, since the _establish_tls_with_client_and_server little dance doesn't happen if it's enabled.

Do you think no-upstream-cert should be moved into the mutually exclusive group?

Is no-upstream-cert mutually exclusive to verify-upstream-cert ?

It sounds like it is, but the underlying verify_options inside _establish_tls_with_server is reachable even if no-upstream-cert is enabled.

@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Mar 16, 2016

Member

Is no-upstream-cert mutually exclusive to verify-upstream-cert ?

I don't think it is or should be. no-upstream-cert merely denotes that the upstream cert/connection is not used/needed for generating the fake certificate, but we still want to retain the ability to verify it if there is an upstream connection. So, we have

add_upstream_certs_to_client_chain xor no_upstream_cert
add_upstream_certs_to_client_chain xor verify_upstream_cert

but no interdependency between no_upstream_cert and verify_upstream_cert. Now that we can't express that with exclusive groups, do we just want to do ditch these and do manual checks in mitmproxy.proxy.config?

Member

mhils commented Mar 16, 2016

Is no-upstream-cert mutually exclusive to verify-upstream-cert ?

I don't think it is or should be. no-upstream-cert merely denotes that the upstream cert/connection is not used/needed for generating the fake certificate, but we still want to retain the ability to verify it if there is an upstream connection. So, we have

add_upstream_certs_to_client_chain xor no_upstream_cert
add_upstream_certs_to_client_chain xor verify_upstream_cert

but no interdependency between no_upstream_cert and verify_upstream_cert. Now that we can't express that with exclusive groups, do we just want to do ditch these and do manual checks in mitmproxy.proxy.config?

ikoz added some commits Mar 16, 2016

Revert "Create mutually exclusive group for add-server-certs-to-clien…
…t-chain and verify-upstream-cert command line options. These are not meaningful together."

This reverts commit 02e3784.
Make the add-server-certs-to-client-chain and verify-upstream-cert op…
…tions mutually exclusive whily processing the proxy options. Do the same for the add-server-certs-to-client-chain and no-upstream-cert options.
@ikoz

This comment has been minimized.

Show comment
Hide comment
@ikoz

ikoz Mar 16, 2016

Contributor

Good point. Let's see if that works.

Contributor

ikoz commented Mar 16, 2016

Good point. Let's see if that works.

@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Mar 17, 2016

Member

Excellent. Thanks for the awesome PR! 😃 🍰 🎉

Member

mhils commented Mar 17, 2016

Excellent. Thanks for the awesome PR! 😃 🍰 🎉

mhils added a commit that referenced this pull request Mar 17, 2016

Merge pull request #1014 from ikoz/master
New option: Add server certs to client chain

@mhils mhils merged commit 983b0dd into mitmproxy:master Mar 17, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 93.321%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment