From bb8060911521b94260e7238360c38eb591b19c98 Mon Sep 17 00:00:00 2001 From: Robert Daunce Date: Tue, 15 Jun 2021 13:32:17 -0400 Subject: [PATCH 1/4] split storage key and encryption key The .NET port did not convert the storage of encrypted passwords correctly. It was using the encryption key as the dictionary key, storing the encrypted password and encrytion key in the same location. This makes the encrypted password decryptable if someone were to compromise the underlying data store. This commit creates a separate storage key for lookup purposes and does not store the encryption key. A separate token is created that includes the storage key and the encryption key that is only used to generate the URL. See https://github.com/pinterest/snappass/issues/63 for additional conversation on the original source repo. --- .../Controllers/PasswordController.cs | 9 +++-- Snappass.NET/Controllers/ShareController.cs | 8 ++-- Snappass.NET/Encryption.cs | 37 +++++++++++++++---- Snappass.NET/Models/GeneratedPassword.cs | 4 +- 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/Snappass.NET/Controllers/PasswordController.cs b/Snappass.NET/Controllers/PasswordController.cs index d508bf7..bf1ce4f 100644 --- a/Snappass.NET/Controllers/PasswordController.cs +++ b/Snappass.NET/Controllers/PasswordController.cs @@ -18,13 +18,14 @@ public PasswordController(IMemoryStore memoryStore, ILogger [HttpGet] public IActionResult Preview(string key) { - if (!_memoryStore.Has(key)) + (string storageKey, string encryptionKey) = Encryption.ParseToken(key); + if (!_memoryStore.Has(storageKey)) { - _logger.LogWarning($@"password with key {key} requested, but not found"); + _logger.LogWarning($@"password with key {storageKey} requested, but not found"); return NotFound(); } - string encryptedPassword = _memoryStore.Retrieve(key); - string decrypted = Encryption.Decrypt(encryptedPassword, key); + string encryptedPassword = _memoryStore.Retrieve(storageKey); + string decrypted = Encryption.Decrypt(encryptedPassword, encryptionKey); return View("Preview", new PreviewModel { Key = decrypted }); } } diff --git a/Snappass.NET/Controllers/ShareController.cs b/Snappass.NET/Controllers/ShareController.cs index acb0593..96ed9c3 100644 --- a/Snappass.NET/Controllers/ShareController.cs +++ b/Snappass.NET/Controllers/ShareController.cs @@ -38,9 +38,11 @@ public IActionResult Share(string password, string ttl) _ => throw new ArgumentException("Expected week, day or hour"), }; TimeToLive timeToLive = Parse(ttl); - (string encryptedPassword, string key) = Encryption.Encrypt(password); - _memoryStore.Store(encryptedPassword, key, timeToLive); - var model = new GeneratedPassword { Key = key, BaseUri = GetBaseUrl() }; + string storageKey = Guid.NewGuid().ToString("N").ToUpper(); + (string encryptedPassword, string encryptionKey) = Encryption.Encrypt(password); + _memoryStore.Store(encryptedPassword, storageKey, timeToLive); + string token = Encryption.CreateToken(storageKey, encryptionKey); + var model = new GeneratedPassword { Token = token, BaseUri = GetBaseUrl() }; return View("Shared", model); } } diff --git a/Snappass.NET/Encryption.cs b/Snappass.NET/Encryption.cs index 712ea2c..ca599e9 100644 --- a/Snappass.NET/Encryption.cs +++ b/Snappass.NET/Encryption.cs @@ -5,21 +5,42 @@ namespace Snappass { public class Encryption { - public static (string encryptedPassword, string key) Encrypt(string password) + private static readonly char tokenSeparator = '~'; + + public static (string encryptedPassword, string encryptionKey) Encrypt(string password) { - byte[] keyBytes = SimpleFernet.GenerateKey().UrlSafe64Decode(); + byte[] EncryptionKeyBytes = SimpleFernet.GenerateKey().UrlSafe64Decode(); var passwordBytes = System.Text.Encoding.Unicode.GetBytes(password); - string encrypted = SimpleFernet.Encrypt(keyBytes, passwordBytes); - string key = keyBytes.UrlSafe64Encode(); - return (encrypted, key); + string encryptedPassword = SimpleFernet.Encrypt(EncryptionKeyBytes, passwordBytes); + string encryptionKey = EncryptionKeyBytes.UrlSafe64Encode(); + return (encryptedPassword, encryptionKey); } - public static string Decrypt(string encrypted, string key) + public static string Decrypt(string encryptedPassword, string encryptionKey) { - var keyBytes = key.UrlSafe64Decode(); - var decryptedBytes = SimpleFernet.Decrypt(keyBytes, encrypted, out DateTime _); + var encryptionKeyBytes = encryptionKey.UrlSafe64Decode(); + var decryptedBytes = SimpleFernet.Decrypt(encryptionKeyBytes, encryptedPassword, out DateTime _); var decrypted = decryptedBytes.UrlSafe64Encode().FromBase64String().Replace("\0", ""); return decrypted; } + + public static (string storageKey, string decryptionKey) ParseToken(string token) + { + var tokenFragments = token.Split(tokenSeparator, 2); + var storageKey = tokenFragments[0]; + var decryptionKey = string.Empty; + + if (tokenFragments.Length > 1) + decryptionKey = tokenFragments[1]; + + return (storageKey, decryptionKey); + } + public static string CreateToken(string storageKey, string encryptionKey) + { + var token = string.Join(tokenSeparator, storageKey, encryptionKey); + + return token; + + } } } diff --git a/Snappass.NET/Models/GeneratedPassword.cs b/Snappass.NET/Models/GeneratedPassword.cs index c5c5161..cf0916c 100644 --- a/Snappass.NET/Models/GeneratedPassword.cs +++ b/Snappass.NET/Models/GeneratedPassword.cs @@ -2,8 +2,8 @@ { public class GeneratedPassword { - public string Key { get; set; } + public string Token { get; set; } public string BaseUri { get; set; } - public string Uri => $@"{BaseUri}/Password/{Key}"; + public string Uri => $@"{BaseUri}/Password/{Token}"; } } From 20a713777774c9f9f290fbe0de6b0d1502f678db Mon Sep 17 00:00:00 2001 From: Robert Daunce Date: Tue, 15 Jun 2021 17:36:30 -0400 Subject: [PATCH 2/4] Fix preview mechanism The .NET port included the password on a hidden form on the initial page request. A button to reveal the password only unhid the form. This caused the initial page request to expire the password, even if it wasn't viewed. The intention of the preview feature was to prevent bots that prefetch the URL from destroying the secret. This commit removes the password from the preview page and adds a new page where the secret is revealed. Now the secret is only destroyed if the secret is revealed. Ported based on https://github.com/pinterest/snappass/pull/100 --- .../Controllers/PasswordController.cs | 12 ++++++--- Snappass.NET/Views/Password/Password.cshtml | 25 +++++++++++++++++++ Snappass.NET/Views/Password/Preview.cshtml | 16 ++---------- Snappass.NET/wwwroot/js/preview.js | 7 ++++-- 4 files changed, 41 insertions(+), 19 deletions(-) create mode 100644 Snappass.NET/Views/Password/Password.cshtml diff --git a/Snappass.NET/Controllers/PasswordController.cs b/Snappass.NET/Controllers/PasswordController.cs index bf1ce4f..238afa5 100644 --- a/Snappass.NET/Controllers/PasswordController.cs +++ b/Snappass.NET/Controllers/PasswordController.cs @@ -16,6 +16,7 @@ public PasswordController(IMemoryStore memoryStore, ILogger } [HttpGet] + [HttpPost] public IActionResult Preview(string key) { (string storageKey, string encryptionKey) = Encryption.ParseToken(key); @@ -24,9 +25,14 @@ public IActionResult Preview(string key) _logger.LogWarning($@"password with key {storageKey} requested, but not found"); return NotFound(); } - string encryptedPassword = _memoryStore.Retrieve(storageKey); - string decrypted = Encryption.Decrypt(encryptedPassword, encryptionKey); - return View("Preview", new PreviewModel { Key = decrypted }); + if (HttpContext.Request.Method == "POST") + { + string encryptedPassword = _memoryStore.Retrieve(storageKey); + string decrypted = Encryption.Decrypt(encryptedPassword, encryptionKey); + return View("Password", new PreviewModel { Key = decrypted }); + } + + return View("Preview"); } } } diff --git a/Snappass.NET/Views/Password/Password.cshtml b/Snappass.NET/Views/Password/Password.cshtml new file mode 100644 index 0000000..30aa282 --- /dev/null +++ b/Snappass.NET/Views/Password/Password.cshtml @@ -0,0 +1,25 @@ +@{ + ViewData["Title"] = "ViewPassword"; + Layout = "~/Views/_Layout.cshtml"; +} +
+
+

