Require session ownership for additional users, capabilities and viewing reports

4 files changed, 216 insertions, 21 deletions

diff --git a/Emby.Server.Implementations/Session/SessionManager.cs b/Emby.Server.Implementations/Session/SessionManager.cs
index 8864ef6df1..16f39f1759 100644
--- a/Emby.Server.Implementations/Session/SessionManager.cs
+++ b/Emby.Server.Implementations/Session/SessionManager.cs
@@ -1569,6 +1569,25 @@ namespace Emby.Server.Implementations.Session
             }
         }
 
+        private void AssertCanAttachUser(SessionInfo controllingSession, Guid userId)
+        {
+            var controllingUserId = controllingSession.UserId;
+
+            // Playback reported by a session is also written to the user data of its additional users,
+            // so attaching anyone but the calling user requires administrative privileges.
+            if (controllingUserId.IsEmpty() || controllingUserId.Equals(userId))
+            {
+                return;
+            }
+
+            var controllingUser = _userManager.GetUserById(controllingUserId);
+            if (controllingUser is null
+                || !controllingUser.HasPermission(PermissionKind.IsAdministrator))
+            {
+                throw new SecurityException("The current user does not have permission to attach another user to a session.");
+            }
+        }
+
         /// <summary>
         /// Sends the restart required message.
         /// </summary>
@@ -1584,16 +1603,24 @@ namespace Emby.Server.Implementations.Session
         /// <summary>
         /// Adds the additional user.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="userId">The user identifier.</param>
-        /// <exception cref="UnauthorizedAccessException">Cannot modify additional users without authenticating first.</exception>
+        /// <exception cref="SecurityException">The controlling user is not allowed to attach the user to the session.</exception>
         /// <exception cref="ArgumentException">The requested user is already the primary user of the session.</exception>
