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

http_client: Improve certificate verification #6197

Conversation

alessandrogario
Copy link
Member

@alessandrogario alessandrogario commented Jan 23, 2020

Fixes #6212

Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

This lgtm overall, did we address the best way to communicate this change to the community? Perhaps an announcement blog post on the site would be useful to alert users of this potential issue in their deployment?

osquery/remote/http_client.cpp Outdated Show resolved Hide resolved
@theopolis
Copy link
Member

Thanks for the bugfix! Can you please describe the conditions where the bug is present, what the bug is, and if there is security impact?

Is there a potential test to make sure the bug is mitigated?

@alessandrogario
Copy link
Member Author

Thanks for the bugfix! Can you please describe the conditions where the bug is present, what the bug is, and if there is security impact?

Is there a potential test to make sure the bug is mitigated?

This fixes an issue that has been reported by a user about how the server certificate is verified against the hostname osquery is trying to connect to. Without this callback, Boost always allows the connection regardless.

We can try to tweak the verification settings and see if we can just validate the hostname check in the tests!

@directionless
Copy link
Member

How does this intersect with --tls_server_certs? How do people toggling whether they want verification or not?

@muffins muffins closed this Jan 27, 2020
@muffins muffins reopened this Jan 27, 2020
@muffins
Copy link
Contributor

muffins commented Jan 29, 2020

This fixes #6212

@verntx
Copy link

verntx commented Jan 30, 2020

This will conflict with PR #6044. That change removes client_options_.sni_hostname_ and this PR has a dependency on it.

@muffins
Copy link
Contributor

muffins commented Jan 30, 2020

@verntx good call. I definitely like the restructuring in #6044, @alessandrogario is there anyway you could rebase on top of that PR and just keep the callback for the sni verification?

Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

So, looks like the ordering we'd desire here would be ship #6170, then ship #6044, then this PR, and work on getting some testing for this which should be relatively easy after #6170 lands. I'm going to request changes on this just to "remove it from the queue" until the other PRs ship.

@directionless directionless modified the milestones: 4.2.0, 4.2.1 Jan 30, 2020
@alessandrogario alessandrogario force-pushed the alessandro/bugfix/http-client-hostname-validation branch 2 times, most recently from bf3342f to e8f2cd2 Compare February 6, 2020 15:47
@alessandrogario alessandrogario added the do not merge Do not merge PR as it's pending on some discussion or external factor. Reviewer should have context. label Feb 6, 2020
@alessandrogario
Copy link
Member Author

This has been rebased on #6044 as requested; since that PR has not been merged yet, this branch also brought @theopolis commits (I've marked this as do-not-merge for now).

The fix is a one liner, so maybe it can be moved inside that other PR

@@ -203,8 +203,12 @@ void Client::createConnection() {

void Client::encryptConnection() {
boost::asio::ssl::context ctx{boost::asio::ssl::context::sslv23};
const auto remote_hostname = client_options_.remote_hostname_->c_str();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: lets make this a normal std::string, the rfc2818_verification constructor takes a string; I would then pass .c_str() to SSL_set_tlsext_host_name.

Smjert
Smjert previously requested changes Feb 10, 2020

if (client_options_.always_verify_peer_) {
ssl_sock_->set_verify_callback(
boost::asio::ssl::rfc2818_verification(remote_hostname));

Copy link
Member

@Smjert Smjert Feb 10, 2020

Choose a reason for hiding this comment

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

This has to be moved after line 246, otherwise ssl_sock_ is not initialized/null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally planned to close this PR, as we could just add this single line inside the base PR. Feel free to remove the do-not-merge tag if you want to update & merge this one!

Before this commit it was possible to have osquery accept
a valid certificate with a wrong hostname.
The commit fixes this adding the hostname verification.

Moreover:

- Regenerate test CA, server certificates and keys, so that
  the CA key is available, to be able to sign other test certs.
- Add a regression test for the hostname verification bug.

Co-Authored-By: Alessandro Gario <alessandro.gario@gmail.com>
@Smjert Smjert force-pushed the alessandro/bugfix/http-client-hostname-validation branch from e8f2cd2 to 3ae9a09 Compare February 12, 2020 16:09
Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

Mostly just questions, this overall looks great! Thanks a ton @Smjert for seeing this through to completion!

@@ -203,21 +203,27 @@ void Client::createConnection() {

void Client::encryptConnection() {
boost::asio::ssl::context ctx{boost::asio::ssl::context::sslv23};
const auto remote_hostname = *client_options_.remote_hostname_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be declared so high up? I'm not seeing anything making modifications to client_options_.remote_hostname_ or using remote_hostname until we check it in the callback. Could we remove this and just pass *client_options_.remote_hostname_ directly in the callback?

Copy link
Member

Choose a reason for hiding this comment

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

It's also used on line 250, but the main purpose I think was just to shorten the name.
I'll change it!

@@ -203,21 +203,27 @@ void Client::createConnection() {

void Client::encryptConnection() {
boost::asio::ssl::context ctx{boost::asio::ssl::context::sslv23};
const auto remote_hostname = *client_options_.remote_hostname_;

bool verify_hostname = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This kinda makes me nervous, as it seems to suggest we don't verify the hostname by default. Under what circumstances would we not want to verify? Wouldn't it just be with something like --tls_allow_unsafe=true or if the cert paths aren't set? Could we make this default to true and then only set to false if client_options_.always_verify_peer_ is false or the like? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Well hostname is not verified until we set the verify_mode to verify_peer in the context, so I was just following that functionality.
Also the context is local to the encryptConnection function, so it cannot change outside.

That been said I could envision a different problem indeed, where there's a new case where verify_peer is enabled and the verify hostname enabling is forgotten.
I guess I could just always set it; would've preferred to have it clearly showing that it depends on the verify_peer mode, but the context cannot be queried it seems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I think I'd be in favor of always setting it, but that's because I'm having a lot of trouble imaging scenarios where you wouldn't want this verification enabled, and more over I think leveraging the --tls_allow_unsafe=true should be a sufficient work around for anyone wanting to do testing and the like?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. I think I'd be in favor of always setting it, but that's because I'm having a lot of trouble imaging scenarios where you wouldn't want this verification enabled, and more over I think leveraging the --tls_allow_unsafe=true should be a sufficient work around for anyone wanting to do testing and the like?

Keep in mind that verify_peer already behaves like that in the context of the encryptConnection function: it's false by default and becomes true only if we explicitly set it.
I was reasoning completely in the context of that specific function because whatever external change is happening, it's not reflected in that function if not through the client_option settings.

I did not touch verify_peer because I was focusing on the hostname verification, but if we want to pursue this (correct) mentality, I'll change that to be enabled by default :).

Copy link
Contributor

Choose a reason for hiding this comment

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

it's false by default and becomes true only if we explicitly set it.

Ah ok, that makes sense. Perhaps for the sake of this PR let's leave things as they are then so that we're pattern matching. For changing the default behavior of this to be enabled by default, maybe let's start a broader discussion in Slack to get more folks to weigh in, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense!

@muffins muffins removed the do not merge Do not merge PR as it's pending on some discussion or external factor. Reviewer should have context. label Feb 12, 2020
Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

This looks good!

@Smjert Smjert dismissed their stale review February 12, 2020 19:52

It's my own PR now

@muffins muffins merged commit 498d64e into osquery:master Feb 12, 2020
@Smjert Smjert deleted the alessandro/bugfix/http-client-hostname-validation branch February 14, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tls: sni hostname is not verified
7 participants