From d77507ba09a010dc653814ea620aafa42348a373 Mon Sep 17 00:00:00 2001 From: Bond_009 Date: Wed, 24 Mar 2021 18:09:40 +0100 Subject: [PATCH 1/3] Rewrite PasswordHash.Parse to work with ReadOnlySpans --- .../Cryptography/PasswordHash.cs | 121 +++++++++++---- .../Cryptography/PasswordHashTests.cs | 139 ++++++++++++++++++ .../PasswordHashTests.cs | 31 ---- 3 files changed, 233 insertions(+), 58 deletions(-) create mode 100644 tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs delete mode 100644 tests/Jellyfin.Common.Tests/PasswordHashTests.cs diff --git a/MediaBrowser.Common/Cryptography/PasswordHash.cs b/MediaBrowser.Common/Cryptography/PasswordHash.cs index 3e2eae1c8e..81e980bf2f 100644 --- a/MediaBrowser.Common/Cryptography/PasswordHash.cs +++ b/MediaBrowser.Common/Cryptography/PasswordHash.cs @@ -1,4 +1,5 @@ #pragma warning disable CS1591 +#nullable enable using System; using System.Collections.Generic; @@ -30,6 +31,16 @@ namespace MediaBrowser.Common.Cryptography public PasswordHash(string id, byte[] hash, byte[] salt, Dictionary parameters) { + if (id == null) + { + throw new ArgumentNullException(nameof(id)); + } + + if (id.Length == 0) + { + throw new ArgumentException("String can't be empty", nameof(id)); + } + Id = id; _hash = hash; _salt = salt; @@ -59,58 +70,109 @@ namespace MediaBrowser.Common.Cryptography /// Return the hashed password. public ReadOnlySpan Hash => _hash; - public static PasswordHash Parse(string hashString) + public static PasswordHash Parse(ReadOnlySpan hashString) { - // The string should at least contain the hash function and the hash itself - string[] splitted = hashString.Split('$'); - if (splitted.Length < 3) + if (hashString.IsEmpty) + { + throw new ArgumentException("String can't be empty", nameof(hashString)); + } + + if (hashString[0] != '$') { - throw new ArgumentException("String doesn't contain enough segments", nameof(hashString)); + throw new FormatException("Hash string must start with a $"); } - // Start at 1, the first index shouldn't contain any data - int index = 1; + // Ignore first $ + hashString = hashString[1..]; - // Name of the hash function - string id = splitted[index++]; + int nextSegment = hashString.IndexOf('$'); + if (hashString.IsEmpty || nextSegment == 0) + { + throw new FormatException("Hash string must contain a valid id"); + } + else if (nextSegment == -1) + { + return new PasswordHash(hashString.ToString(), Array.Empty()); + } + + ReadOnlySpan id = hashString[..nextSegment]; + hashString = hashString[(nextSegment + 1)..]; + Dictionary? parameters = null; + + nextSegment = hashString.IndexOf('$'); // Optional parameters - Dictionary parameters = new Dictionary(); - if (splitted[index].IndexOf('=', StringComparison.Ordinal) != -1) + ReadOnlySpan parametersSpan = nextSegment == -1 ? hashString : hashString[..nextSegment]; + if (parametersSpan.Contains('=')) { - foreach (string paramset in splitted[index++].Split(',')) + while (!parametersSpan.IsEmpty) { - if (string.IsNullOrEmpty(paramset)) + ReadOnlySpan parameter; + int index = parametersSpan.IndexOf(','); + if (index == -1) + { + parameter = parametersSpan; + parametersSpan = ReadOnlySpan.Empty; + } + else { - continue; + parameter = parametersSpan[..index]; + parametersSpan = parametersSpan[(index + 1)..]; } - string[] fields = paramset.Split('='); - if (fields.Length != 2) + int splitIndex = parameter.IndexOf('='); + if (splitIndex == -1 || splitIndex == 0 || splitIndex == parameter.Length - 1) { - throw new InvalidDataException($"Malformed parameter in password hash string {paramset}"); + throw new FormatException($"Malformed parameter in password hash string"); } - parameters.Add(fields[0], fields[1]); + (parameters ??= new Dictionary()).Add( + parameter[..splitIndex].ToString(), + parameter[(splitIndex + 1)..].ToString()); + } + + if (nextSegment == -1) + { + // parameters can't be null here + return new PasswordHash(id.ToString(), Array.Empty(), Array.Empty(), parameters!); } + + hashString = hashString[(nextSegment + 1)..]; + nextSegment = hashString.IndexOf('$'); + } + + if (nextSegment == 0) + { + throw new FormatException($"Hash string contains an empty segment"); } byte[] hash; byte[] salt; - // Check if the string also contains a salt - if (splitted.Length - index == 2) + if (nextSegment == -1) { - salt = Convert.FromHexString(splitted[index++]); - hash = Convert.FromHexString(splitted[index++]); + salt = Array.Empty(); + hash = Convert.FromHexString(hashString); } else { - salt = Array.Empty(); - hash = Convert.FromHexString(splitted[index++]); + salt = Convert.FromHexString(hashString[..nextSegment]); + hashString = hashString[(nextSegment + 1)..]; + nextSegment = hashString.IndexOf('$'); + if (nextSegment != -1) + { + throw new FormatException("Hash string contains too many segments"); + } + + if (hashString.IsEmpty) + { + throw new FormatException("Hash segment is empty"); + } + + hash = Convert.FromHexString(hashString); } - return new PasswordHash(id, hash, salt, parameters); + return new PasswordHash(id.ToString(), hash, salt, parameters ?? new Dictionary()); } private void SerializeParameters(StringBuilder stringBuilder) @@ -147,8 +209,13 @@ namespace MediaBrowser.Common.Cryptography .Append(Convert.ToHexString(_salt)); } - return str.Append('$') - .Append(Convert.ToHexString(_hash)).ToString(); + if (_hash.Length != 0) + { + str.Append('$') + .Append(Convert.ToHexString(_hash)); + } + + return str.ToString(); } } } diff --git a/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs b/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs new file mode 100644 index 0000000000..6da0a3037e --- /dev/null +++ b/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs @@ -0,0 +1,139 @@ +using System; +using System.Collections.Generic; +using MediaBrowser.Common.Cryptography; +using Xunit; + +namespace Jellyfin.Common.Tests.Cryptography +{ + public static class PasswordHashTests + { + [Fact] + public static void Ctor_Null_ThrowsArgumentNullException() + { + Assert.Throws(() => new PasswordHash(null!, Array.Empty())); + } + + [Fact] + public static void Ctor_Empty_ThrowsArgumentException() + { + Assert.Throws(() => new PasswordHash(string.Empty, Array.Empty())); + } + + public static IEnumerable Parse_Valid_TestData() + { + yield return new object[] + { + "$PBKDF2", + new PasswordHash("PBKDF2", Array.Empty()) + }; + + yield return new object[] + { + "$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + new PasswordHash( + "PBKDF2", + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), + new Dictionary() + { + { "iterations", "1000" } + }) + }; + + yield return new object[] + { + "$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + new PasswordHash( + "PBKDF2", + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), + new Dictionary() + { + { "iterations", "1000" }, + { "m", "120" } + }) + }; + + yield return new object[] + { + "$PBKDF2$iterations=1000,m=120$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + new PasswordHash( + "PBKDF2", + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Convert.FromHexString("69F420"), + new Dictionary() + { + { "iterations", "1000" }, + { "m", "120" } + }) + }; + + yield return new object[] + { + "$PBKDF2$iterations=1000,m=120", + new PasswordHash( + "PBKDF2", + Array.Empty(), + Array.Empty(), + new Dictionary() + { + { "iterations", "1000" }, + { "m", "120" } + }) + }; + } + + [Theory] + [MemberData(nameof(Parse_Valid_TestData))] + public static void Parse_Valid_Success(string passwordHashString, PasswordHash expected) + { + var passwordHash = PasswordHash.Parse(passwordHashString); + Assert.Equal(expected.Id, passwordHash.Id); + Assert.Equal(expected.Parameters, passwordHash.Parameters); + Assert.Equal(expected.Salt.ToArray(), passwordHash.Salt.ToArray()); + Assert.Equal(expected.Hash.ToArray(), passwordHash.Hash.ToArray()); + Assert.Equal(expected.ToString(), passwordHash.ToString()); + } + + [Theory] + [InlineData("$PBKDF2")] + [InlineData("$PBKDF2$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] + [InlineData("$PBKDF2$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] + [InlineData("$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] + [InlineData("$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] + [InlineData("$PBKDF2$iterations=1000,m=120")] + public static void ToString_Roundtrip_Success(string passwordHash) + { + Assert.Equal(passwordHash, PasswordHash.Parse(passwordHash).ToString()); + } + + [Fact] + public static void Parse_Null_ThrowsArgumentException() + { + Assert.Throws(() => PasswordHash.Parse(null)); + } + + [Fact] + public static void Parse_Empty_ThrowsArgumentException() + { + Assert.Throws(() => PasswordHash.Parse(string.Empty)); + } + + [Theory] + [InlineData("$")] // No id + [InlineData("$$")] // Empty segments + [InlineData("PBKDF2$")] // Doesn't start with $ + [InlineData("$PBKDF2$$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Empty segment + [InlineData("$PBKDF2$iterations=1000$$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Empty salt segment + [InlineData("$PBKDF2$=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter + [InlineData("$PBKDF2$=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter + [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter + [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$")] // Ends on $ + [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$anotherone")] // Extra segment + [InlineData("$PBKDF2$69F420$")] // Empty hash + public static void Parse_InvalidFormat_ThrowsFormatException(string passwordHash) + { + Assert.Throws(() => PasswordHash.Parse(passwordHash)); + } + } +} diff --git a/tests/Jellyfin.Common.Tests/PasswordHashTests.cs b/tests/Jellyfin.Common.Tests/PasswordHashTests.cs deleted file mode 100644 index c4422bd105..0000000000 --- a/tests/Jellyfin.Common.Tests/PasswordHashTests.cs +++ /dev/null @@ -1,31 +0,0 @@ -using System; -using MediaBrowser.Common; -using MediaBrowser.Common.Cryptography; -using Xunit; - -namespace Jellyfin.Common.Tests -{ - public class PasswordHashTests - { - [Theory] - [InlineData( - "$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", - "PBKDF2", - "", - "62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] - public void ParseTest(string passwordHash, string id, string salt, string hash) - { - var pass = PasswordHash.Parse(passwordHash); - Assert.Equal(id, pass.Id); - Assert.Equal(salt, Convert.ToHexString(pass.Salt)); - Assert.Equal(hash, Convert.ToHexString(pass.Hash)); - } - - [Theory] - [InlineData("$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] - public void ToStringTest(string passwordHash) - { - Assert.Equal(passwordHash, PasswordHash.Parse(passwordHash).ToString()); - } - } -} From c2cd7fa0b2441212ac0de16ffcef18883ee8c665 Mon Sep 17 00:00:00 2001 From: Bond_009 Date: Wed, 24 Mar 2021 18:39:33 +0100 Subject: [PATCH 2/3] Add more PasswordHash tests --- .../Cryptography/PasswordHashTests.cs | 66 ++++++++++++++++--- 1 file changed, 56 insertions(+), 10 deletions(-) diff --git a/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs b/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs index 6da0a3037e..e6c325bac0 100644 --- a/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs +++ b/tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs @@ -21,31 +21,34 @@ namespace Jellyfin.Common.Tests.Cryptography public static IEnumerable Parse_Valid_TestData() { + // Id yield return new object[] { "$PBKDF2", new PasswordHash("PBKDF2", Array.Empty()) }; + // Id + parameter yield return new object[] { - "$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + "$PBKDF2$iterations=1000", new PasswordHash( "PBKDF2", - Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), Array.Empty(), new Dictionary() { - { "iterations", "1000" } + { "iterations", "1000" }, }) }; + // Id + parameters yield return new object[] { - "$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + "$PBKDF2$iterations=1000,m=120", new PasswordHash( "PBKDF2", - Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), Array.Empty(), new Dictionary() { @@ -54,13 +57,50 @@ namespace Jellyfin.Common.Tests.Cryptography }) }; + // Id + hash yield return new object[] { - "$PBKDF2$iterations=1000,m=120$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + "$PBKDF2$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + new PasswordHash( + "PBKDF2", + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), + new Dictionary()) + }; + + // Id + salt + hash + yield return new object[] + { + "$PBKDF2$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", new PasswordHash( "PBKDF2", Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), Convert.FromHexString("69F420"), + new Dictionary()) + }; + + // Id + parameter + hash + yield return new object[] + { + "$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + new PasswordHash( + "PBKDF2", + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), + new Dictionary() + { + { "iterations", "1000" } + }) + }; + + // Id + parameters + hash + yield return new object[] + { + "$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", + new PasswordHash( + "PBKDF2", + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Array.Empty(), new Dictionary() { { "iterations", "1000" }, @@ -68,13 +108,14 @@ namespace Jellyfin.Common.Tests.Cryptography }) }; + // Id + parameters + salt + hash yield return new object[] { - "$PBKDF2$iterations=1000,m=120", + "$PBKDF2$iterations=1000,m=120$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D", new PasswordHash( "PBKDF2", - Array.Empty(), - Array.Empty(), + Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"), + Convert.FromHexString("69F420"), new Dictionary() { { "iterations", "1000" }, @@ -101,6 +142,7 @@ namespace Jellyfin.Common.Tests.Cryptography [InlineData("$PBKDF2$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] [InlineData("$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] [InlineData("$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] + [InlineData("$PBKDF2$iterations=1000,m=120$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] [InlineData("$PBKDF2$iterations=1000,m=120")] public static void ToString_Roundtrip_Success(string passwordHash) { @@ -125,11 +167,15 @@ namespace Jellyfin.Common.Tests.Cryptography [InlineData("PBKDF2$")] // Doesn't start with $ [InlineData("$PBKDF2$$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Empty segment [InlineData("$PBKDF2$iterations=1000$$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Empty salt segment + [InlineData("$PBKDF2$iterations=1000$69F420$")] // Empty hash segment [InlineData("$PBKDF2$=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter [InlineData("$PBKDF2$=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$")] // Ends on $ - [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$anotherone")] // Extra segment + [InlineData("$PBKDF2$iterations=$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$")] // Extra segment + [InlineData("$PBKDF2$iterations=$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$anotherone")] // Extra segment + [InlineData("$PBKDF2$iterations=$invalidstalt$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid salt + [InlineData("$PBKDF2$iterations=$69F420$invalid hash")] // Invalid hash [InlineData("$PBKDF2$69F420$")] // Empty hash public static void Parse_InvalidFormat_ThrowsFormatException(string passwordHash) { From 4cea6d9ccfa5bddaef27800aac1c125ae22747d7 Mon Sep 17 00:00:00 2001 From: Bond-009 Date: Thu, 25 Mar 2021 18:53:36 +0100 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Cody Robibero --- MediaBrowser.Common/Cryptography/PasswordHash.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MediaBrowser.Common/Cryptography/PasswordHash.cs b/MediaBrowser.Common/Cryptography/PasswordHash.cs index 81e980bf2f..f2ecc4741b 100644 --- a/MediaBrowser.Common/Cryptography/PasswordHash.cs +++ b/MediaBrowser.Common/Cryptography/PasswordHash.cs @@ -123,7 +123,7 @@ namespace MediaBrowser.Common.Cryptography int splitIndex = parameter.IndexOf('='); if (splitIndex == -1 || splitIndex == 0 || splitIndex == parameter.Length - 1) { - throw new FormatException($"Malformed parameter in password hash string"); + throw new FormatException("Malformed parameter in password hash string"); } (parameters ??= new Dictionary()).Add( @@ -143,7 +143,7 @@ namespace MediaBrowser.Common.Cryptography if (nextSegment == 0) { - throw new FormatException($"Hash string contains an empty segment"); + throw new FormatException("Hash string contains an empty segment"); } byte[] hash;