[PM-38165] Improve Data Protection error handling and constrain plaintext input#7734
[PM-38165] Improve Data Protection error handling and constrain plaintext input#7734harr1424 wants to merge 2 commits into
Conversation
harr1424
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7734 +/- ##
==========================================
+ Coverage 60.55% 65.02% +4.47%
==========================================
Files 2142 2145 +3
Lines 94847 95055 +208
Branches 8487 8508 +21
==========================================
+ Hits 57430 61809 +4379
+ Misses 35401 31130 -4271
- Partials 2016 2116 +100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // The "P|" prefix is a server-internal sentinel for Data-Protection-wrapped values. | ||
| // Clients must not submit a value starting with this prefix. | ||
| if (send.Emails.StartsWith(Constants.DatabaseFieldProtectedPrefix)) | ||
| { | ||
| throw new BadRequestException("The Emails field contains an invalid character sequence."); | ||
| } |
There was a problem hiding this comment.
You don't need this check the issue isn't new emails being submitted with the P| prefix it was the short period of time where data protection wasn't applied to the column so there was a chance that the prefix was mixed between columns that were protected and weren't protected. Since all brand new stuff will be protected it is safe to have that prefix. The issue was reading from the database.
There was a problem hiding this comment.
I agree, but decided to retain this guard in anticipation of a HackerOne report and future edge case:
If the plain-text input begins with P|, then logic within SendRepository will assume it has been protected using Data Protection, which means that subsequently this string will fail decryption.
| private bool ProtectData(Send send) | ||
| { | ||
| if (send.Emails == null || send.Emails.StartsWith(Constants.DatabaseFieldProtectedPrefix)) | ||
| try | ||
| { | ||
| return; | ||
| } | ||
| if (string.IsNullOrWhiteSpace(send.Emails) || | ||
| send.Emails.StartsWith(Constants.DatabaseFieldProtectedPrefix)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| send.Emails = string.Concat(Constants.DatabaseFieldProtectedPrefix, | ||
| _dataProtector.Protect(send.Emails)); | ||
|
|
||
| send.Emails = string.Concat(Constants.DatabaseFieldProtectedPrefix, | ||
| _dataProtector.Protect(send.Emails)); | ||
| return true; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "An exception occurred while protecting Emauls for Send {SendId}", send.Id); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't do this, there are no known issues of protecting the data and this would only exist to cover up an issue if it did occur. I think now that we know the the issue with unprotecting the data is I would change the try/catch from the unprotect call as well and rethrow the exception after logging the send id that caused the issue
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "Failed to unprotect Emails for Send {SendId}.", send.Id); | ||
| _logger.LogWarning(ex, "An exception occurred while unprotecting Emails for Send {SendId}", send.Id); | ||
| return false; |
There was a problem hiding this comment.
Keep this as CryptographicException the more specific exception type the better. I also think this method should still throw on failures as to keep them from being covered up. The important part of fixing the delete send jobs was just not unprotecting the sends for deletion. The fact that this bug lead to a 500 error for a user is likely the only reason we were able to uncover it. If we just logged a warning and filtered it out it could have been much more easily missed.
if (send.Emails.Length == 4000)
{
_logger.LogError("Emails column for Send {SendId} is max length and may have been truncated.", send.Id);
throw;
}
else
{
_logger.LogError("Failed to unprotect Emails for Send {SendId}", send.Id);
throw;
}
| [Fact] | ||
| public async Task CreateAsync_WhenProtectFails_ThrowsBeforePersisting() | ||
| { | ||
| // ProtectDataAndSaveAsync must throw before invoking saveTask() so that a | ||
| // Data Protection failure cannot result in plaintext Emails reaching the | ||
| // [dbo].[Send].[Emails] column. Dummy connection strings are safe here | ||
| // because the throw happens before any SqlConnection is opened. | ||
| var repository = new SendRepository( | ||
| connectionString: "unused-but-non-empty", | ||
| readOnlyConnectionString: "unused-but-non-empty", | ||
| dataProtectionProvider: new ThrowingDataProtectionProvider(), | ||
| logger: NullLogger<SendRepository>.Instance); | ||
|
|
||
| var send = new Send | ||
| { | ||
| Type = SendType.Text, | ||
| Data = "{\"Text\":\"2.t|t|t\"}", | ||
| Key = "2.t|t|t", | ||
| Emails = "user@example.com", | ||
| DeletionDate = DateTime.UtcNow.AddDays(7), | ||
| }; | ||
|
|
||
| var exception = await Assert.ThrowsAsync<InvalidOperationException>( | ||
| () => repository.CreateAsync(send)); | ||
| Assert.Contains("Emails could not be protected", exception.Message); | ||
| } | ||
|
|
||
| private sealed class ThrowingDataProtectionProvider : IDataProtectionProvider | ||
| { | ||
| public IDataProtector CreateProtector(string purpose) => new ThrowingDataProtector(); | ||
| } | ||
|
|
||
| private sealed class ThrowingDataProtector : IDataProtector | ||
| { | ||
| public IDataProtector CreateProtector(string purpose) => this; | ||
|
|
||
| public byte[] Protect(byte[] plaintext) => | ||
| throw new CryptographicException("Test-induced Protect failure"); | ||
|
|
||
| public byte[] Unprotect(byte[] protectedData) => | ||
| throw new CryptographicException("Test-induced Unprotect failure"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this test helps us, this is asserting that failures to Protect lead to a different exception but we never encountered a failure during protection. I would recommend writing a true regression test in the test/Infrastructure.IntegrationTest project that tries to save a long email column and have it get truncated. After all my recommended changes the before and after will both lead to a cryptographic exception but when it was truncated it will lead to a different log message that you can assert on.
^ That might need a minor change to the test project to include a test logger that is easy to assert on so if you have troubles with that please reach out and I can help make that happen.
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38165
📔 Objective
This PR improves error handling surrounding the use of ASP.NET Data Protection on the
Send.Emailsproperty. Additionally, it limits plaintext content ofEmailsto 2,500 characters or less, so that protection yields encrypted values well below the 4,000 character limit.