diff options
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); } |