matrix-org / matrix-spec-proposals

Proposals for changes to the matrix specification
Apache License 2.0
999 stars 379 forks source link

The security offered by access_token is way too weak, especially for non-TLS client traffic. (SPEC-112) #424

Closed matrixbot closed 6 years ago

matrixbot commented 9 years ago

1) access_tokens never change (unless you log out) 2) they are trivial to sniff from non-TLS HTTP traffic and/or clientside logs.

Surely we should be using an actual digest handshake or HMAC or similar to make it a /little/ harder for clients' access_tokens to not be silently sequestered?

(Imported from https://matrix.org/jira/browse/SPEC-112)

(Reported by @ara4n)

matrixbot commented 9 years ago

Jira watchers: @Kegsay @ara4n @richvdh

matrixbot commented 9 years ago

Links exported from Jira:

is duplicated by SPEC-231 relates to SYN-533 relates to SYN-299

matrixbot commented 9 years ago

https://github.com/rescrv/libmacaroons could be a fun possible solution to this.

-- @ara4n

matrixbot commented 9 years ago

We will have trouble getting any kind of simple curl-like example if we end up forcing HMAC on everything, and it will increase the burden of even noddy client examples. Time-limited access_token s are an improvement on the baseline, without (much) added complexity, but give much worse sniffing protection than HMAC.

We should support alternative authentication mechanisms, but I don't feel that they should be applied by default, since by default people should be using HTTPS. If we do end up doing time limited tokens, we might want to look into the OAuth2 guidelines, and consider if we should be supporting OAuth2 login completely (where clients redirect to your home server to login, then get a token back). The added benefit to that of course would be the ability to define app-specific redirect URIs (e.g. myapp://) which is nice on mobile clients since their apps can intercept them.

-- @Kegsay

matrixbot commented 9 years ago

We now use macaroons and provide a /tokenrefresh endpoint. I feel this can be closed now but would like to check.

-- @Kegsay

matrixbot commented 8 years ago

This bug is a bit handwavey; it's not clear what would represent a satisfactory implementation to allow closure.

Tokens don't actually expire yet (SYN-533), so the presence of a /tokenrefresh endpoint isn't particularly relevant; and macaroons are as susceptible to sniffing out of http logs as the old tokens were (SYN-299).

OTOH without concrete suggestions about what else would be better, my inclination is to close this bug.

-- @richvdh

matrixbot commented 8 years ago

Possibly a bit off topic, but what is the reason for allowing non-TLS connections in the client-server API? This might not be an issue of TLS was mandated by the spec.

-- Jimmy Cuadra

matrixbot commented 8 years ago

Both to make it trivial to debug (e.g. to allow tcpdumping http problems without TLS getting in the way), and to support utterly stupid clients like IOT devices which aren't smart enough to talk TLS.

-- @ara4n

matrixbot commented 8 years ago

In either case, I'd like to see a token invalidating endpoint, for cases like when the token gets compromised.

You also mention logging out, but I cannot see anything about that in the spec.

-- Gergely Polonkai

matrixbot commented 8 years ago

So, this bug had a few references to Macaroons, but I feel that none of the posts that did so explained how Macaroons help - so I'll try and do so.

The really nice thing with Macaroons is that anyone can further constrain them, but nobody can remove a constraint once it's added. This allows the client to constrain the macaroon sent back to the server to a very short lifetime (on the order of seconds), while the one it actually holds may have a very long validity period indeed. If anyone sniffs the in-flight Macaroon, it will (by and large) be useless too soon to do them any good.

In addition, it can be constrained to the operation in question (if Synapse supported such caveats), so the sniffed macaroon would (say) only be usable for sending messages (and not state events), or perhaps even only to a specific room.

It can also be constrained to the user's external IP, which helps even more.

This would (partially) resolve SYN-299, too, so I'm copying it there, although TBH the right solution to that is probably "Authorization: Macaroon "

-- Alex Elsayed

matrixbot commented 7 years ago

An interesting and well-written article discussing the problems with simple bearer tokens in oauth, much of which rings true for us: https://web.archive.org/web/20110518133340/http://hueniverse.com/2010/09/oauth-bearer-tokens-are-a-terrible-idea/

-- @richvdh

ara4n commented 7 years ago

...meanwhile, we've removed the tokenrefresh endpoint for now, as nothing was using it. so if anything this bug has gone backwards :(

ara4n commented 7 years ago

see https://github.com/matrix-org/matrix-doc/pull/395

ara4n commented 7 years ago

On the plus side, we have got https://github.com/matrix-org/synapse/pull/1098, which makes the risk of leaking less. If anything used it.

richvdh commented 6 years ago

I'm closing this in favour of https://github.com/matrix-org/matrix-doc/issues/1042 and https://github.com/matrix-org/matrix-doc/issues/1043.