From 5510ccaa78cb043bf96a614af17c2644bc1d7377 Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Fri, 25 Feb 2022 15:04:09 +0100 Subject: [PATCH] Only allow device deletion if the session matches --- clientapi/routing/device.go | 36 ++++++++++++++++++++++++++++++ clientapi/routing/register.go | 29 +++++++++++++++++++----- clientapi/routing/register_test.go | 10 +++++++++ sytest-whitelist | 2 +- 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/clientapi/routing/device.go b/clientapi/routing/device.go index 9f54a625a..a273dbd46 100644 --- a/clientapi/routing/device.go +++ b/clientapi/routing/device.go @@ -25,10 +25,12 @@ 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 type deviceJSON struct { + UserID string `json:"user_id"` DeviceID string `json:"device_id"` DisplayName string `json:"display_name"` LastSeenIP string `json:"last_seen_ip"` @@ -77,6 +79,7 @@ func GetDeviceByID( return util.JSONResponse{ Code: http.StatusOK, JSON: deviceJSON{ + UserID: targetDevice.UserID, DeviceID: targetDevice.ID, DisplayName: targetDevice.DisplayName, LastSeenIP: stripIPPort(targetDevice.LastSeenIP), @@ -102,6 +105,7 @@ func GetDevicesByLocalpart( for _, dev := range queryRes.Devices { res.Devices = append(res.Devices, deviceJSON{ + UserID: dev.UserID, DeviceID: dev.ID, DisplayName: dev.DisplayName, LastSeenIP: stripIPPort(dev.LastSeenIP), @@ -163,6 +167,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 +185,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 +235,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 10cfa4325..279ae22d7 100644 --- a/clientapi/routing/register.go +++ b/clientapi/routing/register.go @@ -73,9 +73,10 @@ func init() { // It shouldn't be passed by value because it contains a mutex. type sessionsDict struct { sync.RWMutex - sessions map[string][]authtypes.LoginType - params map[string]registerRequest - timer map[string]*time.Timer + sessions map[string][]authtypes.LoginType + params map[string]registerRequest + timer map[string]*time.Timer + sessionToDevice map[string]string } // defaultTimeout is the timeout used to clean up sessions @@ -115,6 +116,7 @@ func (d *sessionsDict) deleteSession(sessionID string) { defer d.Unlock() delete(d.params, sessionID) delete(d.sessions, sessionID) + delete(d.sessionToDevice, sessionID) // stop the timer, e.g. because the registration was completed if t, ok := d.timer[sessionID]; ok { if !t.Stop() { @@ -129,9 +131,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), + sessionToDevice: make(map[string]string), } } @@ -165,6 +168,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.sessionToDevice[sessionID] = deviceID +} + +func (d *sessionsDict) getDeviceToDelete(sessionID string) (string, bool) { + d.RLock() + defer d.RUnlock() + deviceID, ok := d.sessionToDevice[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 c6b7e61cf..520f5ddeb 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/sytest-whitelist b/sytest-whitelist index 12522cfb3..99ef1a5b1 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -604,4 +604,4 @@ 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