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 <neilalexander@users.noreply.github.com>

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 {
+		JSON: Challenge{
-			Completed []string              `json:"completed"`
+			Completed: u.Completed,
-			Flows     []userInteractiveFlow `json:"flows"`
+			Flows:     u.Flows,
-			Session   string                `json:"session"`
+			Session:   sessionID,
-			// TODO: Return any additional `params`
+			Params:    make(map[string]interface{}),
-			Params map[string]interface{} `json:"params"`
-		}{
-			u.Completed,
-			u.Flows,
-			sessionID,
-			make(map[string]interface{}),
 		},
 	}
 }

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

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),
+		sessions:                make(map[string][]authtypes.LoginType),
-		params:   make(map[string]registerRequest),
+		params:                  make(map[string]registerRequest),
-		timer:    make(map[string]*time.Timer),
+		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_\-=./]+$`)

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

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. 

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