Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
matrixbot opened this issue Feb 19, 2015 · 15 comments
Labels
improvement A suggestion for a relatively simple improvement to the protocol p1

Comments

@matrixbot
Copy link
Member

  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
Copy link
Member Author

Jira watchers: @kegsay @ara4n @richvdh

@matrixbot
Copy link
Member Author

matrixbot commented Feb 19, 2015

Links exported from Jira:

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

@matrixbot
Copy link
Member Author

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

-- @ara4n

@matrixbot
Copy link
Member Author

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
Copy link
Member Author

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

-- @kegsay

@matrixbot
Copy link
Member Author

matrixbot commented Dec 1, 2015

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

matrixbot commented Mar 1, 2016

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
Copy link
Member Author

matrixbot commented Oct 26, 2016

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

@matrixbot matrixbot changed the title The security offered by access_token is way too weak, especially for non-TLS client traffic. The security offered by access_token is way too weak, especially for non-TLS client traffic. (SPEC-112) Oct 31, 2016
@matrixbot matrixbot added the spec-bug Something which is in the spec, but is wrong label Nov 7, 2016
@ara4n
Copy link
Member

ara4n commented Dec 20, 2016

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

@ara4n
Copy link
Member

ara4n commented Dec 20, 2016

see #395

@ara4n
Copy link
Member

ara4n commented Dec 20, 2016

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

@richvdh richvdh added improvement A suggestion for a relatively simple improvement to the protocol and removed auth spec-bug Something which is in the spec, but is wrong labels Oct 13, 2017
@richvdh
Copy link
Member

richvdh commented Oct 26, 2017

@richvdh richvdh closed this as completed Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement A suggestion for a relatively simple improvement to the protocol p1
Projects
None yet
Development

No branches or pull requests

3 participants