Unfortunately the problem with this code has not been explained clear enough. It says "The only issue I see is it replace address in all bind calls" but in file 'https://github.com/Whonix/bindp/blob/master/bindp.c' there is no check for address family in your bind function, instead the check is done in connect function at line 145. Unless the packet which is sent to this connect function is corrupted I don't see a reason that the condition (rsk_in->sin_family == AF_INET) return a wrong result. A little more explanation about the problem would help solve it better.

Btw not so much "our" package. We forked it from https://github.com/yongboy/bindp. Please fix any issues you are seeing with it. Untrusted, corrupted, malicious packages as input should be expected.

What this code is doing is pretty clear but I don't know for what reason someone should use it. It simply binds the socket to a specific local address instead of the address the programmer of the external command probably decided to use. This results the incomming communication receive to that address instead of the default one . Note that this code only changes the LOCAL address whether the external command be client or server. It only does an additional bind() call. I should know how you use it that caused the problem so that I can reproduce the problem and work on it.
I used protocol families other than AF_INET and didn't observe the problem you reported.

The purpose of bindp in Whonix is to force applications listening on localhost to rather listen on eth0 so these are reachable from the gateway. Essentially, it lets us use onionshare, ricochet, unMessage and ZeroNet in Whonix 14. More details in T561.

The code where it is being used is this one:
https://github.com/Whonix/uwt/blob/master/usr/lib/uwtwrapper#L238-L247

Using it manually without all the setup, this should emulate it:

## requires Debian stretch because ifconfig output changed
BIND_ADDR="$(/sbin/ifconfig "eth0" | grep -oP 'inet \K\S+')"
LD_PRELOAD+=" /path/to//usr/lib/uwt/libindp.so/
export BIND_ADDR
export LD_PRELOAD
run some server application that would listen to 127.0.0.1 as its default)but would now listen on eth0 instead

Ok but I still don't know when exactly the problem occurs. The only place where the protocol family is checked is at line 145. This is all the scenario of the program:

1. An external program calls bind or connect
2. If protocol family of the remote address is AF_INET then bind the socket to another address which we already specified.

From the person who reported the issue with this code, all I understand is that for families other than AF_INET still the bind takes place, meaning the condition at line 145 becomes true even while the rsk_in->sin_family is something different than AF_INET. Is this what you say?

Could you clarify this please? @marmarek

The problem is not in connect function, but in bind. It assume AF_INET family, casting sk pointer to struct sockaddr_in. But if socket family is something different, the structure also may be different (for example struct sockaddr_un for AF_UNIX). In practice, besides misusing this library, the problem only applies to applications listening on both locak (AF_UNIX) and network (AF_INET) sockets. Because you don't use this library for AF_UNIX-only applications.

Created an issue for that: https://github.com/yongboy/bindp/issues/5

@marmarek If the problem is only with applications listening on both AF_INET and AF_UNIX, I think the solution should be easy, we can change the bind function in a way that it only changes local address of the sockaddr_storage while the protocol family is AF_INET. For AF_UNIX address family there doesn't seem to be any need to change the address (the pathname of the socket). By applying these changes will the problem get solved? If yes, I will rewrite the bind function for you to test.

@marmarek If the problem is only with applications listening on both AF_INET and AF_UNIX,

Yes, I think this is the only problematic case in practice. In theory
there is also a case when application use only AF_UNIX, but it doesn't
make sense to use bindp there.

BTW what about children processes? Those also get LD_PRELOAD variable.
It may happen that application listening on AF_INET will eventually
start (directly or indirectly) application listening on AF_UNIX. Or
another application listening on AF_INET, which we don't want to change
bind address...
Probably do not apply to Whonix case, but in general it may happen.

I think the solution should be easy, we can change the bind function in a way that it only changes local address of the sockaddr_storage while the protocol family is AF_INET. For AF_UNIX address family there doesn't seem to be any need to change the address (the pathname of the socket). By applying these changes will the problem get solved? If yes, I will rewrite the bind function for you to test.

Yes, this should work.

@marmarek I changed bind function and uploaded the code at: http://pastebin.com/dZb4yedz

You only need to replace this function with the one in the main code. Please test and report the results to me.

I created a branch address-family-check to ease review.

https://github.com/Whonix/bindp/tree/address-family-check

Added change by @s.sh:

https://github.com/Whonix/bindp/commit/78d4bedad1095f9446442dc5f3d764a50a46704e

Fixed intent style (hopefully - I am not a C coder):

https://github.com/Whonix/bindp/commit/b683034da9765529f09247917136002b81c93ba3

Comparison:

https://github.com/Whonix/bindp/compare/master...Whonix:address-family-check?expand=1

