Prevent user tokens being used as guest tokens (#1675)

Make sure that a user cannot pretend to be a guest by adding 'guest = True'
caveats.
This commit is contained in:
Richard van der Hoff 2016-12-06 15:31:37 +00:00 committed by GitHub
parent 194b6259c5
commit 1529c19675
3 changed files with 116 additions and 32 deletions

View file

@ -677,31 +677,28 @@ class Auth(object):
@defer.inlineCallbacks @defer.inlineCallbacks
def get_user_by_access_token(self, token, rights="access"): def get_user_by_access_token(self, token, rights="access"):
""" Get a registered user's ID. """ Validate access token and get user_id from it
Args: Args:
token (str): The access token to get the user by. token (str): The access token to get the user by.
rights (str): The operation being performed; the access token must
allow this.
Returns: Returns:
dict : dict that includes the user and the ID of their access token. dict : dict that includes the user and the ID of their access token.
Raises: Raises:
AuthError if no user by that token exists or the token is invalid. AuthError if no user by that token exists or the token is invalid.
""" """
try: try:
ret = yield self.get_user_from_macaroon(token, rights) macaroon = pymacaroons.Macaroon.deserialize(token)
except AuthError: except Exception: # deserialize can throw more-or-less anything
# TODO(daniel): Remove this fallback when all existing access tokens # doesn't look like a macaroon: treat it as an opaque token which
# have been re-issued as macaroons. # must be in the database.
if self.hs.config.expire_access_token: # TODO: it would be nice to get rid of this, but apparently some
raise # people use access tokens which aren't macaroons
ret = yield self._look_up_user_by_access_token(token) r = yield self._look_up_user_by_access_token(token)
defer.returnValue(r)
defer.returnValue(ret)
@defer.inlineCallbacks
def get_user_from_macaroon(self, macaroon_str, rights="access"):
try: try:
macaroon = pymacaroons.Macaroon.deserialize(macaroon_str)
user_id = self.get_user_id_from_macaroon(macaroon) user_id = self.get_user_id_from_macaroon(macaroon)
user = UserID.from_string(user_id) user = UserID.from_string(user_id)
@ -716,6 +713,30 @@ class Auth(object):
guest = True guest = True
if guest: if guest:
# Guest access tokens are not stored in the database (there can
# only be one access token per guest, anyway).
#
# In order to prevent guest access tokens being used as regular
# user access tokens (and hence getting around the invalidation
# process), we look up the user id and check that it is indeed
# a guest user.
#
# It would of course be much easier to store guest access
# tokens in the database as well, but that would break existing
# guest tokens.
stored_user = yield self.store.get_user_by_id(user_id)
if not stored_user:
raise AuthError(
self.TOKEN_NOT_FOUND_HTTP_STATUS,
"Unknown user_id %s" % user_id,
errcode=Codes.UNKNOWN_TOKEN
)
if not stored_user["is_guest"]:
raise AuthError(
self.TOKEN_NOT_FOUND_HTTP_STATUS,
"Guest access token used for regular user",
errcode=Codes.UNKNOWN_TOKEN
)
ret = { ret = {
"user": user, "user": user,
"is_guest": True, "is_guest": True,
@ -743,7 +764,7 @@ class Auth(object):
# macaroon. They probably should be. # macaroon. They probably should be.
# TODO: build the dictionary from the macaroon once the # TODO: build the dictionary from the macaroon once the
# above are fixed # above are fixed
ret = yield self._look_up_user_by_access_token(macaroon_str) ret = yield self._look_up_user_by_access_token(token)
if ret["user"] != user: if ret["user"] != user:
logger.error( logger.error(
"Macaroon user (%s) != DB user (%s)", "Macaroon user (%s) != DB user (%s)",

View file

@ -81,7 +81,7 @@ class RegistrationHandler(BaseHandler):
"User ID already taken.", "User ID already taken.",
errcode=Codes.USER_IN_USE, errcode=Codes.USER_IN_USE,
) )
user_data = yield self.auth.get_user_from_macaroon(guest_access_token) user_data = yield self.auth.get_user_by_access_token(guest_access_token)
if not user_data["is_guest"] or user_data["user"].localpart != localpart: if not user_data["is_guest"] or user_data["user"].localpart != localpart:
raise AuthError( raise AuthError(
403, 403,

View file

@ -12,17 +12,22 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
from tests import unittest
import pymacaroons
from mock import Mock
from twisted.internet import defer from twisted.internet import defer
from mock import Mock import synapse.handlers.auth
from synapse.api.auth import Auth from synapse.api.auth import Auth
from synapse.api.errors import AuthError from synapse.api.errors import AuthError
from synapse.types import UserID from synapse.types import UserID
from tests import unittest
from tests.utils import setup_test_homeserver, mock_getRawHeaders from tests.utils import setup_test_homeserver, mock_getRawHeaders
import pymacaroons
class TestHandlers(object):
def __init__(self, hs):
self.auth_handler = synapse.handlers.auth.AuthHandler(hs)
class AuthTestCase(unittest.TestCase): class AuthTestCase(unittest.TestCase):
@ -34,14 +39,17 @@ class AuthTestCase(unittest.TestCase):
self.hs = yield setup_test_homeserver(handlers=None) self.hs = yield setup_test_homeserver(handlers=None)
self.hs.get_datastore = Mock(return_value=self.store) self.hs.get_datastore = Mock(return_value=self.store)
self.hs.handlers = TestHandlers(self.hs)
self.auth = Auth(self.hs) self.auth = Auth(self.hs)
self.test_user = "@foo:bar" self.test_user = "@foo:bar"
self.test_token = "_test_token_" self.test_token = "_test_token_"
# this is overridden for the appservice tests
self.store.get_app_service_by_token = Mock(return_value=None)
@defer.inlineCallbacks @defer.inlineCallbacks
def test_get_user_by_req_user_valid_token(self): def test_get_user_by_req_user_valid_token(self):
self.store.get_app_service_by_token = Mock(return_value=None)
user_info = { user_info = {
"name": self.test_user, "name": self.test_user,
"token_id": "ditto", "token_id": "ditto",
@ -56,7 +64,6 @@ class AuthTestCase(unittest.TestCase):
self.assertEquals(requester.user.to_string(), self.test_user) self.assertEquals(requester.user.to_string(), self.test_user)
def test_get_user_by_req_user_bad_token(self): def test_get_user_by_req_user_bad_token(self):
self.store.get_app_service_by_token = Mock(return_value=None)
self.store.get_user_by_access_token = Mock(return_value=None) self.store.get_user_by_access_token = Mock(return_value=None)
request = Mock(args={}) request = Mock(args={})
@ -66,7 +73,6 @@ class AuthTestCase(unittest.TestCase):
self.failureResultOf(d, AuthError) self.failureResultOf(d, AuthError)
def test_get_user_by_req_user_missing_token(self): def test_get_user_by_req_user_missing_token(self):
self.store.get_app_service_by_token = Mock(return_value=None)
user_info = { user_info = {
"name": self.test_user, "name": self.test_user,
"token_id": "ditto", "token_id": "ditto",
@ -158,7 +164,7 @@ class AuthTestCase(unittest.TestCase):
macaroon.add_first_party_caveat("gen = 1") macaroon.add_first_party_caveat("gen = 1")
macaroon.add_first_party_caveat("type = access") macaroon.add_first_party_caveat("type = access")
macaroon.add_first_party_caveat("user_id = %s" % (user_id,)) macaroon.add_first_party_caveat("user_id = %s" % (user_id,))
user_info = yield self.auth.get_user_from_macaroon(macaroon.serialize()) user_info = yield self.auth.get_user_by_access_token(macaroon.serialize())
user = user_info["user"] user = user_info["user"]
self.assertEqual(UserID.from_string(user_id), user) self.assertEqual(UserID.from_string(user_id), user)
@ -168,6 +174,10 @@ class AuthTestCase(unittest.TestCase):
@defer.inlineCallbacks @defer.inlineCallbacks
def test_get_guest_user_from_macaroon(self): def test_get_guest_user_from_macaroon(self):
self.store.get_user_by_id = Mock(return_value={
"is_guest": True,
})
user_id = "@baldrick:matrix.org" user_id = "@baldrick:matrix.org"
macaroon = pymacaroons.Macaroon( macaroon = pymacaroons.Macaroon(
location=self.hs.config.server_name, location=self.hs.config.server_name,
@ -179,11 +189,12 @@ class AuthTestCase(unittest.TestCase):
macaroon.add_first_party_caveat("guest = true") macaroon.add_first_party_caveat("guest = true")
serialized = macaroon.serialize() serialized = macaroon.serialize()
user_info = yield self.auth.get_user_from_macaroon(serialized) user_info = yield self.auth.get_user_by_access_token(serialized)
user = user_info["user"] user = user_info["user"]
is_guest = user_info["is_guest"] is_guest = user_info["is_guest"]
self.assertEqual(UserID.from_string(user_id), user) self.assertEqual(UserID.from_string(user_id), user)
self.assertTrue(is_guest) self.assertTrue(is_guest)
self.store.get_user_by_id.assert_called_with(user_id)
@defer.inlineCallbacks @defer.inlineCallbacks
def test_get_user_from_macaroon_user_db_mismatch(self): def test_get_user_from_macaroon_user_db_mismatch(self):
@ -200,7 +211,7 @@ class AuthTestCase(unittest.TestCase):
macaroon.add_first_party_caveat("type = access") macaroon.add_first_party_caveat("type = access")
macaroon.add_first_party_caveat("user_id = %s" % (user,)) macaroon.add_first_party_caveat("user_id = %s" % (user,))
with self.assertRaises(AuthError) as cm: with self.assertRaises(AuthError) as cm:
yield self.auth.get_user_from_macaroon(macaroon.serialize()) yield self.auth.get_user_by_access_token(macaroon.serialize())
self.assertEqual(401, cm.exception.code) self.assertEqual(401, cm.exception.code)
self.assertIn("User mismatch", cm.exception.msg) self.assertIn("User mismatch", cm.exception.msg)
@ -220,7 +231,7 @@ class AuthTestCase(unittest.TestCase):
macaroon.add_first_party_caveat("type = access") macaroon.add_first_party_caveat("type = access")
with self.assertRaises(AuthError) as cm: with self.assertRaises(AuthError) as cm:
yield self.auth.get_user_from_macaroon(macaroon.serialize()) yield self.auth.get_user_by_access_token(macaroon.serialize())
self.assertEqual(401, cm.exception.code) self.assertEqual(401, cm.exception.code)
self.assertIn("No user caveat", cm.exception.msg) self.assertIn("No user caveat", cm.exception.msg)
@ -242,7 +253,7 @@ class AuthTestCase(unittest.TestCase):
macaroon.add_first_party_caveat("user_id = %s" % (user,)) macaroon.add_first_party_caveat("user_id = %s" % (user,))
with self.assertRaises(AuthError) as cm: with self.assertRaises(AuthError) as cm:
yield self.auth.get_user_from_macaroon(macaroon.serialize()) yield self.auth.get_user_by_access_token(macaroon.serialize())
self.assertEqual(401, cm.exception.code) self.assertEqual(401, cm.exception.code)
self.assertIn("Invalid macaroon", cm.exception.msg) self.assertIn("Invalid macaroon", cm.exception.msg)
@ -265,7 +276,7 @@ class AuthTestCase(unittest.TestCase):
macaroon.add_first_party_caveat("cunning > fox") macaroon.add_first_party_caveat("cunning > fox")
with self.assertRaises(AuthError) as cm: with self.assertRaises(AuthError) as cm:
yield self.auth.get_user_from_macaroon(macaroon.serialize()) yield self.auth.get_user_by_access_token(macaroon.serialize())
self.assertEqual(401, cm.exception.code) self.assertEqual(401, cm.exception.code)
self.assertIn("Invalid macaroon", cm.exception.msg) self.assertIn("Invalid macaroon", cm.exception.msg)
@ -293,12 +304,12 @@ class AuthTestCase(unittest.TestCase):
self.hs.clock.now = 5000 # seconds self.hs.clock.now = 5000 # seconds
self.hs.config.expire_access_token = True self.hs.config.expire_access_token = True
# yield self.auth.get_user_from_macaroon(macaroon.serialize()) # yield self.auth.get_user_by_access_token(macaroon.serialize())
# TODO(daniel): Turn on the check that we validate expiration, when we # TODO(daniel): Turn on the check that we validate expiration, when we
# validate expiration (and remove the above line, which will start # validate expiration (and remove the above line, which will start
# throwing). # throwing).
with self.assertRaises(AuthError) as cm: with self.assertRaises(AuthError) as cm:
yield self.auth.get_user_from_macaroon(macaroon.serialize()) yield self.auth.get_user_by_access_token(macaroon.serialize())
self.assertEqual(401, cm.exception.code) self.assertEqual(401, cm.exception.code)
self.assertIn("Invalid macaroon", cm.exception.msg) self.assertIn("Invalid macaroon", cm.exception.msg)
@ -327,6 +338,58 @@ class AuthTestCase(unittest.TestCase):
self.hs.clock.now = 5000 # seconds self.hs.clock.now = 5000 # seconds
self.hs.config.expire_access_token = True self.hs.config.expire_access_token = True
user_info = yield self.auth.get_user_from_macaroon(macaroon.serialize()) user_info = yield self.auth.get_user_by_access_token(macaroon.serialize())
user = user_info["user"] user = user_info["user"]
self.assertEqual(UserID.from_string(user_id), user) self.assertEqual(UserID.from_string(user_id), user)
@defer.inlineCallbacks
def test_cannot_use_regular_token_as_guest(self):
USER_ID = "@percy:matrix.org"
self.store.add_access_token_to_user = Mock()
token = yield self.hs.handlers.auth_handler.issue_access_token(
USER_ID, "DEVICE"
)
self.store.add_access_token_to_user.assert_called_with(
USER_ID, token, "DEVICE"
)
def get_user(tok):
if token != tok:
return None
return {
"name": USER_ID,
"is_guest": False,
"token_id": 1234,
"device_id": "DEVICE",
}
self.store.get_user_by_access_token = get_user
self.store.get_user_by_id = Mock(return_value={
"is_guest": False,
})
# check the token works
request = Mock(args={})
request.args["access_token"] = [token]
request.requestHeaders.getRawHeaders = mock_getRawHeaders()
requester = yield self.auth.get_user_by_req(request, allow_guest=True)
self.assertEqual(UserID.from_string(USER_ID), requester.user)
self.assertFalse(requester.is_guest)
# add an is_guest caveat
mac = pymacaroons.Macaroon.deserialize(token)
mac.add_first_party_caveat("guest = true")
guest_tok = mac.serialize()
# the token should *not* work now
request = Mock(args={})
request.args["access_token"] = [guest_tok]
request.requestHeaders.getRawHeaders = mock_getRawHeaders()
with self.assertRaises(AuthError) as cm:
yield self.auth.get_user_by_req(request, allow_guest=True)
self.assertEqual(401, cm.exception.code)
self.assertEqual("Guest access token used for regular user", cm.exception.msg)
self.store.get_user_by_id.assert_called_with(USER_ID)