Only allow device deletion if the session matches

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{}{},

clientapi/routing/register.go

@@ -76,6 +76,7 @@ type sessionsDict struct {
 	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() {
@@ -132,6 +134,7 @@ func newSessionsDict() *sessionsDict {
 		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_\-=./]+$`)

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")
+		}
 	})
 }

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