Code question...

    switch (_pf){
        case AF_INET:
            {
...
            }
            break;

Excuse me asking, but must that break; really be outside of the { }?

Code you please do a static code level review (just looking at the code) (how to call this?)? @marmarek After that I'd do functionality testing.

@Patrick The reason that I inserted curly braces is that the first line after "case AF_INET:" is not syntactically a statement, to work around this you may use a dummy sentence or use braces like what I did. The break keyword can be inside that block; there is no difference.

one more stylistic fix:

https://github.com/Whonix/bindp/commit/82ab25298e1194570fe9ee1b76941c393bafcf77

In T599#12596, @s.sh wrote:

@Patrick The reason that I inserted curly braces is that the first line after "case AF_INET:" is not syntactically a statement, to work around this you may use a dummy sentence or use braces like what I did. The break keyword can be inside that block; there is no difference.

Great, moved the break; inside the curly braces.

https://github.com/Whonix/bindp/commit/97c07294e558359dae1506be12be02ce4b1c3a45

intent:
https://github.com/Whonix/bindp/commit/37a63005354aa007907e04f87fe9ed1ed399e63d

Do you know how to fix these compiler warning?

gcc -nostartfiles -fpic -shared bindp.c -o libindp.so -ldl -D_GNU_SOURCE   
bindp.c: In function ‘_init’:
bindp.c:83:27: warning: implicit declaration of function ‘inet_addr’ [-Wimplicit-function-declaration]
         bind_addr_saddr = inet_addr (bind_addr_env);
                           ^~~~~~~~~
bindp.c: In function ‘bind’:
bindp.c:129:21: warning: implicit declaration of function ‘inet_ntop’ [-Wimplicit-function-declaration]
                     inet_ntop(AF_INET,&(lsk_in->sin_addr),original_ip,INET_ADDRSTRLEN);
                     ^~~~~~~~~

@Patrick Add

#include <arpa/inet.h>

at the beginning of the file.

In T599#12599, @s.sh wrote:

@Patrick Add

#include <arpa/inet.h>

at the beginning of the file.

Great! That worked. Added:

https://github.com/Whonix/bindp/commit/09ea8adad40db9c62d5fe3847e8796796829ed90

Do you know how to fix this ld warning?

gcc -nostartfiles -fpic -shared bindp.c -o libindp.so -ldl -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -g -O2 -fPIE -fstack-protector-strong -Wformat -Werror=format-security -fPIE -pie -Wl,-z,relro -Wl,-z,now

/usr/bin/ld: warning: cannot find entry symbol _start; defaulting to 00000000000006e0

@Patrick You can compile without -fPIE (I think -fPIC is enough):

gcc -nostartfiles -fpic -shared bindp.c -o libindp.so -ldl -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security  -Wl,-z,relro -Wl,-z,now

But these warnings are not related to my revisions, they probably existed in original code.

In T599#12601, @s.sh wrote:

But these warnings are not related to my revisions, they probably existed in original code.

Yes. That is for sure. These are just part of the original issue description in this ticket at the very top. Was wondering if you'd like to fix those while you are at bindp.

@Patrick You can compile without -fPIE (I think -fPIC is enough):

gcc -nostartfiles -fpic -shared bindp.c -o libindp.so -ldl -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security  -Wl,-z,relro -Wl,-z,now

That indeed removes the ld warning but apparently reduced hardening. We'd like to have all compiler hardening flags enabled for better security. (As long as sensible in this situation.)

gcc -nostartfiles -fpic -shared bindp.c -o libindp.so -ldl -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z,relro -Wl,-z,now results in:

RELRO           STACK CANARY      NX            PIE             RPATH      RUNPATH      FILE
Full RELRO      Canary found      NX enabled    DSO             No RPATH   No RUNPATH   libindp.so

PIE, DSO is shown as yellow by checksec.sh. What does DSO mean? Before it was green PIE enabled.

Can we have all hardening with PIE enabled as well as without ld warning?

Can we have all hardening with PIE enabled as well as without ld warning?

I don't think so, but I think PIE is more necessary for regular executables not dynamic libraries, since libraries are relocatable by nature. (Of course PIE adds some more optimization for ASLR mechanism, however)
So for now I can't say whether it's wise to remove PIE, it needs more research.
You can keep compiling with PIE, apparently the warning is about start entry which again I don't believe is problematic for libraries.

What does DSO mean?

Must be Dynamic Shared Object

@Patrick Add

int main(int argc,char **argv){return 0;}

to the end of bindp.c, now compile with :

gcc -nostartfiles -fpic -shared --entry main bindp.c -o libindp.so -ldl -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -g -O2 -fPIE -fstack-protector-strong -Wformat -Werror=format-security -fPIE -pie -Wl,-z,relro -Wl,-z,now

The warning should disappear

Did that.

https://github.com/Whonix/bindp/commit/5019b6be908a0f7ab66af7cfd109a26bd0926423

checksec --file /usr/lib/uwt/libindp.so

RELRO           STACK CANARY      NX            PIE             RPATH      RUNPATH      FILE
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   No RUNPATH   /usr/lib/uwt/libindp.so

Perfect! Now all compilation warnings are fixed and missing hardening features are implemented.

@Patrick Nice, now regular testing needs to be done from your side, please keep in mind that connect function has to change as well, but before that I must assure that current bind() works properly and as expected. If everything goes well I will write whole code from scratch for you by modifying init and connect functions.

That's great! Thank you for your help! Just now tested. Unfortunately it does not work.

user@host:~$ BIND_ADDR=10.137.11.80
user@host:~$ LD_PRELOAD=/usr/lib/bindp/libindp.so
user@host:~$ export BIND_ADDR
user@host:~$ export LD_PRELOAD
user@host:~$ /usr/bin/onionshare.anondist-orig a

Segmentation fault

no segfault:

gcc -nostartfiles -fpic -shared --entry main bindp.c -o libindp.so -ldl -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z,relro -Wl,-z,now

segfault:

gcc -nostartfiles -fpic -shared --entry main bindp.c -o libindp.so -ldl -D_GNU_SOURCE -fPIE -pie -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fdebug-prefix-map=/home/user/bindp=. -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z,relro -Wl,-z,now
user@host:~/bindp$ LD_PRELOAD=/home/user/bindp/libindp.so onionshare.anondist-orig /home/user/a
Segmentation fault

I think we can safely remove -fPIE? That solves the segfault and still shows all hardening flags.

Did that.

https://github.com/Whonix/bindp/commit/725042f5d4865be27a9f2ac45a250219cee6d5cc

remove `-fPIE` to fix segmentation fault (we still use `-pie`)

We still have all hardening flags.

checksec --file /usr/lib/bindp/libindp.so
RELRO           STACK CANARY      NX            PIE             RPATH      RUNPATH      FILE
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   No RUNPATH   /usr/lib/bindp/libindp.so

Would it with -fPIE be more secure? If so, can this libindp.so be fixed to work with -fPIE?

It's functional! Successfully changes the listener IP.

/usr/bin/onionshare.anondist-orig a

Onionshare 0.9.1 | https://onionshare.org/
[-] LIB received AF_INET bind request
[!] AF_INET: Leaving request unchanged
[-] LIB received AF_INET bind request
[!] AF_INET: Leaving request unchanged
Connecting to Tor control port to set up onion service on port 17600.
Staring ephemeral Tor onion service and awaiting publication
Preparing files to share.
[-] LIB received AF_INET bind request
[!] AF_INET: Leaving request unchanged
[-] LIB received AF_INET bind request
[!] AF_INET: Leaving request unchanged
 * Running on http://127.0.0.1:17600/ (Press CTRL+C to quit)
Give this URL to the person you're sending the file to:
http://3i4em3y4rkxbntqj.onion/daley-err

Press Ctrl-C to stop server

Well the segfault error is strange here, I must run it locally with your setup to check and debug which is not possible for now, but it's good you made it work finally. Good job Patrick. Still I think PIE related options are not needed for libraries.
I will modify the code by tonight and send the new revisions.

Okay, great!

Why did you add #ifdef SO_REUSEPORT?

if (l_bind_addr && l_bind_port) {

As a minor point, I think the debug messages are slightly broken. It shows [!] AF_INET: Leaving request unchanged since we are only setting envrionment variable BIND_ADDR and leaving BIND_PORT unset.

In T599#12621, @Patrick wrote:

Why did you add #ifdef SO_REUSEPORT?

It was in the original code; not added by me. SO_REUSEPORT is defined in asm-generic/socket.h but apparently the original author wanted to make sure it is defined.

As a minor point, I think the debug messages are slightly broken. It shows [!] AF_INET: Leaving request unchanged since we are only setting envrionment variable BIND_ADDR and leaving BIND_PORT unset.

Yes I know, I will fix it.

@Patrick
Code fixed. Please check and use this: http://pastebin.com/GvDpuC0f

Added that. And added some intent style changes. Please tell me if my C intent style is actually making things non-standard / worse than before.

https://github.com/Whonix/bindp/blob/address-family-check/bindp.c

That version is currently broken. (In comparison git master version is still functional which makes me reasonable sure nothing else in the setup is broken.)

Onionshare 0.9.1 | https://onionshare.org/
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
Can't connect to Tor control port on port [9151, 9153, 9051]. OnionShare requires Tor Browser to be running in the background to work. If you don't have it you can get it from https://www.torproject.org/.

Might it perhaps bend a few too many connections, i.e does it bend now connections for 127.0.0.1 that it should not change?

@Patrick I need more debug info, I suspect the connect function is causing trouble.
Please use this and send the output again: http://pastebin.com/BZqTRBTc

Onionshare 0.9.1 | https://onionshare.org/
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[!] connect(): AF_INET connect() call
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[!] connect(): AF_INET connect() call
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[!] connect(): AF_INET connect() call
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
Can't connect to Tor control port on port [9151, 9153, 9051]. OnionShare requires Tor Browser to be running in the background to work. If you don't have it you can get it from https://www.torproject.org/.

Could you please base any other changes on top of the current https://github.com/Whonix/bindp/blob/address-family-check/bindp.c so it keeps intent and typo fixes?

@Patrick Updated: http://pastebin.com/9XcTZwVG

Works for me!

/usr/bin/onionshare.anondist-orig a
Onionshare 0.9.1 | https://onionshare.org/
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[-] connect(): AF_INET connect() call, binding to local address
[-] LIB received AF_INET bind request
[-] Changing 10.137.11.80 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
Connecting to Tor control port to set up onion service on port 17600.
Staring ephemeral Tor onion service and awaiting publication
Preparing files to share.
[-] LIB received AF_INET bind request
[-] Changing 127.0.0.1 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
[-] connect(): AF_INET connect() call, binding to local address
[-] LIB received AF_INET bind request
[-] Changing 10.137.11.80 to 10.137.11.80
[-] AF_INET: Leaving port unchanged
 * Running on http://127.0.0.1:17600/ (Press CTRL+C to quit)
Give this URL to the person you're sending the file to:
[redacted]

sudo netstat -tulpen | grep python
tcp        0      0 10.137.11.80:17600      0.0.0.0:*               LISTEN      1000       375953     20647/python3

(The usual operating mode of onionshare [that has nothing to do with bindp] is that it first creates the ephemeral Tor hidden service using Tor control protocol commands add_onion etc. and once is notices that it's ready starts the webserver on 127.0.0.1. [Which uwt / bindp bends to eth0's IP in this example 10.137.11.80.])

@Patrick Do you have anything else from this project remained that needs extra working on ?

Functionally speaking, as I tested, it works great in Whonix. Pretty much done.

Do you see any things a malicious application could to gain arbitrary code execution through bindp?

In the final version we need to disable debug mode by default, but that is trivial. (Already tried that and it's working.)

For now, the only missing thing is a code review for https://github.com/Whonix/bindp/blob/address-family-check/bindp.c. Only @marmarek comes to mind who could do that.

Lower priority, but I'd still like to see these changes upstreamed (git pull request to the original author at https://github.com/yongboy/bindp). That would give us another code review, prevent us maintaining a fork at Whonix, and we'd benefit from further development/review by others.

Notified upstream. Let's see if he replies.

bindp pull request - security fixes, debian packaging, and more:
https://github.com/yongboy/bindp/pull/6

Do you see any things a malicious application could to gain arbitrary code execution through bindp?

Very unlikely. I used standard socket functions. If a malicious program could use bindp to gain code execution it most probably can do the same with regular socket applications too. But I will check it more observantly when I got the time to.

Lower priority, but I'd still like to see these changes upstreamed (git pull request to the original author at https://github.com/yongboy/bindp). That would give us another code review, prevent us maintaining a fork at Whonix, and we'd benefit from further development/review by others.
Notified upstream. Let's see if he replies.

From experience, I can say if the person does not answer within 2 days he won't answer at all.

@Patrick In the master branch the only difference in comparison to the original version that I can see is the main function at the bottom of the file. Did you not apply the changes? This code is still the previous one.

Now pushed.

In T599#13705, @Patrick wrote:

Now pushed.

I see debug_enabled is '1'; maybe it's better we disable debug messages for final release.

Thanks! Done.

What about...?

/*
    FIXME: Be careful when using setsockopt
    Is it valid to use these options for AF_UNIX?
    Must be checked
*/

Is good enough?

What about...?

I believe the answer to the question that I mentioned in the comment, needs more extensive research, maybe at a later time, with having that fact in mind, I think the code would be prettier if this comment block is removed. Now as you wish.

In T599#12631, @Patrick wrote:

Notified upstream. Let's see if he replies.

bindp pull request - security fixes, debian packaging, and more:
https://github.com/yongboy/bindp/pull/6

In T599#12640, @s.sh wrote:

From experience, I can say if the person does not answer within 2 days he won't answer at all.

A miracle has happened. All of https://github.com/yongboy/bindp/pull/6 was merged by upstream.

A miracle has happened. All of https://github.com/yongboy/bindp/pull/6 was merged by upstream.

Very well. So finally they applied our patch.