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

Transparent proxy: bind_peer() fails when connection comes from a local address #379

Closed
tzvetkoff opened this issue Apr 12, 2023 · 11 comments

Comments

@tzvetkoff
Copy link
Contributor

Just as expected, if a connection comes from a local IP address, bind_peer() fails as the (IP, port) tuple is already in use by the original source connection.

My stupid solution is to ignore EADDRINUSE errors in bind_peer() but I wonder if there is a better way to do it.

I thought of checking if the IP address belongs to one of current NIFs but this seemed like a huge overkill.

WDYT?

@yrutschle
Copy link
Owner

Checking the IP address vs local addresses would also help solve #266.

I don't think your solution is all that bad: bind fails, so what? Arguably we don't even need to log it. I'll think about it some more :)

@tzvetkoff
Copy link
Contributor Author

Well, here's my patch: https://github.com/yrutschle/sslh/compare/master...tzvetkoff:sslh:fix-tproxy-local-addr-binds?expand=1
I hope I'll have the time to tinker a bit more these days too :-)

@pcrow
Copy link

pcrow commented Oct 5, 2023

Sorry, this is long, but I don't want to leave anything out:

That patch helped, but didn't completely solve my issues, but I've got a really weird setup:

External -> sslh:443 -> stunnel:441-> sslh:442 -> apache:80

I originally tried one sslh process, bouncing through it twice, but until I have it solid, I'm going to run two separate processes.

That allows multiple services on 443, and it allows multiple services wrapped in SSL, which is why I have stunnel stripping the SSL before it goes to Apache.
This works fine without transparent mode.
This works fine with the first sslh and stunnel running in transparent mode, and the last sslh running without transparent mode.
If the second sslh process is in transparent mode, it fails to forward to Apache, and I get the bind error messages:

common.c:281:bind:98:Address already in use
common.c:352:bind_peer:98:Address already in use
sslh-fork.c:114:connect: Address already in use

Adding the fix, those error messages go away, and the second sslh does forward to Apache as expected. However, the second sslh skips the transparent proxy.

With your fix, it does forward to Apache, and everything appears to work, but the last connection isn't transparent. This makes sense, as the bind error is real, and your fix ignores it instead of aborting, so the connection goes through, but the setting of the alternate source address never happens.

I added some code:

     else if (res == -1 ) {
       char host[256],serv[40];
       getnameinfo(from.ai_addr,from.ai_addrlen,host,sizeof(host),serv,sizeof(serv),NI_NUMERICHOST | NI_NUMERICSERV);
       print_message(msg_system_error, "%s:%d:bind for transparent address failed: %s:%s\n", __FILE__, __LINE__, host,serv);
    }

And as expected it shows that the original source IP:port

I dug through the code in stunnel, and while more convoluted, it seems to boil down the exact same logic, and it's perfectly happy being second in a transparent proxy chain, so I'm confused.

I also tried connecting directly to stunnel, eliminating the first sslh, and I get the same results.

So while your fix does clean up the error messages and connections work, it allows for transparent proxies to fail silently

@pcrow
Copy link

pcrow commented Oct 6, 2023

Follow-up:
I was very frustrated that stunnel worked in transparent mode as a target of sslh, but not the other way around. So I ran strace on stunnel to see what it was doing. It follows the exact same sequence, even getting the bind failure with EADDRINUSE. But instead of stopping there, it takes the source address, changes the port to zero, and retries the bind. We don't care about preserving the source port anyway, so it doesn't matter that it changes. I made the same change in sslh, and now it works perfectly for me!

@pcrow
Copy link

pcrow commented Oct 6, 2023

I'm not clear on whether there's any reason to preserve the original source port when doing a transparent proxy other than "because we can." The simple solution would be to clear it before doing the bind, but I suspect trying to preserve it and doing a retry is the right thing to do, mainly because stunnel does it, and I tend to assume they know what they're doing.

@tzvetkoff
Copy link
Contributor Author

@pcrow, if you check the latest source, there's a check now to test if the address is local, and if it is, no bind() is attempted.

As for your particular case -- running (chaining) more than 1 transparent proxy, I think there's no simple solution.
Perhaps the best will be to check if the address is local in stunnel, or simply silently ignore bind() errors.

Preserving the original source port is needed because of ident lookup, so simply setting it to 0 is not a good option IMHO.

Anyway, I see things have progressed with 2.0 so if there's no further comment, I'll simply close this issue in a few days.

@pcrow
Copy link

pcrow commented Oct 6, 2023

Avoiding the bind if it's local only works if it's really 127.0.0.1 originating the connection. If it's coming from another transparent-mode local connection, it will have an external IP and port. Not only should it not be detected as local, the bind is required to preserve the external IP. If at any step in a chain of transparent-mode connections you don't do the bind, then you lose the original IP.

As to preserving the original source port for ident, I didn't think of that; that's good. Unfortunately, it's impossible to do multi-stage transparent-mode while preserving the port. Each outgoing connection, whether to another proxy stage or to the final service, must bind to an IP and port to pass the information to the next stage. That (IP,port) must be unique on the system, as that's what the kernel uses to look up which socket receives the return packets. So for multi-stage transparent mode, you simply can't preserve the port. That's why stunnel uses zero (to force allocation of a new high port number). For most users, all that they care about is the IP being correct in the logs, so that's usually good enough.

So IMHO, the right solution is to retry bind failures assigning a new port. This should be clearly documented as the expected behavior on multi-stage transparent mode. If you're paranoid, add a --donotpreserveport option to enable the retry, which would be required for multi-stage transparent mode.

BTW: I'm running sslh-2.0.1 on Gentoo with my own retry patch. It works great, preserving the IP of the original source in all application log files.

@yrutschle
Copy link
Owner

If I get this right:

  • in bind_peer(), when chaining transparent proxies, bind() fails because it tries to keep the origin port
  • instead of failing, you can retry with port == 0 so the proxying goes forward, albeit with a different port
  • the only downside is ident no longer works

Arguably the last point is not much of a problem: I expect someone investigating with that protocol would be able to go back up the chain of proxies anyway.
On my side, I can't remember any reason to keep the port, it's just there in the address so leaving it in was just the lazy thing to do.
Would you file a PR? Otherwise, I think I can do that on my side, but seeing as I don't (currently) use transparent mode, that'd help :-)

@pcrow
Copy link

pcrow commented Oct 6, 2023

Yes, that's exactly it. I'll do a PR.

@pcrow
Copy link

pcrow commented Oct 6, 2023

#408

I cleaned it up slightly (consistent indents and C-style comments), re-tested, and created the pull request.

@tzvetkoff
Copy link
Contributor Author

Closing in favor of #408.
Also check #412.

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

No branches or pull requests

3 participants