Secret

+

Save the following secret to a secure location.

+
+
+ +
+ +
+ +
+
+

The secret has now been permanently deleted from the system, and the URL will no longer work. Refresh this page to verify.

+
+
+ diff --git a/Snappass.NET/Views/Password/Preview.cshtml b/Snappass.NET/Views/Password/Preview.cshtml index f52ac7d..1b28aea 100644 --- a/Snappass.NET/Views/Password/Preview.cshtml +++ b/Snappass.NET/Views/Password/Preview.cshtml @@ -1,5 +1,5 @@ @{ - ViewData["Title"] = "ViewPassword"; + ViewData["Title"] = "PreviewPassword"; Layout = "~/Views/_Layout.cshtml"; }
@@ -14,18 +14,6 @@
-
-

Save the following secret to a secure location.

-
- - -
-

The secret has now been permanently deleted from the system, and the URL will no longer work. Refresh this page to verify.

-
- + diff --git a/Snappass.NET/wwwroot/js/preview.js b/Snappass.NET/wwwroot/js/preview.js index eaa7999..fafca51 100644 --- a/Snappass.NET/wwwroot/js/preview.js +++ b/Snappass.NET/wwwroot/js/preview.js @@ -1,6 +1,9 @@ (function () { $('#revealSecret').click(function (e) { - $('#revealed').show(); - $('#revealSecret').prop("disabled", true); + var form = $('
') + .attr('id', 'revealSecretForm') + .attr('method', 'post'); + form.appendTo($('body')); + form.submit(); }); })(); \ No newline at end of file From 81872997b33657ba528d3658040b7343990b8f04 Mon Sep 17 00:00:00 2001 From: Robert Daunce Date: Tue, 15 Jun 2021 17:51:13 -0400 Subject: [PATCH 3/4] Quote url to fix equal sign breaking outlook client This was not ported properly to encode the encryption key. See https://github.com/pinterest/snappass/issues/73 --- Snappass.NET/Encryption.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Snappass.NET/Encryption.cs b/Snappass.NET/Encryption.cs index ca599e9..89bd152 100644 --- a/Snappass.NET/Encryption.cs +++ b/Snappass.NET/Encryption.cs @@ -1,4 +1,5 @@ using System; +using System.Net; using Fernet; namespace Snappass @@ -31,13 +32,13 @@ public static (string storageKey, string decryptionKey) ParseToken(string token) var decryptionKey = string.Empty; if (tokenFragments.Length > 1) - decryptionKey = tokenFragments[1]; + decryptionKey = WebUtility.UrlDecode(tokenFragments[1]); return (storageKey, decryptionKey); } public static string CreateToken(string storageKey, string encryptionKey) { - var token = string.Join(tokenSeparator, storageKey, encryptionKey); + var token = string.Join(tokenSeparator, storageKey, WebUtility.UrlEncode(encryptionKey)); return token; From c3ec5ccdd5b4e6261f68a58c5d5d1db649a80a98 Mon Sep 17 00:00:00 2001 From: Robert Daunce Date: Tue, 15 Jun 2021 18:12:49 -0400 Subject: [PATCH 4/4] Added 2 weeks TTL and defaulted TTL to 1 hour --- Snappass.NET/Controllers/ShareController.cs | 3 ++- Snappass.NET/MemoryStore.cs | 12 ++++++++---- Snappass.NET/SqliteStore.cs | 11 +++++++---- Snappass.NET/TimeToLive.cs | 2 +- Snappass.NET/Views/Share/Share.cshtml | 3 ++- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/Snappass.NET/Controllers/ShareController.cs b/Snappass.NET/Controllers/ShareController.cs index 96ed9c3..b1e2540 100644 --- a/Snappass.NET/Controllers/ShareController.cs +++ b/Snappass.NET/Controllers/ShareController.cs @@ -35,7 +35,8 @@ public IActionResult Share(string password, string ttl) "week" => TimeToLive.Week, "day" => TimeToLive.Day, "hour" => TimeToLive.Hour, - _ => throw new ArgumentException("Expected week, day or hour"), + "twoweeks" => TimeToLive.TwoWeeks, + _ => throw new ArgumentException("Expected twoweeks, week, day or hour"), }; TimeToLive timeToLive = Parse(ttl); string storageKey = Guid.NewGuid().ToString("N").ToUpper(); diff --git a/Snappass.NET/MemoryStore.cs b/Snappass.NET/MemoryStore.cs index a8146db..3f0ae8e 100644 --- a/Snappass.NET/MemoryStore.cs +++ b/Snappass.NET/MemoryStore.cs @@ -71,18 +71,22 @@ public string Retrieve(string key) TimeToLive.Day => item.StoredDateTime.AddDays(1), TimeToLive.Week => item.StoredDateTime.AddDays(7), TimeToLive.Hour => item.StoredDateTime.AddHours(1), + TimeToLive.TwoWeeks => item.StoredDateTime.AddDays(14), + _ => item.StoredDateTime.AddHours(1), }; DateTime atTheLatest = GetAtTheLatest(item.TimeToLive); if (_dateTimeProvider.Now > atTheLatest) { static string ToString(TimeToLive ttl) => ttl switch { - TimeToLive.Week => "week", - TimeToLive.Day => "day", - TimeToLive.Hour => "hour", + TimeToLive.Week => "1 week", + TimeToLive.Day => "1 day", + TimeToLive.Hour => "1 hour", + TimeToLive.TwoWeeks => "2 weeks", + _ => "1 hour", }; var ttlString = ToString(item.TimeToLive); - _logger.Log(LogLevel.Warning, $@"Tried to retrieve password for key [{key}] after date is expired. Key set at [{item.StoredDateTime}] for 1 [{ttlString}]"); + _logger.Log(LogLevel.Warning, $@"Tried to retrieve password for key [{key}] after date is expired. Key set at [{item.StoredDateTime}] for [{ttlString}]"); _items.Remove(key); // ensure "read-once" is implemented return null; } diff --git a/Snappass.NET/SqliteStore.cs b/Snappass.NET/SqliteStore.cs index 80096b9..c303722 100644 --- a/Snappass.NET/SqliteStore.cs +++ b/Snappass.NET/SqliteStore.cs @@ -46,6 +46,7 @@ public override TimeToLive Parse(object value) 0 => TimeToLive.Hour, 1 => TimeToLive.Day, 2 => TimeToLive.Week, + 3 => TimeToLive.TwoWeeks, _ => TimeToLive.Hour, }; } @@ -103,6 +104,7 @@ FROM SECRET TimeToLive.Day => dateTime.AddDays(1), TimeToLive.Week => dateTime.AddDays(7), TimeToLive.Hour => dateTime.AddHours(1), + TimeToLive.TwoWeeks => dateTime.AddDays(14), _ => dateTime.AddHours(1) }; DateTime atTheLatest = GetAtTheLatest(secret.TimeToLive, secret.StoredDateTime); @@ -110,13 +112,14 @@ FROM SECRET { static string ToString(TimeToLive ttl) => ttl switch { - TimeToLive.Week => "week", - TimeToLive.Day => "day", - TimeToLive.Hour => "hour", + TimeToLive.Week => "1 week", + TimeToLive.Day => "1 day", + TimeToLive.Hour => "1 hour", + TimeToLive.TwoWeeks => "2 weeks", _ => "hour" }; var ttlString = ToString(secret.TimeToLive); - _logger.Log(LogLevel.Warning, $@"Tried to retrieve password for key [{key}] after date is expired. Key set at [{secret.StoredDateTime}] for 1 [{ttlString}]"); + _logger.Log(LogLevel.Warning, $@"Tried to retrieve password for key [{key}] after date is expired. Key set at [{secret.StoredDateTime}] for [{ttlString}]"); Remove(key); return null; } diff --git a/Snappass.NET/TimeToLive.cs b/Snappass.NET/TimeToLive.cs index e5fe43b..82a223e 100644 --- a/Snappass.NET/TimeToLive.cs +++ b/Snappass.NET/TimeToLive.cs @@ -2,6 +2,6 @@ { public enum TimeToLive { - Week, Day, Hour + Week, Day, Hour, TwoWeeks } } diff --git a/Snappass.NET/Views/Share/Share.cshtml b/Snappass.NET/Views/Share/Share.cshtml index 660073c..f274b78 100644 --- a/Snappass.NET/Views/Share/Share.cshtml +++ b/Snappass.NET/Views/Share/Share.cshtml @@ -16,9 +16,10 @@