Require SSH prefix in router.DialHost connections

[AntonAM]

This PR makes sure that first bytes we see on connections that were created in router.DialHost() is the SSH protocol prefix, so malicious users can't send their own PROXY headers.

Fixes https://github.com/gravitational/teleport-private/issues/737

[rosstimothy]

I believe that all connections established via tsh ssh no longer use the proxy subsystem to connect to a host. Do we need this for those as well?

[espadolini]

The correct place for this is (*Router).DialHost in lib/proxy/router.go, which ends up being used by proxy subsys and TransportService/ProxySSH (and even then, maybe only in places where we end up actually dialing a connection to the outside world?).

[jentfoo]

I left a couple comments to consider, it would be good to add test cases for them as well.

[jentfoo]

By not setting this till the end, we are introducing a weird behavior pattern. If there is an error we end up discarding a chunk of the data, but we technically leave this function open to then verify later.

I would suggest setting upstreamReader to a reader which always returns an error around line 289, then let it be reset to a valid reader after verification

[espadolini]

This is kind of a bad assumption and it will lead to really strange issues precisely in situations where connectivity is already bad enough to be annoying in other ways.

I recommend changing requiredPrefix and checked into requiredData, and then you can check if the beginning of p matches the beginning of requiredData (depending on which one is the shorter), then writing, then advancing requiredData by the returned amount by Write.

Right now you can bypass this (accidentally or not):

w := &checkedPrefixWriter{requiredPrefix: p}

w.SetWriteDeadline(time.UnixNano(1))
w.Write(p) // guaranteed to fail, but sets "checked" to true
w.SetWriteDeadline(time.Time{})
w.Write(notP)

[AntonAM]

Yeah, but it still will return an error. Our goal is to not let attacker write data starting with anything but required prefix without an error, and in our use case that will be enough and connection will be rejected, unless there's a bug somewhere else 😅
But I changed it to the scheme you suggested and it can now accommodate multiple writes to satisfy the prefix requirement.

[AntonAM]

@rosstimothy yes, it's not tsh ssh itself, but the fact that we have ProxySSH() on server, so we need to make sure malicious attacker can't use it to self-call Proxy and then send fake PROXY header. For some reason I thought new transport is only available starting from v14 🤦‍♂️ , and in v14 this attack already doesn't work, so we mainly need this fix for v12 and v13 (although it's safer to have it everywhere)

I've moved the check to the write side in the DialHost as @espadolini suggested.

[AntonAM]

@jentfoo changed the implementation.

[espadolini]

It feels simpler to just advance the slice, but if you want to keep the entire original prefix (maybe to output as an error) feel free to ignore that.

The net.Conn contract prescribes that all functions must be callable concurrently, which this implementation fails to provide - that being said, neither does lib/multiplexer.Conn, so supporting concurrent calls to Write doesn't seem to be particularly important, anyway. It might be worth noting in a comment, tho.

[AntonAM]

@espadolini yeah, I started with advancing the slice, but then error message is kinda incorrect.

Yep, I know it's not concurrent-safe, but as you said, for our code in general that train has left the station (and I think that's fine, our networking is complicated as it is without concurrent read/writes to connections 😅 )
I'll add comment.

[public-teleport-github-review-bot]

@AntonAM See the table below for backport results.

Branch	Result
branch/v12	Failed
branch/v13	Failed
branch/v14	Create PR