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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃崄 UDP: Cache and reply from the same interface and IP #106

Closed
wants to merge 1 commit into from
Closed

馃崄 UDP: Cache and reply from the same interface and IP #106

wants to merge 1 commit into from

Conversation

database64128
Copy link

Previously, outline-ss-server simply writes the UDP response, which gets routed via the default gateway.

This behavior becomes a problem, when a server has multiple IP addresses of the same address family. Response UDP packets may get routed via a different interface or IP than the incoming packets. At the client's point of view, the outgoing UDP socket received packets from another IP. Most firewalls simply drop such packets.

This commit borrows the idea from https://github.com/WireGuard/wireguard-go/blob/master/conn/bind_linux.go, where IP_PKTINFO and IPV6_PKTINFO are used to determine the interface and IP where the incoming packets are from. Then we send the response using the information as socket out-of-band data to inform the kernel how to route the packets.

Currently it's only implemented for Linux. Implementing it for other platforms is simply not worth the effort, in my opinion.

@database64128 database64128 marked this pull request as draft September 15, 2021 16:12
@database64128 database64128 marked this pull request as ready for review September 16, 2021 03:27
Copy link

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Before we get too deep into this, could you clarify your use case?
Are you running outline-ss-server directly or the standard Outline Server?
Why do you need multiple interfaces?
Can you bind to a specific interface or address instead?

@@ -420,8 +428,8 @@ func (m *natmap) Close() error {
var maxAddrLen int = len(socks.ParseAddr("[2001:db8::1]:12345"))

// copy from target to client until read timeout
func timedCopy(clientAddr net.Addr, clientConn net.PacketConn, targetConn *natconn,
keyID string, sm metrics.ShadowsocksMetrics) {
func timedCopy(clientAddr *net.UDPAddr, clientConn onet.UDPPacketConn, oobCache []byte,
Copy link

Choose a reason for hiding this comment

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

The NAT will tie the targetConn and the clientConn, so every packet from targetConn will go to the same clientConn.

My understanding was that once you get the first packet the socket would be bound to the IP that received that packed. Is that not the case?

If it's not the case, this change can be made way less intrusive by implementing a PacketConn that has that behavior. Right now your PR is spreading a bunch of implementation details around, while we just need to change how the packet connection works.

Copy link
Author

Choose a reason for hiding this comment

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

This change does not affect the behavior of targetConn.

This change can be made way less intrusive by implementing a PacketConn that has that behavior.

I don't think this is possible. The write calls needs the OOB data to know which interface and IP to send from. You can't implement a PacketConn with the knowledge of that.

Copy link

Choose a reason for hiding this comment

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

Why not?

  1. Extend UDPConn and add a field to hold the address to use
  2. Write a ListenUDP that will capture the address
  3. Replace WriteTo to use the saved address

service/udp_linux.go Show resolved Hide resolved
}

func getOobForCache4(clientOob4 []byte) []byte {
cmsg := (*oob4)(unsafe.Pointer(&clientOob4))
Copy link

Choose a reason for hiding this comment

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

The unsafe operations worry me. They may make the server less stable and less safe.
Can you use https://pkg.go.dev/golang.org/x/net/ipv4 or https://pkg.go.dev/golang.org/x/net/ipv6 instead?

Copy link
Author

Choose a reason for hiding this comment

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

These unsafe operations are mostly identical with wireguard-go's implemention. I'm fairly confident these operations are safe. The OOB data is generated by the kernel, so there shouldn't be any security concerns.

}
}

func getOobForCache(clientOob []byte) []byte {
Copy link

Choose a reason for hiding this comment

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

Can't you get the address with PacketConn.LocalAddr()?
https://pkg.go.dev/net#PacketConn

Copy link
Author

Choose a reason for hiding this comment

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

You can't. Because our clientConn listens on ::.

Copy link

Choose a reason for hiding this comment

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

I haven't looked into this in great detail, but I think the cleanest solution here would be to launch a separate UDPService for each network interface, each holding a clientConn bound to one server IP address.

Copy link

Choose a reason for hiding this comment

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

I agree. Or simply bind to a single interface. @database64128 is support for multiple interfaces needed? Why?

@database64128
Copy link
Author

Before we get too deep into this, could you clarify your use case?
Are you running outline-ss-server directly or the standard Outline Server?
Why do you need multiple interfaces?

We first identified this issue with UDP on some of our shadowbox deployments with multiple interfaces. Basically, outbound UDP packets are always sent via the gateway interface, causing them to be dropped by client firewall if the client is connecting from a non-gateway interface.

Suppose we have interface eth0 with 2001:db8:: and eth1 with 2001:db8::1. eth1 is the default gateway. When a client connects via 2001:db8::, or eth0, the client firewall expects incoming UDP packets are all from 2001:db8::. But the current implementation simply calls clientConn.WriteTo for packed response. The response gets routed via the default gateway, and the client receives UDP packets from 2001:db8::1. The firewall drops these packets because they are from a different IP than the original dial target.

Can you bind to a specific interface or address instead?

I'm not trying to bind the clientConn to any specific interface. I simply changed the write call to write from the interface and IP where the packed is received.

}
}

