From cda2452ba00afffa9a73870ca09047ce52dd28c7 Mon Sep 17 00:00:00 2001 From: S7evinK <2353100+S7evinK@users.noreply.github.com> Date: Tue, 1 Mar 2022 17:39:57 +0100 Subject: [PATCH] Only allow device deletion from session UIA was initiated from (#2235) * Only allow device deletion if the session matches * Make the challenge response available to other packages * Remove userID, as it's not in the spec * Remove tests * Add passing test & remove obsolete config * Rename field, add comment Co-authored-by: Neil Alexander --- clientapi/auth/user_interactive.go | 24 ++++++++++++---------- clientapi/routing/device.go | 33 ++++++++++++++++++++++++++++++ clientapi/routing/register.go | 26 ++++++++++++++++++++--- clientapi/routing/register_test.go | 10 +++++++++ dendrite-config.yaml | 5 ----- sytest-whitelist | 2 ++ 6 files changed, 81 insertions(+), 19 deletions(-) diff --git a/clientapi/auth/user_interactive.go b/clientapi/auth/user_interactive.go index 9cab7956..4db75809 100644 --- a/clientapi/auth/user_interactive.go +++ b/clientapi/auth/user_interactive.go @@ -144,21 +144,23 @@ func (u *UserInteractive) AddCompletedStage(sessionID, authType string) { delete(u.Sessions, sessionID) } +type Challenge struct { + Completed []string `json:"completed"` + Flows []userInteractiveFlow `json:"flows"` + Session string `json:"session"` + // TODO: Return any additional `params` + Params map[string]interface{} `json:"params"` +} + // Challenge returns an HTTP 401 with the supported flows for authenticating func (u *UserInteractive) Challenge(sessionID string) *util.JSONResponse { return &util.JSONResponse{ Code: 401, - JSON: struct { - Completed []string `json:"completed"` - Flows []userInteractiveFlow `json:"flows"` - Session string `json:"session"` - // TODO: Return any additional `params` - Params map[string]interface{} `json:"params"` - }{ - u.Completed, - u.Flows, - sessionID, - make(map[string]interface{}), + JSON: Challenge{ + Completed: u.Completed, + Flows: u.Flows, + Session: sessionID, + Params: make(map[string]interface{}), }, } } diff --git a/clientapi/routing/device.go b/clientapi/routing/device.go index 9f54a625..161bc273 100644 --- a/clientapi/routing/device.go +++ b/clientapi/routing/device.go @@ -25,6 +25,7 @@ import ( "github.com/matrix-org/dendrite/userapi/api" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" + "github.com/tidwall/gjson" ) // https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-client-r0-devices @@ -163,6 +164,15 @@ func DeleteDeviceById( req *http.Request, userInteractiveAuth *auth.UserInteractive, userAPI api.UserInternalAPI, device *api.Device, deviceID string, ) util.JSONResponse { + var ( + deleteOK bool + sessionID string + ) + defer func() { + if deleteOK { + sessions.deleteSession(sessionID) + } + }() ctx := req.Context() defer req.Body.Close() // nolint:errcheck bodyBytes, err := ioutil.ReadAll(req.Body) @@ -172,8 +182,29 @@ func DeleteDeviceById( JSON: jsonerror.BadJSON("The request body could not be read: " + err.Error()), } } + + // check that we know this session, and it matches with the device to delete + s := gjson.GetBytes(bodyBytes, "auth.session").Str + if dev, ok := sessions.getDeviceToDelete(s); ok { + if dev != deviceID { + return util.JSONResponse{ + Code: http.StatusForbidden, + JSON: jsonerror.Forbidden("session & device mismatch"), + } + } + } + + if s != "" { + sessionID = s + } + login, errRes := userInteractiveAuth.Verify(ctx, bodyBytes, device) if errRes != nil { + switch data := errRes.JSON.(type) { + case auth.Challenge: + sessions.addDeviceToDelete(data.Session, deviceID) + default: + } return *errRes } @@ -201,6 +232,8 @@ func DeleteDeviceById( return jsonerror.InternalServerError() } + deleteOK = true + return util.JSONResponse{ Code: http.StatusOK, JSON: struct{}{}, diff --git a/clientapi/routing/register.go b/clientapi/routing/register.go index 10cfa432..c9787659 100644 --- a/clientapi/routing/register.go +++ b/clientapi/routing/register.go @@ -76,6 +76,10 @@ type sessionsDict struct { sessions map[string][]authtypes.LoginType params map[string]registerRequest timer map[string]*time.Timer + // deleteSessionToDeviceID protects requests to DELETE /devices/{deviceID} from being abused. + // If a UIA session is started by trying to delete device1, and then UIA is completed by deleting device2, + // the delete request will fail for device2 since the UIA was initiated by trying to delete device1. + deleteSessionToDeviceID map[string]string } // defaultTimeout is the timeout used to clean up sessions @@ -115,6 +119,7 @@ func (d *sessionsDict) deleteSession(sessionID string) { defer d.Unlock() delete(d.params, sessionID) delete(d.sessions, sessionID) + delete(d.deleteSessionToDeviceID, sessionID) // stop the timer, e.g. because the registration was completed if t, ok := d.timer[sessionID]; ok { if !t.Stop() { @@ -129,9 +134,10 @@ func (d *sessionsDict) deleteSession(sessionID string) { func newSessionsDict() *sessionsDict { return &sessionsDict{ - sessions: make(map[string][]authtypes.LoginType), - params: make(map[string]registerRequest), - timer: make(map[string]*time.Timer), + sessions: make(map[string][]authtypes.LoginType), + params: make(map[string]registerRequest), + timer: make(map[string]*time.Timer), + deleteSessionToDeviceID: make(map[string]string), } } @@ -165,6 +171,20 @@ func (d *sessionsDict) addCompletedSessionStage(sessionID string, stage authtype d.sessions[sessionID] = append(sessions.sessions[sessionID], stage) } +func (d *sessionsDict) addDeviceToDelete(sessionID, deviceID string) { + d.startTimer(defaultTimeOut, sessionID) + d.Lock() + defer d.Unlock() + d.deleteSessionToDeviceID[sessionID] = deviceID +} + +func (d *sessionsDict) getDeviceToDelete(sessionID string) (string, bool) { + d.RLock() + defer d.RUnlock() + deviceID, ok := d.deleteSessionToDeviceID[sessionID] + return deviceID, ok +} + var ( sessions = newSessionsDict() validUsernameRegex = regexp.MustCompile(`^[0-9a-z_\-=./]+$`) diff --git a/clientapi/routing/register_test.go b/clientapi/routing/register_test.go index c6b7e61c..520f5dde 100644 --- a/clientapi/routing/register_test.go +++ b/clientapi/routing/register_test.go @@ -242,6 +242,7 @@ func TestSessionCleanUp(t *testing.T) { s.addParams(dummySession, registerRequest{Username: "Testing"}) s.addCompletedSessionStage(dummySession, authtypes.LoginTypeRecaptcha) s.addCompletedSessionStage(dummySession, authtypes.LoginTypeDummy) + s.addDeviceToDelete(dummySession, "dummyDevice") s.getCompletedStages(dummySession) // reset the timer with a lower timeout s.startTimer(time.Millisecond, dummySession) @@ -249,5 +250,14 @@ func TestSessionCleanUp(t *testing.T) { if data, ok := s.getParams(dummySession); ok { t.Errorf("expected session to be deleted: %+v", data) } + if _, ok := s.timer[dummySession]; ok { + t.Error("expected timer to be delete") + } + if _, ok := s.sessions[dummySession]; ok { + t.Error("expected session to be delete") + } + if _, ok := s.getDeviceToDelete(dummySession); ok { + t.Error("expected session to device to be delete") + } }) } \ No newline at end of file diff --git a/dendrite-config.yaml b/dendrite-config.yaml index 6d086ed7..533b5c95 100644 --- a/dendrite-config.yaml +++ b/dendrite-config.yaml @@ -345,11 +345,6 @@ user_api: max_open_conns: 10 max_idle_conns: 2 conn_max_lifetime: -1 - device_database: - connection_string: file:userapi_devices.db - max_open_conns: 10 - max_idle_conns: 2 - conn_max_lifetime: -1 # The length of time that a token issued for a relying party from # /_matrix/client/r0/user/{userId}/openid/request_token endpoint # is considered to be valid in milliseconds. diff --git a/sytest-whitelist b/sytest-whitelist index 2dd56d26..0cb80b7b 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -605,4 +605,6 @@ Remote banned user is kicked and may not rejoin until unbanned registration remembers parameters registration accepts non-ascii passwords registration with inhibit_login inhibits login +The operation must be consistent through an interactive authentication session +Multiple calls to /sync should not cause 500 errors