Disable forwarding redirect for peer's HTTP communication

[ahrtr]

What would you like to be added?

It's possible that etcd server may run into SSRF (Server-side request forgery) when adding a new member. If users provide a malicious peer URL, the existing etcd members may be redirected to other unexpected internal URL when getting the new member's version,

etcd/server/etcdserver/cluster_util.go

Line 250 in a9cf27b

addr := u + "/version"

We should disable the forwarding redirect for the endpoint /version, as there is no need to support it.

client := &http.Client{
    Transport: rt,
    Timeout:   timeout,
    CheckRedirect: func(req *http.Request, via []*http.Request) error {
        return http.ErrUseLastResponse
    },
}

We should also evaluate all peer's HTTP communication, and disable the redirect as well if needed.

etcd/server/etcdserver/api/etcdhttp/peer.go

Lines 61 to 75 in d0114cf

	mux.Handle(rafthttp.RaftPrefix, raftHandler)
	mux.Handle(rafthttp.RaftPrefix+"/", raftHandler)
	mux.Handle(peerMembersPath, peerMembersHandler)
	mux.Handle(peerMemberPromotePrefix, peerMemberPromoteHandler)
	if leaseHandler != nil {
	mux.Handle(leasehttp.LeasePrefix, leaseHandler)
	mux.Handle(leasehttp.LeaseInternalPrefix, leaseHandler)
	}
	if downgradeEnabledHandler != nil {
	mux.Handle(etcdserver.DowngradeEnabledPath, downgradeEnabledHandler)
	}
	if hashKVHandler != nil {
	mux.Handle(etcdserver.PeerHashKVPath, hashKVHandler)
	}
	mux.HandleFunc(versionPath, versionHandler(s, serveVersion))

Why is this needed?

Improve security

[jmhbnz]

Discussed during sig etcd triage meeting on 23/11/2023. Assigned to @ivanvc to investigate remaining etcd peer http interactions.

[ivanvc]

Happy to help. Can you assign it to me? Thanks

[lavacat]

Another area to check out are endpoints from Transport.Hander
https://github.com/etcd-io/etcd/blob/904c0769e9aaf4b676bda8ff2a866e558a40270b/server/etcdserver/api/rafthttp/transport.go#L157C1-L166C12

[ivanvc]

If I understand correctly, we need to update the HTTP client from the following locations:

• Promote Member: https://github.com/etcd-io/etcd/blob/3b37afe/server/etcdserver/cluster_util.go#L292
• Lease
  • Renew: https://github.com/etcd-io/etcd/blob/3b37afe/server/lease/leasehttp/http.go#L153
  • TTL: https://github.com/etcd-io/etcd/blob/3b37afe/server/lease/leasehttp/http.go#L213
• Downgrade Enabled: https://github.com/etcd-io/etcd/blob/3b37afe/server/etcdserver/cluster_util.go#L362-365
• Hash KV: https://github.com/etcd-io/etcd/blob/3b37afe/server/etcdserver/corrupt.go#L470

As for the raft routes, I think they're safe as http.RoundTripper doesn't follow redirects, and the checkPostResponse from raft's utils (https://github.com/etcd-io/etcd/blob/3b37afe/server/etcdserver/api/rafthttp/util.go#L85-L117), doesn't handle redirects, so it shouldn't follow a redirection.

There's also a request in cluster_util.go, to get the cluster from remote peers (https://github.com/etcd-io/etcd/blob/3b37afe/server/etcdserver/cluster_util.go#L72-75). Should we disallow redirections there too?

[jmhbnz]

Thanks @ivanvc - I can't see how these additional http clients could be abused but I also don't see the harm in just disabling following redirects for them as to the best of my knowledge they should not be needed by etcd.

My vote would go to raising a pull request to update all of these instances to not follow redirects. Perhaps leave it a few days to see if maintainers or other contributors have any different views on it.

[ivanvc]

@ahrtr, @lavacat, any thoughts on this? Else, I'll go ahead and implement as @jmhbnz suggested :)

[ivanvc]

Hey @jmhbnz, I saw you added the backport v3.5 label. Should I cherry-pick my commit and create a new PR targeting the release-3.5 branch?

[jmhbnz]

Hey @ivanvc - Yes given this was a security concern my view is we should backport this to release-3.5.

Note - the codebase structure can be quite different going back to release-3.5 and particularly release-3.4 so we generally avoid any automated cherry pick and just add a manual commit as normal to complete backports.

I will re-open until we get the backport merged, thanks for your help on this @ivanvc 🙏🏻

[ivanvc]

@jmhbnz I think we can close the ticket now unless we want to backport it to another version.

[jmhbnz]

Hey @ahrtr do you think we should be trying to backport an update for this to 3.4? It is security related so probably, but defer to you as release manager for the branch.

[ahrtr]

Hey @ahrtr do you think we should be trying to backport an update for this to 3.4? It is security related so probably, but defer to you as release manager for the branch.

Yes, please backport the fix to 3.4 as well. Thanks.

[ivanvc]

Yes, please backport the fix to 3.4 as well. Thanks.

Oops I didn't see this before. I will do it.

[ivanvc]

@jmhbnz, I included the change you did to getVersion in my commit: 838cd9a.