func getOobForCache(clientOob []byte) []byte {
Copy link

Choose a reason for hiding this comment

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

I agree. Or simply bind to a single interface. @database64128 is support for multiple interfaces needed? Why?

@@ -420,8 +428,8 @@ func (m *natmap) Close() error {
var maxAddrLen int = len(socks.ParseAddr("[2001:db8::1]:12345"))

// copy from target to client until read timeout
func timedCopy(clientAddr net.Addr, clientConn net.PacketConn, targetConn *natconn,
keyID string, sm metrics.ShadowsocksMetrics) {
func timedCopy(clientAddr *net.UDPAddr, clientConn onet.UDPPacketConn, oobCache []byte,
Copy link

Choose a reason for hiding this comment

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

Why not?

  1. Extend UDPConn and add a field to hold the address to use
  2. Write a ListenUDP that will capture the address
  3. Replace WriteTo to use the saved address

func ListenUDP(network string, laddr *net.UDPAddr) (onet.UDPPacketConn, error) {
lc := &net.ListenConfig{
Control: func(network, address string, c syscall.RawConn) error {
return c.Control(func(fd uintptr) {
Copy link

Choose a reason for hiding this comment

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

Can you use https://pkg.go.dev/syscall#Bind or https://pkg.go.dev/syscall#BindToDevice at this point so you don't need to change the write code?

Copy link
Author

Choose a reason for hiding this comment

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

Or simply bind to a single interface.

How do you determine which interface to bind to? Most likely the user wants to bind to all interfaces.

Can you use https://pkg.go.dev/syscall#Bind or https://pkg.go.dev/syscall#BindToDevice at this point so you don't need to change the write code?

SO_BINDTODEVICE does not allow you to specify a source IP. If an interface has multiple IP addresses of the same address family, outbound packets are always sent via the default IP. IP_PKTINFO allows you to set a source IP in addition to an interface index. With the source IP setting, you don't have to bind the socket to an IP to send from that IP.

Why not?

For every interface you would have to create one clientConn. My implementation creates one clientConn that listens on all interfaces and IPs. This is also how wireguard-go handles multiple interfaces.

Previously, outline-ss-server simply writes the UDP response, which gets routed via the default gateway.

This behavior becomes a problem, when a server has multiple IP addresses of the same address family. Response UDP packets may get routed via a different interface or IP than the incoming packets. At the client's point of view, the outgoing UDP socket received packets from another IP. Most firewalls simply drop such packets.

This commit borrows the idea from https://github.com/WireGuard/wireguard-go/blob/master/conn/bind_linux.go, where IP_PKTINFO and IPV6_PKTINFO are used to determine the interface and IP where the incoming packets are from. Then we send the response using the information as socket out-of-band data to inform the kernel how to route the packets.

Currently it's only implemented for Linux. Implementing it for other platforms is simply not worth the effort, in my opinion.
bemasc pushed a commit that referenced this pull request Dec 24, 2021
As discussed in #106, a UDP socket bound to 0.0.0.0 results in
pathological behavior when there are multiple interfaces of the same
address family.  This change binds a separate instance of the UDP
service to each interface IP, ensuring that outbound Shadowsocks packets
have the expected source IP.
@bemasc
Copy link

bemasc commented Dec 24, 2021

@database64128 Would #109 work for your purpose?

That implementation is perhaps less memory-efficient than yours, but it's entirely safe, platform-independent, and seems easier to reason about. I think the overhead (which should be much less than 1 MB of RAM) is not anything to worry about.

@database64128
Copy link
Author

database64128 commented Dec 24, 2021

but it's entirely safe

The unsafe part in this PR is exactly the same as in wireguard-go.

Thank you for your proposed changes, but I'm going to close this PR, since I have been maintaining and using my own fork for a long time. Shadowsocks-NET/outline-ss-server contains this fix, as well as a few other changes:

  1. Only drain on decryption failure (including early EOF). If the connection to the remote target is unexpectedly closed, or the SOCKS address header is malformed, we should immediately close the connection, instead of draining.
  2. Allow access to localhost and private IPs by default (like every other implementation). Opt in to blocking these targets with a flag.
  3. Add support for TCP Fast Open with my tfo-go. Lots of unsafe uses there, even had to wrap my own system calls on some platforms.

I'm planning to introduce some protocol changes in my fork. Backwards compatibility is just not a priority for me. My goals include changing key derivation method to drop sha1 and md5, adding timestamps to header to allow for proper replay protection, new optional UDP mode with handshake so we don't have to derive a new session key for every UDP packet, new optional UDP-over-TCP mode.

@database64128 database64128 deleted the udp-interface branch December 24, 2021 02:02
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.

None yet

3 participants