v0.5.4.1 - Security Hotfix (#1414)

* Updated our security file with our Huntr.

* Fixed a security vulnerability where through the API, an unauthorized user could delete/modify reading lists that did not belong to them.

Fixed a bug where when creating a reading list with the name of another users, the API would throw an exception (but reading list would still get created)

* Fixed a security vulnerability where through the API, an unauthorized user could delete/modify reading lists that did not belong to them.

Fixed a bug where when creating a reading list with the name of another users, the API would throw an exception (but reading list would still get created)

* Ensure all reading list apis are authorized

* Ensured all APIs require authentication, except those that explicitly don't. All APIs are default requiring Authentication.

Fixed a security vulnerability which would allow a user to take over an admin account.

* Fixed a bug where cover-upload would accept filenames that were not expected.

* Explicitly check that a user has access to the pdf file before we serve it back.

* Enabled lock out when invalid user auth occurs. After 5 invalid auths, the user account will be locked out for 10 mins.

* Version bump for hotfix release

API/Controllers/AccountController.cs

@@ -70,13 +70,21 @@ public class AccountController : BaseApiController
         /// </summary>
         /// <param name="resetPasswordDto"></param>
         /// <returns></returns>
+        [AllowAnonymous]
         [HttpPost("reset-password")]
         public async Task<ActionResult> UpdatePassword(ResetPasswordDto resetPasswordDto)
         {
+            // TODO: Log this request to Audit Table
             _logger.LogInformation("{UserName} is changing {ResetUser}'s password", User.GetUsername(), resetPasswordDto.UserName);
-            var user = await _userManager.Users.SingleAsync(x => x.UserName == resetPasswordDto.UserName);
 
-            if (resetPasswordDto.UserName != User.GetUsername() && !(User.IsInRole(PolicyConstants.AdminRole) || User.IsInRole(PolicyConstants.ChangePasswordRole)))
+            var user = await _userManager.Users.SingleOrDefaultAsync(x => x.UserName == resetPasswordDto.UserName);
+            if (user == null) return Ok(); // Don't report BadRequest as that would allow brute forcing to find accounts on system
+
+
+            if (resetPasswordDto.UserName == User.GetUsername() && !(User.IsInRole(PolicyConstants.ChangePasswordRole) || User.IsInRole(PolicyConstants.AdminRole)))
+                return Unauthorized("You are not permitted to this operation.");
+
+            if (resetPasswordDto.UserName != User.GetUsername() && !User.IsInRole(PolicyConstants.AdminRole))
                 return Unauthorized("You are not permitted to this operation.");
 
             var errors = await _accountService.ChangeUserPassword(user, resetPasswordDto.Password);
@@ -94,6 +102,7 @@ public async Task<ActionResult> UpdatePassword(ResetPasswordDto resetPasswordDto
         /// </summary>
         /// <param name="registerDto"></param>
         /// <returns></returns>
+        [AllowAnonymous]
         [HttpPost("register")]
         public async Task<ActionResult<UserDto>> RegisterFirstUser(RegisterDto registerDto)
         {
@@ -155,6 +164,7 @@ public async Task<ActionResult<UserDto>> RegisterFirstUser(RegisterDto registerD
         /// </summary>
         /// <param name="loginDto"></param>
         /// <returns></returns>
+        [AllowAnonymous]
         [HttpPost("login")]
         public async Task<ActionResult<UserDto>> Login(LoginDto loginDto)
         {
@@ -173,14 +183,14 @@ public async Task<ActionResult<UserDto>> Login(LoginDto loginDto)
                     "You are missing an email on your account. Please wait while we migrate your account.");
             }
 
-            if (!validPassword)
+            var result = await _signInManager
+                .CheckPasswordSignInAsync(user, loginDto.Password, true);
+
+            if (result.IsLockedOut)
             {
-                return Unauthorized("Your credentials are not correct");
+                return Unauthorized("You've been locked out from too many authorization attempts. Please wait 10 minutes.");
             }
 
-            var result = await _signInManager
-                .CheckPasswordSignInAsync(user, loginDto.Password, false);
-
             if (!result.Succeeded)
             {
                 return Unauthorized(result.IsNotAllowed ? "You must confirm your email first" : "Your credentials are not correct.");
@@ -212,6 +222,7 @@ public async Task<ActionResult<UserDto>> Login(LoginDto loginDto)
         /// </summary>
         /// <param name="tokenRequestDto"></param>
         /// <returns></returns>
+        [AllowAnonymous]
         [HttpPost("refresh-token")]
         public async Task<ActionResult<TokenRequestDto>> RefreshToken([FromBody] TokenRequestDto tokenRequestDto)
         {
@@ -462,6 +473,7 @@ await _emailService.SendConfirmationEmail(new ConfirmationEmailDto()
             return BadRequest("There was an error setting up your account. Please check the logs");
         }
 
+        [AllowAnonymous]
         [HttpPost("confirm-email")]
         public async Task<ActionResult<UserDto>> ConfirmEmail(ConfirmEmailDto dto)
         {

API/Controllers/AdminController.cs

@@ -1,5 +1,6 @@
 using System.Threading.Tasks;
 using API.Entities;
+using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Identity;
 using Microsoft.AspNetCore.Mvc;
 
@@ -18,6 +19,7 @@ public AdminController(UserManager<AppUser> userManager)
         /// Checks if an admin exists on the system. This is essentially a check to validate if the system has been setup.
         /// </summary>
         /// <returns></returns>
+        [AllowAnonymous]
         [HttpGet("exists")]
         public async Task<ActionResult<bool>> AdminExists()
         {

API/Controllers/BaseApiController.cs

@@ -1,10 +1,12 @@
-using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Authorization;
+using Microsoft.AspNetCore.Mvc;
 
 namespace API.Controllers
 {
     [ApiController]
     [Route("api/[controller]")]
+    [Authorize]
     public class BaseApiController : ControllerBase
     {
     }
-}
+}

API/Controllers/FallbackController.cs

@@ -1,24 +1,26 @@
 using System.IO;
 using API.Services;
+using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
 
-namespace API.Controllers
+namespace API.Controllers;
+
+[AllowAnonymous]
+public class FallbackController : Controller
 {
-    public class FallbackController : Controller
-    {
-        // ReSharper disable once S4487
-        // ReSharper disable once NotAccessedField.Local
-        private readonly ITaskScheduler _taskScheduler;
+    // ReSharper disable once S4487
+    // ReSharper disable once NotAccessedField.Local
+    private readonly ITaskScheduler _taskScheduler;
 
-        public FallbackController(ITaskScheduler taskScheduler)
-        {
-            // This is used to load TaskScheduler on startup without having to navigate to a Controller that uses.
-            _taskScheduler = taskScheduler;
-        }
+    public FallbackController(ITaskScheduler taskScheduler)
+    {
+        // This is used to load TaskScheduler on startup without having to navigate to a Controller that uses.
+        _taskScheduler = taskScheduler;
+    }
 
-        public ActionResult Index()
-        {
-            return PhysicalFile(Path.Combine(Directory.GetCurrentDirectory(), "wwwroot", "index.html"), "text/HTML");
-        }
+    public ActionResult Index()
+    {
+        return PhysicalFile(Path.Combine(Directory.GetCurrentDirectory(), "wwwroot", "index.html"), "text/HTML");
     }
 }
+

API/Controllers/ImageController.cs

@@ -137,6 +137,8 @@ public async Task<ActionResult> GetBookmarkImage(int chapterId, int pageNum, str
         [HttpGet("cover-upload")]
         public ActionResult GetCoverUploadImage(string filename)
         {
+            if (filename.Contains("..")) return BadRequest("Invalid Filename");
+
             var path = Path.Join(_directoryService.TempDirectory, filename);
             if (string.IsNullOrEmpty(path) || !_directoryService.FileSystem.File.Exists(path)) return BadRequest($"File does not exist");
             var format = _directoryService.FileSystem.Path.GetExtension(path).Replace(".", "");

API/Controllers/OPDSController.cs

@@ -17,10 +17,12 @@
 using API.Helpers;
 using API.Services;
 using Kavita.Common;
+using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
 
 namespace API.Controllers;
 
+[AllowAnonymous]
 public class OpdsController : BaseApiController
 {
     private readonly IUnitOfWork _unitOfWork;

API/Controllers/ReaderController.cs

@@ -57,6 +57,11 @@ public async Task<ActionResult> GetPdf(int chapterId)
             var chapter = await _cacheService.Ensure(chapterId);
             if (chapter == null) return BadRequest("There was an issue finding pdf file for reading");
 
+            // Validate the user has access to the PDF
+            var series = await _unitOfWork.SeriesRepository.GetSeriesForChapter(chapter.Id,
+                await _unitOfWork.UserRepository.GetUserIdByUsernameAsync(User.GetUsername()));
+            if (series == null) return BadRequest("Invalid Access");
+
             try
             {
                 var path = _cacheService.GetCachedFile(chapter);

API/Controllers/ReadingListController.cs

@@ -3,15 +3,18 @@
 using System.Threading.Tasks;
 using API.Comparators;
 using API.Data;
+using API.Data.Repositories;
 using API.DTOs.ReadingLists;
 using API.Entities;
 using API.Extensions;
 using API.Helpers;
 using API.SignalR;
+using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
 
 namespace API.Controllers
 {
+    [Authorize]
     public class ReadingListController : BaseApiController
     {
         private readonly IUnitOfWork _unitOfWork;
@@ -73,8 +76,18 @@ public async Task<ActionResult<IEnumerable<ReadingListItemDto>>> GetListForUser(
             var userId = await _unitOfWork.UserRepository.GetUserIdByUsernameAsync(User.GetUsername());
             var items = await _unitOfWork.ReadingListRepository.GetReadingListItemDtosByIdAsync(readingListId, userId);
             return Ok(items);
+        }
+
+        private async Task<AppUser?> UserHasReadingListAccess(int readingListId)
+        {
+            var user = await _unitOfWork.UserRepository.GetUserByUsernameAsync(User.GetUsername(),
+                AppUserIncludes.ReadingLists);
+            if (user.ReadingLists.SingleOrDefault(rl => rl.Id == readingListId) == null && !await _unitOfWork.UserRepository.IsUserAdminAsync(user))
+            {
+                return null;
+            }
 
-            //return Ok(await _unitOfWork.ReadingListRepository.AddReadingProgressModifiers(userId, items.ToList()));
+            return user;
         }
 
         /// <summary>
@@ -86,6 +99,11 @@ public async Task<ActionResult<IEnumerable<ReadingListItemDto>>> GetListForUser(
         public async Task<ActionResult> UpdateListItemPosition(UpdateReadingListPosition dto)
         {
             // Make sure UI buffers events
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
             var items = (await _unitOfWork.ReadingListRepository.GetReadingListItemsByIdAsync(dto.ReadingListId)).ToList();
             var item = items.Find(r => r.Id == dto.ReadingListItemId);
             items.Remove(item);
@@ -112,10 +130,15 @@ public async Task<ActionResult> UpdateListItemPosition(UpdateReadingListPosition
         [HttpPost("delete-item")]
         public async Task<ActionResult> DeleteListItem(UpdateReadingListPosition dto)
         {
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
+
             var readingList = await _unitOfWork.ReadingListRepository.GetReadingListByIdAsync(dto.ReadingListId);
             readingList.Items = readingList.Items.Where(r => r.Id != dto.ReadingListItemId).ToList();
 
-
             var index = 0;
             foreach (var readingListItem in readingList.Items)
             {
@@ -141,9 +164,14 @@ public async Task<ActionResult> DeleteListItem(UpdateReadingListPosition dto)
         [HttpPost("remove-read")]
         public async Task<ActionResult> DeleteReadFromList([FromQuery] int readingListId)
         {
-            var userId = await _unitOfWork.UserRepository.GetUserIdByUsernameAsync(User.GetUsername());
-            var items = await _unitOfWork.ReadingListRepository.GetReadingListItemDtosByIdAsync(readingListId, userId);
-            items = await _unitOfWork.ReadingListRepository.AddReadingProgressModifiers(userId, items.ToList());
+            var user = await UserHasReadingListAccess(readingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
+
+            var items = await _unitOfWork.ReadingListRepository.GetReadingListItemDtosByIdAsync(readingListId, user.Id);
+            items = await _unitOfWork.ReadingListRepository.AddReadingProgressModifiers(user.Id, items.ToList());
 
             // Collect all Ids to remove
             var itemIdsToRemove = items.Where(item => item.PagesRead == item.PagesTotal).Select(item => item.Id);
@@ -176,15 +204,13 @@ public async Task<ActionResult> DeleteReadFromList([FromQuery] int readingListId
         [HttpDelete]
         public async Task<ActionResult> DeleteList([FromQuery] int readingListId)
         {
-            var user = await _unitOfWork.UserRepository.GetUserWithReadingListsByUsernameAsync(User.GetUsername());
-            var isAdmin = await _unitOfWork.UserRepository.IsUserAdminAsync(user);
-            var readingList = user.ReadingLists.SingleOrDefault(r => r.Id == readingListId);
-            if (readingList == null && !isAdmin)
+            var user = await UserHasReadingListAccess(readingListId);
+            if (user == null)
             {
-                return BadRequest("User is not associated with this reading list");
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
             }
 
-            readingList = await _unitOfWork.ReadingListRepository.GetReadingListByIdAsync(readingListId);
+            var readingList = await _unitOfWork.ReadingListRepository.GetReadingListByIdAsync(readingListId);
 
             user.ReadingLists.Remove(readingList);
 
@@ -213,13 +239,14 @@ public async Task<ActionResult<ReadingListDto>> CreateList(CreateReadingListDto
                 return BadRequest("A list of this name already exists");
             }
 
-            user.ReadingLists.Add(DbFactory.ReadingList(dto.Title, string.Empty, false));
+            var readingList = DbFactory.ReadingList(dto.Title, string.Empty, false);
+            user.ReadingLists.Add(readingList);
 
             if (!_unitOfWork.HasChanges()) return BadRequest("There was a problem creating list");
 
             await _unitOfWork.CommitAsync();
 
-            return Ok(await _unitOfWork.ReadingListRepository.GetReadingListDtoByTitleAsync(dto.Title));
+            return Ok(await _unitOfWork.ReadingListRepository.GetReadingListDtoByTitleAsync(user.Id, dto.Title));
         }
 
         /// <summary>
@@ -233,7 +260,11 @@ public async Task<ActionResult> UpdateList(UpdateReadingListDto dto)
             var readingList = await _unitOfWork.ReadingListRepository.GetReadingListByIdAsync(dto.ReadingListId);
             if (readingList == null) return BadRequest("List does not exist");
 
-
+            var user = await UserHasReadingListAccess(readingList.Id);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
 
             if (!string.IsNullOrEmpty(dto.Title))
             {
@@ -277,7 +308,12 @@ public async Task<ActionResult> UpdateList(UpdateReadingListDto dto)
         [HttpPost("update-by-series")]
         public async Task<ActionResult> UpdateListBySeries(UpdateReadingListBySeriesDto dto)
         {
-            var user = await _unitOfWork.UserRepository.GetUserWithReadingListsByUsernameAsync(User.GetUsername());
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
+
             var readingList = user.ReadingLists.SingleOrDefault(l => l.Id == dto.ReadingListId);
             if (readingList == null) return BadRequest("Reading List does not exist");
             var chapterIdsForSeries =
@@ -314,7 +350,11 @@ public async Task<ActionResult> UpdateListBySeries(UpdateReadingListBySeriesDto
         [HttpPost("update-by-multiple")]
         public async Task<ActionResult> UpdateListByMultiple(UpdateReadingListByMultipleDto dto)
         {
-            var user = await _unitOfWork.UserRepository.GetUserWithReadingListsByUsernameAsync(User.GetUsername());
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
             var readingList = user.ReadingLists.SingleOrDefault(l => l.Id == dto.ReadingListId);
             if (readingList == null) return BadRequest("Reading List does not exist");
 
@@ -354,7 +394,11 @@ public async Task<ActionResult> UpdateListByMultiple(UpdateReadingListByMultiple
         [HttpPost("update-by-multiple-series")]
         public async Task<ActionResult> UpdateListByMultipleSeries(UpdateReadingListByMultipleSeriesDto dto)
         {
-            var user = await _unitOfWork.UserRepository.GetUserWithReadingListsByUsernameAsync(User.GetUsername());
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
             var readingList = user.ReadingLists.SingleOrDefault(l => l.Id == dto.ReadingListId);
             if (readingList == null) return BadRequest("Reading List does not exist");
 
@@ -388,9 +432,14 @@ public async Task<ActionResult> UpdateListByMultipleSeries(UpdateReadingListByMu
         [HttpPost("update-by-volume")]
         public async Task<ActionResult> UpdateListByVolume(UpdateReadingListByVolumeDto dto)
         {
-            var user = await _unitOfWork.UserRepository.GetUserWithReadingListsByUsernameAsync(User.GetUsername());
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
             var readingList = user.ReadingLists.SingleOrDefault(l => l.Id == dto.ReadingListId);
             if (readingList == null) return BadRequest("Reading List does not exist");
+
             var chapterIdsForVolume =
                 (await _unitOfWork.ChapterRepository.GetChaptersAsync(dto.VolumeId)).Select(c => c.Id).ToList();
 
@@ -419,7 +468,11 @@ public async Task<ActionResult> UpdateListByVolume(UpdateReadingListByVolumeDto
         [HttpPost("update-by-chapter")]
         public async Task<ActionResult> UpdateListByChapter(UpdateReadingListByChapterDto dto)
         {
-            var user = await _unitOfWork.UserRepository.GetUserWithReadingListsByUsernameAsync(User.GetUsername());
+            var user = await UserHasReadingListAccess(dto.ReadingListId);
+            if (user == null)
+            {
+                return BadRequest("You do not have permissions on this reading list or the list doesn't exist");
+            }
             var readingList = user.ReadingLists.SingleOrDefault(l => l.Id == dto.ReadingListId);
             if (readingList == null) return BadRequest("Reading List does not exist");
 

API/Controllers/ThemeController.cs

@@ -24,6 +24,7 @@ public ThemeController(IUnitOfWork unitOfWork, IThemeService themeService, ITask
         _taskScheduler = taskScheduler;
     }
 
+    [AllowAnonymous]
     [HttpGet]
     public async Task<ActionResult<IEnumerable<SiteThemeDto>>> GetThemes()
     {