network: skip socket options and source address for UDS client connec…

…tions (envoyproxy#4252)

There are no meaningful semantics for socket options and UDS in Envoy; there is a single socket option for UDS, SO_PASSCRED, which doesn't seem to be interesting. Since socket options and source address are specified at cluster level, and a cluster might have a mix of IP and UDS hosts, we need to silently skip rather than reject the config.

As a bonus, cleaned up time sources to be fully mocked in connection_impl_test, since we needed this for the new unit tests.

Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10015.

Risk level: Low
Testing: Unit tests and corpus entry added.

Signed-off-by: Harvey Tuch <htuch@google.com>

source/common/network/connection_impl.cc

@@ -542,28 +542,32 @@ ClientConnectionImpl::ClientConnectionImpl(
     const Network::ConnectionSocket::OptionsSharedPtr& options)
     : ConnectionImpl(dispatcher, std::make_unique<ClientSocketImpl>(remote_address),
                      std::move(transport_socket), false) {
-  if (!Network::Socket::applyOptions(options, *socket_,
-                                     envoy::api::v2::core::SocketOption::STATE_PREBIND)) {
-    // Set a special error state to ensure asynchronous close to give the owner of the
-    // ConnectionImpl a chance to add callbacks and detect the "disconnect".
-    immediate_error_event_ = ConnectionEvent::LocalClose;
-    // Trigger a write event to close this connection out-of-band.
-    file_event_->activate(Event::FileReadyType::Write);
-    return;
-  }
-
-  if (source_address != nullptr) {
-    const Api::SysCallIntResult result = source_address->bind(fd());
-    if (result.rc_ < 0) {
-      ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(),
-                     strerror(result.errno_));
-      bind_error_ = true;
+  // There are no meaningful socket options or source address semantics for
+  // non-IP sockets, so skip.
+  if (remote_address->ip() != nullptr) {
+    if (!Network::Socket::applyOptions(options, *socket_,
+                                       envoy::api::v2::core::SocketOption::STATE_PREBIND)) {
       // Set a special error state to ensure asynchronous close to give the owner of the
       // ConnectionImpl a chance to add callbacks and detect the "disconnect".
       immediate_error_event_ = ConnectionEvent::LocalClose;
-
       // Trigger a write event to close this connection out-of-band.
       file_event_->activate(Event::FileReadyType::Write);
+      return;
+    }
+
+    if (source_address != nullptr) {
+      const Api::SysCallIntResult result = source_address->bind(fd());
+      if (result.rc_ < 0) {
+        ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(),
+                       strerror(result.errno_));
+        bind_error_ = true;
+        // Set a special error state to ensure asynchronous close to give the owner of the
+        // ConnectionImpl a chance to add callbacks and detect the "disconnect".
+        immediate_error_event_ = ConnectionEvent::LocalClose;
+
+        // Trigger a write event to close this connection out-of-band.
+        file_event_->activate(Event::FileReadyType::Write);
+      }
     }
   }
 }

test/common/network/connection_impl_test.cc

@@ -77,8 +77,8 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest,
                         TestUtility::ipTestParamsToString);
 
 TEST_P(ConnectionImplDeathTest, BadFd) {
-  DangerousDeprecatedTestTime test_time;
-  Event::DispatcherImpl dispatcher(test_time.timeSource());
+  MockTimeSource time_source;
+  Event::DispatcherImpl dispatcher(time_source);
   EXPECT_DEATH_LOG_TO_STDERR(
       ConnectionImpl(dispatcher, std::make_unique<ConnectionSocketImpl>(-1, nullptr, nullptr),
                      Network::Test::createRawBufferSocket(), false),
@@ -89,7 +89,7 @@ class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
 public:
   void setUpBasicConnection() {
     if (dispatcher_.get() == nullptr) {
-      dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
+      dispatcher_.reset(new Event::DispatcherImpl(time_source_));
     }
     listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false);
 
@@ -152,7 +152,7 @@ class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
 
     MockBufferFactory* factory = new StrictMock<MockBufferFactory>;
     dispatcher_.reset(
-        new Event::DispatcherImpl(test_time_.timeSource(), Buffer::WatermarkFactoryPtr{factory}));
+        new Event::DispatcherImpl(time_source_, Buffer::WatermarkFactoryPtr{factory}));
     // The first call to create a client session will get a MockBuffer.
     // Other calls for server sessions will by default get a normal OwnedImpl.
     EXPECT_CALL(*factory, create_(_, _))
@@ -169,7 +169,7 @@ class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
   }
 
 protected:
-  DangerousDeprecatedTestTime test_time_;
+  MockTimeSource time_source_;
   Event::DispatcherPtr dispatcher_;
   Stats::IsolatedStoreImpl stats_store_;
   Network::TcpListenSocket socket_{Network::Test::getAnyAddress(GetParam()), nullptr, true};