-        public void AddAdditionalUser(string sessionId, Guid userId)
+        public void AddAdditionalUser(string controllingSessionId, string sessionId, Guid userId)
         {
             CheckDisposed();
 
             var session = GetSession(sessionId);
 
+            if (!string.IsNullOrEmpty(controllingSessionId))
+            {
+                var controllingSession = GetSession(controllingSessionId);
+                AssertCanControl(session, controllingSession);
+                AssertCanAttachUser(controllingSession, userId);
+            }
+
             if (session.UserId.Equals(userId))
             {
                 throw new ArgumentException("The requested user is already the primary user of the session.");
@@ -1601,7 +1628,8 @@ namespace Emby.Server.Implementations.Session
 
             if (session.AdditionalUsers.All(i => !i.UserId.Equals(userId)))
             {
-                var user = _userManager.GetUserById(userId);
+                var user = _userManager.GetUserById(userId)
+                    ?? throw new ArgumentException("The requested user does not exist.");
                 var newUser = new SessionUserInfo
                 {
                     UserId = userId,
@@ -1615,16 +1643,22 @@ namespace Emby.Server.Implementations.Session
         /// <summary>
         /// Removes the additional user.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="userId">The user identifier.</param>
-        /// <exception cref="UnauthorizedAccessException">Cannot modify additional users without authenticating first.</exception>
+        /// <exception cref="SecurityException">The controlling user is not allowed to control the session.</exception>
         /// <exception cref="ArgumentException">The requested user is already the primary user of the session.</exception>
-        public void RemoveAdditionalUser(string sessionId, Guid userId)
+        public void RemoveAdditionalUser(string controllingSessionId, string sessionId, Guid userId)
         {
             CheckDisposed();
 
             var session = GetSession(sessionId);
 
+            if (!string.IsNullOrEmpty(controllingSessionId))
+            {
+                AssertCanControl(session, GetSession(controllingSessionId));
+            }
+
             if (session.UserId.Equals(userId))
             {
                 throw new ArgumentException("The requested user is already the primary user of the session.");
@@ -1828,14 +1862,21 @@ namespace Emby.Server.Implementations.Session
         /// <summary>
         /// Reports the capabilities.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="capabilities">The capabilities.</param>
-        public void ReportCapabilities(string sessionId, ClientCapabilities capabilities)
+        /// <exception cref="SecurityException">The controlling user is not allowed to control the session.</exception>
+        public void ReportCapabilities(string controllingSessionId, string sessionId, ClientCapabilities capabilities)
         {
             CheckDisposed();
 
             var session = GetSession(sessionId);
 
+            if (!string.IsNullOrEmpty(controllingSessionId))
+            {
+                AssertCanControl(session, GetSession(controllingSessionId));
+            }
+
             ReportCapabilities(session, capabilities, true);
         }
 
@@ -1930,13 +1971,18 @@ namespace Emby.Server.Implementations.Session
         }
 
         /// <inheritdoc />
-        public void ReportNowViewingItem(string sessionId, string itemId)
+        public void ReportNowViewingItem(string controllingSessionId, string sessionId, string itemId)
         {
             ArgumentException.ThrowIfNullOrEmpty(itemId);
 
             var item = _libraryManager.GetItemById(new Guid(itemId));
             var session = GetSession(sessionId);
 
+            if (!string.IsNullOrEmpty(controllingSessionId))
+            {
+                AssertCanControl(session, GetSession(controllingSessionId));
+            }
+
             session.NowViewingItem = GetItemInfo(item, null);
         }
 
diff --git a/Jellyfin.Api/Controllers/SessionController.cs b/Jellyfin.Api/Controllers/SessionController.cs
index a144961d74..84c2d90fb1 100644
--- a/Jellyfin.Api/Controllers/SessionController.cs
+++ b/Jellyfin.Api/Controllers/SessionController.cs
@@ -306,11 +306,14 @@ public class SessionController : BaseJellyfinApiController
     [HttpPost("Sessions/{sessionId}/User/{userId}")]
     [Authorize]
     [ProducesResponseType(StatusCodes.Status204NoContent)]
-    public ActionResult AddUserToSession(
+    public async Task<ActionResult> AddUserToSession(
         [FromRoute, Required] string sessionId,
         [FromRoute, Required] Guid userId)
     {
-        _sessionManager.AddAdditionalUser(sessionId, userId);
+        _sessionManager.AddAdditionalUser(
+            await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false),
+            sessionId,
+            userId);
         return NoContent();
     }
 
@@ -324,11 +327,14 @@ public class SessionController : BaseJellyfinApiController
     [HttpDelete("Sessions/{sessionId}/User/{userId}")]
     [Authorize]
     [ProducesResponseType(StatusCodes.Status204NoContent)]
-    public ActionResult RemoveUserFromSession(
+    public async Task<ActionResult> RemoveUserFromSession(
         [FromRoute, Required] string sessionId,
         [FromRoute, Required] Guid userId)
     {
-        _sessionManager.RemoveAdditionalUser(sessionId, userId);
+        _sessionManager.RemoveAdditionalUser(
+            await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false),
+            sessionId,
+            userId);
         return NoContent();
     }
 
@@ -352,12 +358,13 @@ public class SessionController : BaseJellyfinApiController
         [FromQuery] bool supportsMediaControl = false,
         [FromQuery] bool supportsPersistentIdentifier = true)
     {
+        var currentSessionId = await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false);
         if (string.IsNullOrWhiteSpace(id))
         {
-            id = await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false);
+            id = currentSessionId;
         }
 
-        _sessionManager.ReportCapabilities(id, new ClientCapabilities
+        _sessionManager.ReportCapabilities(currentSessionId, id, new ClientCapabilities
         {
             PlayableMediaTypes = playableMediaTypes,
             SupportedCommands = supportedCommands,
@@ -381,12 +388,13 @@ public class SessionController : BaseJellyfinApiController
         [FromQuery] string? id,
         [FromBody, Required] ClientCapabilitiesDto capabilities)
     {
+        var currentSessionId = await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false);
         if (string.IsNullOrWhiteSpace(id))
         {
-            id = await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false);
+            id = currentSessionId;
         }
 
-        _sessionManager.ReportCapabilities(id, capabilities.ToClientCapabilities());
+        _sessionManager.ReportCapabilities(currentSessionId, id, capabilities.ToClientCapabilities());
 
         return NoContent();
     }
@@ -405,9 +413,9 @@ public class SessionController : BaseJellyfinApiController
         [FromQuery] string? sessionId,
         [FromQuery, Required] string? itemId)
     {
-        string session = sessionId ?? await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false);
+        var currentSessionId = await RequestHelpers.GetSessionId(_sessionManager, _userManager, HttpContext).ConfigureAwait(false);
 
-        _sessionManager.ReportNowViewingItem(session, itemId);
+        _sessionManager.ReportNowViewingItem(currentSessionId, sessionId ?? currentSessionId, itemId);
         return NoContent();
     }
 
diff --git a/MediaBrowser.Controller/Session/ISessionManager.cs b/MediaBrowser.Controller/Session/ISessionManager.cs
index c11c65c334..9acff745b9 100644
--- a/MediaBrowser.Controller/Session/ISessionManager.cs
+++ b/MediaBrowser.Controller/Session/ISessionManager.cs
@@ -238,23 +238,26 @@ namespace MediaBrowser.Controller.Session
         /// <summary>
         /// Adds the additional user.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="userId">The user identifier.</param>
-        void AddAdditionalUser(string sessionId, Guid userId);
+        void AddAdditionalUser(string controllingSessionId, string sessionId, Guid userId);
 
         /// <summary>
         /// Removes the additional user.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="userId">The user identifier.</param>
-        void RemoveAdditionalUser(string sessionId, Guid userId);
+        void RemoveAdditionalUser(string controllingSessionId, string sessionId, Guid userId);
 
         /// <summary>
         /// Reports the now viewing item.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="itemId">The item identifier.</param>
-        void ReportNowViewingItem(string sessionId, string itemId);
+        void ReportNowViewingItem(string controllingSessionId, string sessionId, string itemId);
 
         /// <summary>
         /// Authenticates the new session.
@@ -268,9 +271,10 @@ namespace MediaBrowser.Controller.Session
         /// <summary>
         /// Reports the capabilities.
         /// </summary>
+        /// <param name="controllingSessionId">The controlling session identifier.</param>
         /// <param name="sessionId">The session identifier.</param>
         /// <param name="capabilities">The capabilities.</param>
-        void ReportCapabilities(string sessionId, ClientCapabilities capabilities);
+        void ReportCapabilities(string controllingSessionId, string sessionId, ClientCapabilities capabilities);
 
         /// <summary>
         /// Reports the transcoding information.
diff --git a/tests/Jellyfin.Server.Implementations.Tests/SessionManager/SessionManagerTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SessionManager/SessionManagerTests.cs
index a5a67046d1..f803c69af2 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/SessionManager/SessionManagerTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/SessionManager/SessionManagerTests.cs
@@ -1,6 +1,9 @@
 using System;
+using System.Threading;
 using System.Threading.Tasks;
+using Jellyfin.Data;
 using Jellyfin.Database.Implementations.Entities;
+using Jellyfin.Database.Implementations.Enums;
 using MediaBrowser.Controller;
 using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Devices;
@@ -8,7 +11,9 @@ using MediaBrowser.Controller.Drawing;
 using MediaBrowser.Controller.Dto;
 using MediaBrowser.Controller.Events;
 using MediaBrowser.Controller.Library;
+using MediaBrowser.Controller.Net;
 using MediaBrowser.Controller.Session;
+using MediaBrowser.Model.Session;
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
@@ -108,4 +113,136 @@ public class SessionManagerTests
 
         return data;
     }
