Skip to content
Permalink
Browse files
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
  • Loading branch information
majora2007 committed Aug 8, 2022
1 parent 6d0b18c commit 9c31f7e7c81b919923cb2e3857439ec0d16243e4
Show file tree
Hide file tree
Showing 16 changed files with 171 additions and 50 deletions.
@@ -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)
{
@@ -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()
{
@@ -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
{
}
}
}
@@ -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");
}
}

@@ -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(".", "");
@@ -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;
@@ -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);
@@ -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");

@@ -24,6 +24,7 @@ public ThemeController(IUnitOfWork unitOfWork, IThemeService themeService, ITask
_taskScheduler = taskScheduler;
}

[AllowAnonymous]
[HttpGet]
public async Task<ActionResult<IEnumerable<SiteThemeDto>>> GetThemes()
{

0 comments on commit 9c31f7e

Please sign in to comment.