mirror/libsignal
0
0
Fork 0
mirror of https://github.com/signalapp/libsignal.git synced 2024-09-20 03:52:17 +02:00
Code

net: "Poison" the TCP connector if an invalid proxy is set

Browse Source 
This ensures that if there's an error setting a proxy, the previous
settings won't continue to be used for new connections.

This only applies to the Java, Swift, and TypeScript layers; the Rust
layer's set_proxy isn't a fallible API in the first place today. The
Java API now explicitly throws a checked IOException instead of
IllegalArgumentException.
This commit is contained in:
Jordan Rose 2024-05-08 11:20:27 -07:00
parent 62d88b1198
commit a09eb567f0
13 changed files with 156 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
java/client/src/main/java/org/signal/libsignal/net/Network.java
View File

@ -5,6 +5,8 @@
package org.signal.libsignal.net;
import static org.signal.libsignal.internal.FilterExceptions.filterExceptions;
import java.io.IOException;
import java.util.concurrent.ExecutionException;
import java.util.function.Consumer;
@ -46,8 +48,11 @@ public class Network {
*
* <p>Sets a domain name and port to be used to proxy all new outgoing connections. The proxy can
* be overridden by calling this method again or unset by calling {@link #clearProxy}.
*
* @throws IOException if the host or port are not (structurally) valid, such as a port that
* doesn't fit in u16.
*/
public void setProxy(String host, int port) {
public void setProxy(String host, int port) throws IOException {
this.connectionManager.setProxy(host, port);
}
@ -120,11 +125,10 @@ public class Network {
super(Native.ConnectionManager_new(env.value, userAgent));
}
private void setProxy(String host, int port) {
if (port == 0) {
throw new IllegalArgumentException("Port cannot be zero");
}
guardedRun(nativeHandle -> Native.ConnectionManager_set_proxy(nativeHandle, host, port));
private void setProxy(String host, int port) throws IOException {
filterExceptions(
IOException.class,
() -> guardedRunChecked(h -> Native.ConnectionManager_set_proxy(h, host, port)));
}
private void clearProxy() {

17
java/client/src/test/java/org/signal/libsignal/net/ChatServiceTest.java
View File

@ -7,8 +7,10 @@ package org.signal.libsignal.net;
import static org.junit.Assert.*;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import org.junit.Assume;
import org.junit.Test;
import org.signal.libsignal.internal.Native;
@ -139,4 +141,19 @@ public class ChatServiceTest {
chat.connectUnauthenticated().get();
chat.disconnect();
}
@Test
public void testConnectFailsWithInvalidProxy() throws Exception {
// The default TLS proxy config doesn't support staging, so we connect to production.
final Network net = new Network(Network.Environment.PRODUCTION, USER_AGENT);
assertThrows(IOException.class, () -> net.setProxy("signalfoundation.org", 0));
assertThrows(IOException.class, () -> net.setProxy("signalfoundation.org", 100_000));
assertThrows(IOException.class, () -> net.setProxy("signalfoundation.org", -1));
final ChatService chat = net.createChatService("", "");
// Make sure we *can't* connect.
final ExecutionException failure =
assertThrows(ExecutionException.class, () -> chat.connectUnauthenticated().get());
assertTrue(failure.getCause().toString(), failure.getCause() instanceof ChatServiceException);
}
}

2
java/shared/java/org/signal/libsignal/internal/Native.java
View File

@ -176,7 +176,7 @@ public final class Native {
public static native void ConnectionManager_Destroy(long handle);
public static native void ConnectionManager_clear_proxy(long connectionManager);
public static native long ConnectionManager_new(int environment, String userAgent);
public static native void ConnectionManager_set_proxy(long connectionManager, String host, int port);
public static native void ConnectionManager_set_proxy(long connectionManager, String host, int port) throws Exception;
public static native void CreateCallLinkCredentialPresentation_CheckValidContents(byte[] presentationBytes) throws Exception;
public static native void CreateCallLinkCredentialPresentation_Verify(byte[] presentationBytes, byte[] roomId, long now, byte[] serverParamsBytes, byte[] callLinkParamsBytes) throws Exception;

2
node/ts/net.ts
View File

@ -219,6 +219,8 @@ export class Net {
* Sets a domain name and port to be used to proxy all new outgoing
* connections. The proxy can be overridden by calling this method again or
* unset by calling {@link #clearProxy}.
*
* Throws if the host or port is structurally invalid, such as a port that doesn't fit in u16.
*/
setProxy(host: string, port: number): void {
Native.ConnectionManager_set_proxy(this.connectionManager, host, port);

15
node/ts/test/NetTest.ts
View File

@ -151,6 +151,21 @@ describe('chat service api', () => {
await chatService.disconnect();
}).timeout(10000);
});
it('cannot connect through an invalid proxy', async () => {
// The default TLS proxy config doesn't support staging, so we connect to production.
const net = new Net(Environment.Production, userAgent);
expect(() => net.setProxy('signalfoundation.org', 0)).throws(Error);
expect(() => net.setProxy('signalfoundation.org', 100_000)).throws(Error);
expect(() => net.setProxy('signalfoundation.org', -1)).throws(Error);
expect(() => net.setProxy('signalfoundation.org', 0.1)).throws(Error);
const chatService = net.newChatService();
// Make sure we *can't* connect.
await expect(chatService.connectUnauthenticated()).to.be.rejectedWith(
LibSignalErrorBase
);
}).timeout(10000);
});
describe('cdsi lookup', () => {

1
rust/bridge/shared/src/ffi/convert.rs
View File

@ -788,6 +788,7 @@ trivial!(bool);
macro_rules! ffi_arg_type {
(u8) => (u8);
(u16) => (u16);
(i32) => (i32);
(u32) => (u32);
(u64) => (u64);
(Option<u32>) => (u32);

3
rust/bridge/shared/src/jni/convert.rs
View File

@ -1244,6 +1244,9 @@ macro_rules! jni_arg_type {
(u16) => {
jni::jint
};
(i32) => {
jni::jint
};
(u32) => {
jni::jint
};

46
rust/bridge/shared/src/net.rs
View File

@ -28,7 +28,8 @@ use libsignal_net::env::{add_user_agent_header, Env, Svr3Env};
use libsignal_net::infra::connection_manager::MultiRouteConnectionManager;
use libsignal_net::infra::dns::DnsResolver;
use libsignal_net::infra::tcp_ssl::{
DirectConnector as TcpSslDirectConnector, TcpSslConnector, TcpSslConnectorStream,
DirectConnector as TcpSslDirectConnector, ProxyConnector as TcpSslProxyConnector,
TcpSslConnector, TcpSslConnectorStream,
};
use libsignal_net::infra::{make_ws_config, EndpointConnection};
use libsignal_net::svr::{self, SvrConnection};
@ -130,18 +131,40 @@ fn ConnectionManager_new(
fn ConnectionManager_set_proxy(
connection_manager: &ConnectionManager,
host: String,
port: AsType<NonZeroU16, u16>,
) {
let port = port.into_inner();
let proxy_addr = (host.as_str(), port);
port: i32,
) -> Result<(), std::io::Error> {
let mut guard = connection_manager
.transport_connector
.lock()
.expect("not poisoned");
match &mut *guard {
TcpSslConnector::Direct(direct) => *guard = direct.with_proxy(proxy_addr).into(),
TcpSslConnector::Proxied(proxied) => proxied.set_proxy(proxy_addr),
};
// We take port as an i32 because Java 'short' is signed and thus can't represent all port
// numbers, and we want too-large port numbers to be handled the same way as 0.
match u16::try_from(port)
.ok()
.and_then(|port| NonZeroU16::try_from(port).ok())
{
Some(port) => {
let proxy_addr = (host.as_str(), port);
match &mut *guard {
TcpSslConnector::Direct(direct) => *guard = direct.with_proxy(proxy_addr).into(),
TcpSslConnector::Proxied(proxied) => proxied.set_proxy(proxy_addr),
TcpSslConnector::Invalid(dns_resolver) => {
*guard = TcpSslProxyConnector::new(dns_resolver.clone(), proxy_addr).into()
}
};
Ok(())
}
None => {
match &*guard {
TcpSslConnector::Direct(TcpSslDirectConnector { dns_resolver, .. })
| TcpSslConnector::Proxied(TcpSslProxyConnector { dns_resolver, .. }) => {
*guard = TcpSslConnector::Invalid(dns_resolver.clone())
}
TcpSslConnector::Invalid(_dns_resolver) => (),
}
Err(std::io::ErrorKind::InvalidInput.into())
}
}
}
#[bridge_fn]
@ -152,8 +175,9 @@ fn ConnectionManager_clear_proxy(connection_manager: &ConnectionManager) {
.expect("not poisoned");
match &*guard {
TcpSslConnector::Direct(_direct) => (),
TcpSslConnector::Proxied(proxied) => {
*guard = TcpSslDirectConnector::new(proxied.dns_resolver.clone()).into()
TcpSslConnector::Proxied(TcpSslProxyConnector { dns_resolver, .. })
| TcpSslConnector::Invalid(dns_resolver) => {
*guard = TcpSslDirectConnector::new(dns_resolver.clone()).into()
}
};
}

3
rust/net/src/infra/errors.rs
View File

@ -14,6 +14,8 @@ pub trait LogSafeDisplay: Display {}
/// Errors that can occur during transport-level connection establishment.
#[derive(displaydoc::Display, Debug, thiserror::Error)]
pub enum TransportConnectError {
/// Invalid configuration for this connection
InvalidConfiguration,
/// Failed to establish TCP connection to any of the IPs
TcpConnectionFailed,
/// DNS lookup failed
@ -96,6 +98,7 @@ impl From<TransportConnectError> for std::io::Error {
fn from(value: TransportConnectError) -> Self {
use std::io::ErrorKind;
let kind = match value {
TransportConnectError::InvalidConfiguration => ErrorKind::InvalidInput,
TransportConnectError::TcpConnectionFailed => ErrorKind::ConnectionRefused,
TransportConnectError::SslFailedHandshake(_)
| TransportConnectError::SslError(_)

39
rust/net/src/infra/tcp_ssl.rs
View File

@ -30,13 +30,17 @@ const CONNECTION_ATTEMPT_DELAY: Duration = Duration::from_millis(200);
pub enum TcpSslConnector {
Direct(DirectConnector),
Proxied(ProxyConnector),
/// Used when configuring one of the other kinds of connector isn't possible, perhaps because
/// invalid configuration options were provided.
Invalid(DnsResolver),
}
impl TcpSslConnector {
pub fn set_ipv6_enabled(&mut self, ipv6_enabled: bool) {
let dns_resolver = match self {
TcpSslConnector::Direct(ref mut c) => &mut c.dns_resolver,
TcpSslConnector::Proxied(ref mut c) => &mut c.dns_resolver,
TcpSslConnector::Direct(c) => &mut c.dns_resolver,
TcpSslConnector::Proxied(c) => &mut c.dns_resolver,
TcpSslConnector::Invalid(resolver) => resolver,
};
dns_resolver.set_ipv6_enabled(ipv6_enabled);
}
@ -51,7 +55,7 @@ pub struct TcpSslConnectorStream(
#[derive(Clone)]
pub struct DirectConnector {
dns_resolver: DnsResolver,
pub dns_resolver: DnsResolver,
}
#[async_trait]
@ -323,6 +327,7 @@ impl TransportConnector for TcpSslConnector {
.connect(connection_params, alpn)
.await
.map(|s| s.map_stream(Either::Right)),
Self::Invalid(_) => Err(TransportConnectError::InvalidConfiguration),
}
.map(|s| s.map_stream(TcpSslConnectorStream))
}
@ -672,4 +677,32 @@ mod test {
make_http_request_response_over(stream).await;
}
#[tokio::test]
async fn connect_through_invalid() {
let (addr, server) = localhost_http_server();
let _server_handle = tokio::spawn(server);
let connector = TcpSslConnector::Invalid(DnsResolver::new_with_static_fallback(
HashMap::from([(SERVER_HOSTNAME, LookupResult::localhost())]),
));
let connection_params = ConnectionParams {
route_type: RouteType::Test,
sni: SERVER_HOSTNAME.into(),
host: addr.ip().to_string().into(),
port: addr.port().try_into().expect("bound port"),
http_request_decorator: HttpRequestDecoratorSeq::default(),
certs: RootCertificates::FromDer(Cow::Borrowed(SERVER_CERTIFICATE.cert.der())),
};
match connector.connect(&connection_params, Alpn::Http1_1).await {
Ok(_) => {
// We can't use expect_err() or assert_matches! because the success case isn't Debug.
panic!("should have failed");
}
Err(e) => {
assert_matches!(e, TransportConnectError::InvalidConfiguration);
}
}
}
}

10
swift/Sources/LibSignalClient/Net.swift
View File

@ -36,6 +36,8 @@ public class Net {
///
/// Sets a domain name and port to be used to proxy all new outgoing connections. The proxy can
/// be overridden by calling this method again or unset by calling ``Net/clearProxy()``.
///
/// - Throws: if the host or port is not structurally valid, such as a port of 0.
public func setProxy(host: String, port: UInt16) throws {
try self.connectionManager.setProxy(host: host, port: port)
}
@ -380,11 +382,9 @@ internal class ConnectionManager: NativeHandleOwner {
}
internal func setProxy(host: String, port: UInt16) throws {
if port == 0 {
throw SignalError.invalidArgument("port cannot be 0")
}
self.withNativeHandle {
failOnError(signal_connection_manager_set_proxy($0, host, port))
try self.withNativeHandle {
// We have to cast to Int32 because of how the port number is validated...for Java.
try checkError(signal_connection_manager_set_proxy($0, host, Int32(port)))
}
}

2
swift/Sources/SignalFfi/signal_ffi.h
View File

@ -1395,7 +1395,7 @@ SignalFfiError *signal_verify_signature(bool *out, SignalBorrowedBuffer cert_pem
SignalFfiError *signal_connection_manager_new(SignalConnectionManager **out, uint8_t environment, const char *user_agent);
SignalFfiError *signal_connection_manager_set_proxy(const SignalConnectionManager *connection_manager, const char *host, uint16_t port);
SignalFfiError *signal_connection_manager_set_proxy(const SignalConnectionManager *connection_manager, const char *host, int32_t port);
SignalFfiError *signal_connection_manager_clear_proxy(const SignalConnectionManager *connection_manager);

32
swift/Tests/LibSignalClientTests/ChatServiceTests.swift
View File

@ -3,15 +3,15 @@
// SPDX-License-Identifier: AGPL-3.0-only
//
// These testing endpoints aren't generated in device builds, to save on code size.
#if !os(iOS) || targetEnvironment(simulator)
import Foundation
@testable import LibSignalClient
import SignalFfi
import XCTest
final class ChatServiceTests: TestCaseBase {
// These testing endpoints aren't generated in device builds, to save on code size.
#if !os(iOS) || targetEnvironment(simulator)
private static let userAgent = "test"
private static let expectedStatus: UInt16 = 200
private static let expectedMessage = "OK"
@ -72,11 +72,13 @@ final class ChatServiceTests: TestCaseBase {
func testConvertError() throws {
do {
try checkError(signal_testing_chat_service_error_convert())
XCTFail("error not thrown")
} catch SignalError.connectionTimeoutError(_) {
// Okay
}
do {
try checkError(signal_testing_chat_service_inactive_error_convert())
XCTFail("error not thrown")
} catch SignalError.chatServiceInactive(_) {
// Okay
}
@ -106,6 +108,8 @@ final class ChatServiceTests: TestCaseBase {
}
}
#endif
func testConnectUnauth() async throws {
// Use the presence of the proxy server environment setting to know whether we should make network requests in our tests.
guard ProcessInfo.processInfo.environment["LIBSIGNAL_TESTING_PROXY_SERVER"] != nil else {
@ -142,6 +146,24 @@ final class ChatServiceTests: TestCaseBase {
try await chat.connectUnauthenticated()
try await chat.disconnect()
}
}
#endif
func testConnectFailsWithInvalidProxy() async throws {
// The default TLS proxy config doesn't support staging, so we connect to production.
let net = Net(env: .production, userAgent: Self.userAgent)
do {
try net.setProxy(host: "signalfoundation.org", port: 0)
XCTFail("should not allow setting invalid proxy")
} catch SignalError.ioError {
// Okay
}
let chat = net.createChatService(username: "", password: "")
// Make sure we *can't* connect.
do {
try await chat.connectUnauthenticated()
XCTFail("should not allow connecting")
} catch SignalError.connectionFailed {
// Okay
}
}
}