+
+    [Fact]
+    public async Task SendMessageCommand_Should_ThrowSecurityException_WhenControllingAnotherUsersSession()
+    {
+        var victim = new User("victim", "default", "default");
+        var attacker = new User("attacker", "default", "default");
+        await using var sessionManager = CreateSessionManager(victim, attacker);
+
+        var victimSession = await LogSessionActivity(sessionManager, victim);
+        var attackerSession = await LogSessionActivity(sessionManager, attacker);
+
+        await Assert.ThrowsAsync<SecurityException>(() => sessionManager.SendMessageCommand(
+            attackerSession.Id,
+            victimSession.Id,
+            new MessageCommand { Header = "Custom Message", Text = "test exploit!" },
+            CancellationToken.None));
+    }
+
+    [Fact]
+    public async Task SendMessageCommand_Should_Succeed_WhenAllowedToControlOtherUsers()
+    {
+        var victim = new User("victim", "default", "default");
+        var attacker = new User("controller", "default", "default");
+        attacker.SetPermission(PermissionKind.EnableRemoteControlOfOtherUsers, true);
+        await using var sessionManager = CreateSessionManager(victim, attacker);
+
+        var victimSession = await LogSessionActivity(sessionManager, victim);
+        var controllingSession = await LogSessionActivity(sessionManager, attacker);
+
+        await sessionManager.SendMessageCommand(
+            controllingSession.Id,
+            victimSession.Id,
+            new MessageCommand { Header = "Custom Message", Text = "hello" },
+            CancellationToken.None);
+    }
+
+    [Fact]
+    public async Task LogSessionActivity_Should_NotReuseAnotherUsersSession()
+    {
+        var victim = new User("victim", "default", "default");
+        var attacker = new User("attacker", "default", "default");
+        await using var sessionManager = CreateSessionManager(victim, attacker);
+
+        // Client name and device id are attacker controlled, so they must not identify a session on their own.
+        var victimSession = await LogSessionActivity(sessionManager, victim);
+        var attackerSession = await LogSessionActivity(sessionManager, attacker);
+
+        Assert.NotEqual(victimSession.Id, attackerSession.Id);
+        Assert.Equal(victim.Id, victimSession.UserId);
+    }
+
+    [Fact]
+    public async Task AddAdditionalUser_Should_ThrowSecurityException_WhenAttachingAnotherUser()
+    {
+        var attacker = new User("attacker", "default", "default");
+        var victim = new User("victim", "default", "default");
+        await using var sessionManager = CreateSessionManager(victim, attacker);
+
+        var attackerSession = await LogSessionActivity(sessionManager, attacker);
+
+        Assert.Throws<SecurityException>(() => sessionManager.AddAdditionalUser(attackerSession.Id, attackerSession.Id, victim.Id));
+    }
+
+    [Fact]
+    public async Task AddAdditionalUser_Should_Succeed_WhenCallerIsAdministrator()
+    {
+        var admin = new User("admin", "default", "default");
+        admin.SetPermission(PermissionKind.IsAdministrator, true);
+        var guest = new User("guest", "default", "default");
+        await using var sessionManager = CreateSessionManager(admin, guest);
+
+        var adminSession = await LogSessionActivity(sessionManager, admin);
+
+        sessionManager.AddAdditionalUser(adminSession.Id, adminSession.Id, guest.Id);
+
+        Assert.Contains(adminSession.AdditionalUsers, i => i.UserId.Equals(guest.Id));
+    }
+
+    [Fact]
+    public async Task RemoveAdditionalUser_Should_ThrowSecurityException_WhenModifyingAnotherUsersSession()
+    {
+        var victim = new User("victim", "default", "default");
+        var attacker = new User("attacker", "default", "default");
+        await using var sessionManager = CreateSessionManager(victim, attacker);
+
+        var victimSession = await LogSessionActivity(sessionManager, victim);
+        var attackerSession = await LogSessionActivity(sessionManager, attacker);
+
+        Assert.Throws<SecurityException>(() => sessionManager.RemoveAdditionalUser(attackerSession.Id, victimSession.Id, attacker.Id));
+    }
+
+    [Fact]
+    public async Task ReportCapabilities_Should_ThrowSecurityException_WhenReportingForAnotherUsersSession()
+    {
+        var victim = new User("victim", "default", "default");
+        var attacker = new User("attacker", "default", "default");
+        await using var sessionManager = CreateSessionManager(victim, attacker);
+
+        var victimSession = await LogSessionActivity(sessionManager, victim);
+        var attackerSession = await LogSessionActivity(sessionManager, attacker);
+
+        Assert.Throws<SecurityException>(() => sessionManager.ReportCapabilities(attackerSession.Id, victimSession.Id, new ClientCapabilities()));
+    }
+
+    private static Emby.Server.Implementations.Session.SessionManager CreateSessionManager(params User[] users)
+    {
+        var userManager = new Mock<IUserManager>();
+        foreach (var user in users)
+        {
+            userManager.Setup(i => i.GetUserById(user.Id)).Returns(user);
+        }
+
+        return new Emby.Server.Implementations.Session.SessionManager(
+            NullLogger<Emby.Server.Implementations.Session.SessionManager>.Instance,
+            Mock.Of<IEventManager>(),
+            Mock.Of<IUserDataManager>(),
+            Mock.Of<IServerConfigurationManager>(),
+            Mock.Of<ILibraryManager>(),
+            userManager.Object,
+            Mock.Of<IMusicManager>(),
+            Mock.Of<IDtoService>(),
+            Mock.Of<IImageProcessor>(),
+            Mock.Of<IServerApplicationHost>(),
+            Mock.Of<IDeviceManager>(),
+            Mock.Of<IMediaSourceManager>(),
+            Mock.Of<IHostApplicationLifetime>());
+    }
+
+    // All sessions are logged with the same client and device id on purpose, those values are taken
+    // from the request headers and are not bound to the access token of the calling user.
+    private static Task<SessionInfo> LogSessionActivity(ISessionManager sessionManager, User user)
+        => sessionManager.LogSessionActivity("Jellyfin Web", "1.0.0", "victim-tv-01", "device_name", "127.0.0.1", user);
 }