From 4b4f530914d3e2af29757a0fd1e8acf117fd40c0 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Tue, 21 Apr 2020 18:54:56 +0000 Subject: [PATCH] --- devdocs/todo.txt | 8 +++- .../Controllers/AttachmentController.cs | 44 +++++++++++++------ server/AyaNova/Controllers/AuthController.cs | 10 ----- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/devdocs/todo.txt b/devdocs/todo.txt index 70e16496..097c1ff3 100644 --- a/devdocs/todo.txt +++ b/devdocs/todo.txt @@ -10,7 +10,13 @@ todo: check attachment NOTES property is actually supported //todo: search tables in schema, I think there is a missing index here, need to look at the search query section again as it was changed several times from the original schema creation - +todo: log failed + - Download attempts with wrong key + - Add delay for failed download + - authentication attempts (or would this cause a possible DOS?) +todo: some kind of ops specific event log or system that triggers alerts to ops people, I guess notifications would be the word I'm looking for... + - Once that exists a lot of backfilling needs to be done for example attachment files missing notification etc etc etc + todo: can a user be locked out from the server end even though they posess a valid token? - and prevent download of images etc? diff --git a/server/AyaNova/Controllers/AttachmentController.cs b/server/AyaNova/Controllers/AttachmentController.cs index 46d2580b..2985f7db 100644 --- a/server/AyaNova/Controllers/AttachmentController.cs +++ b/server/AyaNova/Controllers/AttachmentController.cs @@ -36,7 +36,6 @@ namespace AyaNova.Api.Controllers [ApiVersion("8.0")] [Route("api/v{version:apiVersion}/[controller]")] [Produces("application/json")] - [Authorize] public class AttachmentController : ControllerBase { private readonly AyContext ct; @@ -118,6 +117,7 @@ namespace AyaNova.Api.Controllers /// /// /// NameValue list of filenames and attachment id's + [Authorize] [HttpPost] [DisableFormValueModelBinding] [RequestSizeLimit(10737418241)]//10737418240 = 10gb https://github.com/aspnet/Announcements/issues/267 @@ -269,6 +269,7 @@ namespace AyaNova.Api.Controllers /// /// /// Ok + [Authorize] [HttpDelete("{id}")] public async Task DeleteAttachmentAsync([FromRoute] long id) { @@ -313,11 +314,14 @@ namespace AyaNova.Api.Controllers /// Download a file attachment /// /// - /// + /// download token /// [HttpGet("download/{id}")] - public async Task DownloadAsync([FromRoute] long id, [FromQuery] string dlkey) + public async Task DownloadAsync([FromRoute] long id, [FromQuery] string t) { + int nFailedAuthDelay = 3000;//should be just long enough to make brute force a hassle but short enough to not annoy people who just mistyped their creds to login + + //NOTE this is the only unauthorized route as it needs to work with wiki url links and relies on the dlkey to work //copied from Rockfish //https://dotnetcoretutorials.com/2017/03/12/uploading-files-asp-net-core/ //https://stackoverflow.com/questions/45763149/asp-net-core-jwt-in-uri-query-parameter/45811270#45811270 @@ -325,47 +329,57 @@ namespace AyaNova.Api.Controllers return StatusCode(503, new ApiErrorResponse(serverState.ApiErrorCode, null, serverState.Reason)); - if (string.IsNullOrWhiteSpace(dlkey)) + //NOTE: this is a potentially dangerous route since it's not Authorized so we need to treat it like Auth route and not leak any + //useful information to bad actors and also ensure a delay to avoid brute force or DOS attacks + + if (string.IsNullOrWhiteSpace(t)) { - return NotFound(); + await Task.Delay(nFailedAuthDelay);//DOS protection + return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); } + + + //get user by key, if not found then reject //If user dlkeyexp has not expired then return file - var dlkeyUser = await ct.User.SingleOrDefaultAsync(m => m.DlKey == dlkey); + var dlkeyUser = await ct.User.SingleOrDefaultAsync(m => m.DlKey == t && m.Active == true); if (dlkeyUser == null) { - //don't want to leak information so just say not found - //return BadRequest(new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, "dlkey", "Download token not valid")); + await Task.Delay(nFailedAuthDelay);//DOS protection return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); } //Make sure the token provided is for the current user + //bugbug: Is this not a bug, there's no way to set this properly is there? long UserId = UserIdFromContext.Id(HttpContext.Items); if (UserId != dlkeyUser.Id) { - // return BadRequest(new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, "dlkey", "Download token not valid")); - return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); + await Task.Delay(nFailedAuthDelay);//DOS protection + return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); } var utcNow = new DateTimeOffset(DateTime.Now.ToUniversalTime(), TimeSpan.Zero); if (dlkeyUser.DlKeyExpire < utcNow.DateTime) { - // return BadRequest(new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, "dlkey", "Download token has expired")); - return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); + + await Task.Delay(nFailedAuthDelay);//DOS protection + return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); } //Ok, user has a valid download key and it's not expired yet so get the attachment record var dbObj = await ct.FileAttachment.SingleOrDefaultAsync(m => m.Id == id); if (dbObj == null) { + await Task.Delay(nFailedAuthDelay);//fishing protection return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND)); } //is this allowed? if (!Authorized.HasReadFullRole(HttpContext.Items, dbObj.AttachToObjectType)) { + await Task.Delay(nFailedAuthDelay);//DOS protection return StatusCode(403, new ApiNotAuthorizedResponse()); } @@ -376,7 +390,11 @@ namespace AyaNova.Api.Controllers { //TODO: this should trigger some kind of notification to the ops people //and a red light on the dashboard - return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND, null, $"Physical file {dbObj.StoredFileName} not found despite attachment record, this file is missing")); + + var errText = $"Physical file {dbObj.StoredFileName} not found despite attachment record, this file is missing"; + log.LogError(errText); + + return NotFound(new ApiErrorResponse(ApiErrorCode.NOT_FOUND, null, errText)); } //Log diff --git a/server/AyaNova/Controllers/AuthController.cs b/server/AyaNova/Controllers/AuthController.cs index f45229ae..6e1ed7df 100644 --- a/server/AyaNova/Controllers/AuthController.cs +++ b/server/AyaNova/Controllers/AuthController.cs @@ -177,16 +177,6 @@ namespace AyaNova.Api.Controllers } - // //If the user is inactive they may not login - // if (!u.Active) - // { - // //This is leaking information, instead just act like bad creds - // //return StatusCode(401, new ApiErrorResponse(ApiErrorCode.NOT_AUTHORIZED, null, "User deactivated")); - // return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED)); - // } - - - //build the key (JWT set in startup.cs) byte[] secretKey = System.Text.Encoding.ASCII.GetBytes(ServerBootConfig.AYANOVA_JWT_SECRET);