LGTM error: Binding a socket to all network interfaces

[DimitriPapadopoulos]

Describe the bug

In pynetdicom/transport.py, socket.bind() may be called with a "null" argument ('', 0):

    def _create_socket(
        self, address: Tuple[str, int] = ('', 0)
    ) -> socket.socket:
        [...]
        sock.bind(address)

Just wondering, could the default value ('', 0) be changed to something else, perhaps something such as ('127.0.0.1', 0)?

The socket.bind() documentation doesn't say much about this.:

• A pair (host, port) is used for the AF_INET address family, where host is a string representing either a hostname in Internet domain notation like 'daring.cwi.nl' or an IPv4 address like '100.50.200.5', and port is an integer.
  • For IPv4 addresses, two special forms are accepted instead of a host address: '' represents INADDR_ANY, which is used to bind to all interfaces, and the string '<broadcast>' represents INADDR_BROADCAST. This behavior is not compatible with IPv6, therefore, you may want to avoid these if you intend to support IPv6 with your Python programs.

Issue found by LGTM:
https://lgtm.com/projects/g/pydicom/pynetdicom/snapshot/9800a4952dcb886395cdd829a236fe56949307aa/files/pynetdicom/transport.py?sort=name&dir=ASC&mode=heatmap#x961e515b44b6cc5d:1

[DimitriPapadopoulos]

See:
https://lgtm.com/rules/1510594847238/

Sockets can be used to communicate with other machines on a network. You can use the (IP address, port) pair to define the access restrictions for the socket you create. When using the built-in Python socket module (for instance, when building a message sender service or an FTP server data transmitter), one has to bind the port to some interface. When you bind the port to all interfaces using 0.0.0.0 as the IP address, you essentially allow it to accept connections from any IPv4 address provided that it can get to the socket via routing. Binding to all interfaces is therefore associated with security risks.

[scaramallion]

Done, thanks!

[scaramallion]

Changing it doesn't seem to have removed the error notice

[DimitriPapadopoulos]

Indeed, most other alerts are gone, this error is still there:
https://lgtm.com/projects/g/pydicom/pynetdicom/snapshot/8f44476086bf2a164d16fb31fc6c6443e5cc8150/files/pynetdicom/transport.py?sort=name&dir=ASC&mode=heatmap#x961e515b44b6cc5d:1

And yet you have changed to:

    def _create_socket(self, address: Tuple[str, int] = BIND_ADDRESS) -> socket.socket:
        [...]
        sock.bind(address)

where:

# The default address that client sockets are bound to
BIND_ADDRESS = ("127.0.0.1", 0)

[DimitriPapadopoulos]

The exact error message refers to an empty string, which represents INADDR_ANY:

'' binds a socket to all interfaces.

My guess is that the error message suggests the function should bail out if the address argument is an empty string:

    def _create_socket(self, address: Tuple[str, int] = BIND_ADDRESS) -> socket.socket:
        [...]
        host, port = address
        if host == "":
            raise ValueError("cannot bind the socket to empty host which represents INADDR_ANY")
        [...]
        sock.bind(host, port)

[scaramallion]

Hmm... that's actually a bit cleverer than I was expecting their check to be. Thanks for digging into it, I'll make the change in the next PR

It would imply I'd have to check the various accepted tested forms of INADDR_ANY...

// IPv4
"0.0.0.0", "",
// IPv6
"::", "::0"

[DimitriPapadopoulos]

That said, aren't there cases where one may actually want INADDR_ANY?

[scaramallion]

Yeah, I'll add a new config variable as List[str] and mention it in the exception.

[DimitriPapadopoulos]

That said, I'm not certain INADDR_ANY should be disallowed by default. Replacing "" with "localhost" in documentation and examples seems sufficient to me.

I see this LGTM.com alert as a mere warning. Once you are warned and aware of possible issues, you may silence the warning - with # lgtm[py/bind-socket-all-network-interfaces].

[scaramallion]

That said, I'm not certain INADDR_ANY should be disallowed by default.

I've been going both ways on it, it'll definitely mean more issues. It'd be nice if I could tell the difference between dev and production...