@@ -233,7 +233,7 @@ TEST_P(ConnectionImplTest, CloseDuringConnectCallback) {
 }
 
 TEST_P(ConnectionImplTest, ImmediateConnectError) {
-  dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
+  dispatcher_.reset(new Event::DispatcherImpl(time_source_));
 
   // Using a broadcast/multicast address as the connection destiantion address causes an
   // immediate error return from connect().
@@ -805,7 +805,7 @@ TEST_P(ConnectionImplTest, BindFailureTest) {
     source_address_ = Network::Address::InstanceConstSharedPtr{
         new Network::Address::Ipv6Instance(address_string, 0)};
   }
-  dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
+  dispatcher_.reset(new Event::DispatcherImpl(time_source_));
   listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false);
 
   client_connection_ = dispatcher_->createClientConnection(
@@ -1199,7 +1199,7 @@ class ReadBufferLimitTest : public ConnectionImplTest {
 public:
   void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size) {
     const uint32_t buffer_size = 256 * 1024;
-    dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
+    dispatcher_.reset(new Event::DispatcherImpl(time_source_));
     listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false);
 
     client_connection_ = dispatcher_->createClientConnection(
@@ -1270,8 +1270,8 @@ TEST_P(ReadBufferLimitTest, SomeLimit) {
 
 class TcpClientConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
 protected:
-  TcpClientConnectionImplTest() : dispatcher_(test_time_.timeSource()) {}
-  DangerousDeprecatedTestTime test_time_;
+  TcpClientConnectionImplTest() : dispatcher_(time_source_) {}
+  MockTimeSource time_source_;
   Event::DispatcherImpl dispatcher_;
 };
 INSTANTIATE_TEST_CASE_P(IpVersions, TcpClientConnectionImplTest,
@@ -1309,5 +1309,33 @@ TEST_P(TcpClientConnectionImplTest, BadConnectConnRefused) {
   dispatcher_.run(Event::Dispatcher::RunType::Block);
 }
 
+class PipeClientConnectionImplTest : public testing::Test {
+protected:
+  PipeClientConnectionImplTest() : dispatcher_(time_source_) {}
+  MockTimeSource time_source_;
+  Event::DispatcherImpl dispatcher_;
+  const std::string path_{TestEnvironment::unixDomainSocketPath("foo")};
+};
+
+// Validate we skip setting socket options on UDS.
+TEST_F(PipeClientConnectionImplTest, SkipSocketOptions) {
+  auto option = std::make_shared<MockSocketOption>();
+  EXPECT_CALL(*option, setOption(_, _)).Times(0);
+  auto options = std::make_shared<Socket::Options>();
+  options->emplace_back(option);
+  ClientConnectionPtr connection = dispatcher_.createClientConnection(
+      Utility::resolveUrl("unix://" + path_), Network::Address::InstanceConstSharedPtr(),
+      Network::Test::createRawBufferSocket(), options);
+  connection->close(ConnectionCloseType::NoFlush);
+}
+
+// Validate we skip setting source address.
+TEST_F(PipeClientConnectionImplTest, SkipSourceAddress) {
+  ClientConnectionPtr connection = dispatcher_.createClientConnection(
+      Utility::resolveUrl("unix://" + path_), Utility::resolveUrl("tcp://1.2.3.4:5"),
+      Network::Test::createRawBufferSocket(), nullptr);
+  connection->close(ConnectionCloseType::NoFlush);
+}
+
 } // namespace Network
 } // namespace Envoy

test/mocks/common.cc

@@ -10,6 +10,9 @@ MockSystemTimeSource::~MockSystemTimeSource() {}
 MockMonotonicTimeSource::MockMonotonicTimeSource() {}
 MockMonotonicTimeSource::~MockMonotonicTimeSource() {}
 
+MockTimeSource::MockTimeSource() : TimeSource(system_, monotonic_) {}
+MockTimeSource::~MockTimeSource() {}
+
 MockTokenBucket::MockTokenBucket() {}
 MockTokenBucket::~MockTokenBucket() {}
 

test/mocks/common.h

@@ -51,6 +51,15 @@ class MockMonotonicTimeSource : public MonotonicTimeSource {
   MOCK_METHOD0(currentTime, MonotonicTime());
 };
 
+class MockTimeSource : public TimeSource {
+public:
+  MockTimeSource();
+  ~MockTimeSource();
+
+  MockSystemTimeSource system_;
+  MockMonotonicTimeSource monotonic_;
+};
+
 class MockTokenBucket : public TokenBucket {
 public:
   MockTokenBucket();

test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5632902623657984

@@ -0,0 +1 @@
+static_resources {   clusters {     name: " "     connect_timeout {       nanos: 6     }     hosts {       pipe {         path: " "       }     }     health_checks {       timeout {         nanos: 6       }       interval {         nanos: 6       }       unhealthy_threshold {       }       healthy_threshold {       }       tcp_health_check {       }     }   } } cluster_manager {   upstream_bind_config {     source_address {       address: "0.0.0.0"       port_value: 0     }     freebind {       value: true     }   } } admin {   address {     pipe {       path: " "     }   } }