httpclient::connect - silently falls back to insecure `http` for `https` url's that do not explicitly specify a port

[tintinweb]

Description

• httpClient silently falls back to insecure http transport for https uri's that do not specify the port.
• port is parsed with parseInt allowing a negative port to be passed. Check bounds or use parseSaturatedNatural instead. This may fail in nim-chronos though (which still allows port 0)
• implement ssl/tls support (I assume that's part of the reason for this issue) (nim-chronos/transports/common does not implement tls either) or reject https URLs and make users aware that their RPC is established over an insecure transport.

Note: Under normal circumstances, the RPC endpoint is probably system local anyway, however, one might choose to connect to an RPC endpoint on a different host/net inter or intra datacenter.

How to reproduce

• web3 rpc URL: https://mainnet.infura.io

expected: transport security enabled for https URL: https://mainnet.infura.io
actual result: no transport security as client silently falls back to http://mainnet.infura.io:80

see: logline file=httpclient.nim:47 address=34.227.246.217:80

⇒  ./beacon_node --web3-url=https://mainnet.infura.io --deposit-contract=0x92c506d3dd51a37650cc8e352a7551c26e2c607d --deposit-contract-block=0x8491ec6dfea11adc920bd33ff66a0523de1da24f913bca6f6ef71d5b298c6c9c --data-dir=/tmp 
DBG 2020-07-15 15:03:24+02:00 Launching beacon node                      topics="beacnde" tid=7171536 file=beacon_node.nim:1295 cmdParams="@[\"--web3-url=https://mainnet.infura.io\", \"--deposit-contract=0x92c506d3dd51a37650cc8e352a7551c26e2c607d\", \"--deposit-contract-block=0x8491ec6dfea11adc920bd33ff66a0523de1da24f913bca6f6ef71d5b298c6c9c\", \"--data-dir=/tmp\"]" config="(logLevel: \"DEBUG\", eth2Network: None[TaintedString], dataDir: /tmp, web3Url: \"https://mainnet.infura.io\", depositContractAddress: Some(0x92c506d3dd51a37650cc8e352a7551c26e2c607d), depositContractDeployedAt: Some(0x8491ec6dfea11adc920bd33ff66a0523de1da24f913bca6f6ef71d5b298c6c9c), nonInteractive: false, cmd: noCommand, bootstrapNodes: @[], bootstrapNodesFile: , libp2pAddress: 0.0.0.0, tcpPort: 9000, udpPort: 9000, maxPeers: 79, nat: \"any\", validators: ..., validatorsDirFlag: None[InputDir], secretsDirFlag: None[InputDir], walletsDirFlag: None[InputDir], stateSnapshot: None[InputFile], stateSnapshotContents: ..., runtimePreset: (GENESIS_FORK_VERSION: 00000000, GENESIS_DELAY: 172800, MIN_GENESIS_ACTIVE_VALIDATOR_COUNT: 16384, MIN_GENESIS_TIME: 1578009600), nodeName: \"\", graffiti: None[GraffitiBytes], verifyFinalization: false, stopAtEpoch: 0, metricsEnabled: false, metricsAddress: 127.0.0.1, metricsPort: 8008, statusBarEnabled: true, statusBarContents: \"peers: $connected_peers;finalized: $finalized_root:$finalized_epoch;head: $head_root:$head_epoch:$head_epoch_slot;time: $epoch:$epoch_slot ($slot)|ETH: $attached_validators_balance\", rpcEnabled: false, rpcPort: 9090, rpcAddress: 127.0.0.1, dumpEnabled: false)" version="0.5.0 (b0470d7)"
INF 2020-07-15 15:03:24+02:00 Starting Eth1 deposit contract monitoring  tid=7171536 file=mainchain_monitor.nim:752 contract=0x92c506d3dd51a37650cc8e352a7551c26e2c607d url=web3(https://mainnet.infura.io)
INF 2020-07-15 15:03:24+02:00 Waiting for new Eth1 block headers         tid=7171536 file=mainchain_monitor.nim:417
DBG 2020-07-15 15:03:25+02:00 Message sent to RPC server                 topics="JSONRPC-HTTP-CLIENT" tid=7171536 file=httpclient.nim:188 address=34.227.246.217:80 msg_len=74
DBG 2020-07-15 15:03:25+02:00 Server returns error                       topics="JSONRPC-HTTP-CLIENT" tid=7171536 file=httpclient.nim:47 address=34.227.246.217:80 httpcode=400 httpreason="Bad Request"
ERR 2020-07-15 15:03:25+02:00 Mainchain monitor failure, restarting      tid=7171536 file=mainchain_monitor.nim:790 err="Empty response from server"

parseUri(http://somedomain.tld:-22).port == -22 => parseInt("-22") = int(-22)

Details

nim-json-rpc/json_rpc/clients/httpclient.nim

Lines 207 to 213 in f53e0b9

	proc connect*(client: RpcHttpClient, url: string) {.async.} =
	# TODO: The url (path, etc) should make it into the request
	let pu = parseUri(url)
	var port = Port(80)
	if pu.port.len != 0:
	port = parseInt(pu.port).Port
	client.addresses = resolveTAddress(pu.hostname, port)

Recommendation

• enforce tls if scheme is https (with any port specified; don't fall back to http if port 80)
• if port not specified and scheme is https set port to default 443
• check port bounds. disallow port 0. use parseSaturatedNatural instead of parseInt
• make users aware of the fact that their connection is established with an insecure endpoint if URL.scheme is https. reject to connect to https if it is not implemented.

[tintinweb]

I am lacking permissions to set the nbc-... and security audit label. Would be great if someone could tag this issue :)

This code is not in scope for round1 but we agreed that security-relevant issues should be tracked anyway. better safe than sorry :)

thx 🙏