Better default authentication #870

LogicalPhallacy · 2019-02-12T10:25:41Z

This PR changes the default auth method in a few ways to address some security problems that jellyfin has. I need more people on more OS' to test this before it should be merged, but it works on Windows.

Changes
Changed Emby.Server.Implementations to target netcore 2.1 instead of netstandard, this was necessary for the better crypto library
Replaced the dumb not really functional username validation code with a piece of regex that should accomplish the original goal
Added a converter to make it so existing dbs load in cleanly with the changes to the password string.

PLEASE NOTE THAT WHEN YOU RUN A BUILD WITH THIS PR YOU CANNOT GO BACK WITHOUT A BACKUP OF YOUR USERS.DB

Enables SHA256 with Salt authentication. In theory this also enables any other hashing algo you might want to use from the crypto provider in case SHA1 and MD5 aren't cutting it (we might want to start moving away from these as they both have been proven to have collisions)

Issues
Fixes #302 Fixes #545 Addresses #187 because at least they get a salt now.

Yanking in latest changes

.gitignore

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

EraYaN · 2019-02-12T13:48:52Z

SHA256 is not any better for passwords than SHA1 and MD5, please use bcrypt or Pbkdf2.
My GPU still runs it very very fast.

In the end we should end up here for example: (or using the actual Identity classes)
https://docs.microsoft.com/en-us/aspnet/core/security/data-protection/consumer-apis/password-hashing?view=aspnetcore-2.2

But (alternatively) this is in netstandard2.0:
https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rfc2898derivebytes?view=netframework-4.7.2

Really, simple hashes were never made for password auth.

Good resource: https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet

Bond-009

Use braces

Emby.Server.Implementations/Emby.Server.Implementations.csproj

LogicalPhallacy · 2019-02-13T07:06:28Z

SHA256 is not any better for passwords than SHA1 and MD5, please use bcrypt or Pbkdf2.
My GPU still runs it very very fast.
In the end we should end up here for example: (or using the actual Identity classes)
https://docs.microsoft.com/en-us/aspnet/core/security/data-protection/consumer-apis/password-hashing?view=aspnetcore-2.2
But (alternatively) this is in netstandard2.0:
https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rfc2898derivebytes?view=netframework-4.7.2
Really, simple hashes were never made for password auth.
Good resource: https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet

So fun fact. I am thick as a brick this week and missed it. You can see PBKDF2 is already implemented in the crypto provider, it just uses SHA256 as the hash algo that it runs over and over. If you give a salt, it uses PBKDF2 via Rfc2898DeriveBytes

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

…der.cs fix to styling Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Bond-009

Style issues.
Newline after a closing brace.

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Data/SqliteUserRepository.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

JustAMan

Nitpicking mostly, overall code is good. Should stamp approval when my comments are addressed (fixed or explained me wrong).

Emby.Server.Implementations/Data/SqliteUserRepository.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Adding minor stylistic suggestions from Bond-009 Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

LogicalPhallacy · 2019-02-18T09:30:05Z

Aside from whether or not netstandard vs netcore is appropriate for the models project, all feedback has been addressed by the lastest set of pushes. I have making new users, changing blank user passwords, authenticating existing users, changing existing user passwords, and authenticating blank password users all working on native windows. I see no reason why it shouldn't on *nix, but again, testing would be wise.

JustAMan

No major blockings, so after all my comments are addressed I could approve it.

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Data/SqliteUserRepository.cs

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Data/SqliteUserRepository.cs

Emby.Server.Implementations/Emby.Server.Implementations.csproj

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

Emby.Server.Implementations/Library/UserManager.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

hawken93

My review is all about whitespace. I think it's important because there are a lot of lines changed here that are changed for no reason and it introduces inconsistency. There are more files that my comments apply to but I'm on mobile and wasn't sure I would catch it all anyway. Otherwise, thank you for this, it's a really needed change

.gitignore

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

hawken93

Thanks, much better now

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

JustAMan

LGTM.

@Bond-009 can you pls provide new feedback?

Emby.Server.Implementations/Library/UserManager.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

Bond-009

Looks good, except for all the style issues.

Empty lines around properties
Empty line after closing brace
private fields are prefixed with _ and are camelBack
Space after the start of a comment //
variables are camelBack
use string not String
space between if and opening bracket

LogicalPhallacy · 2019-03-05T07:59:09Z

If there are still style issues there I don't see them, then again I don't see them much in the first place...

Emby.Server.Implementations/Library/UserManager.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

Bond-009

Mostly style issues.
https://github.com/jellyfin/jellyfin/pull/870/files#diff-77a2b0931f9792dc15da0970d5453232R103
This still bothers me ;)

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

MediaBrowser.Model/Cryptography/PasswordHash.cs

more minor fixes before I do larger fixes Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

JustAMan

LGTM

MediaBrowser.Model/Cryptography/ICryptoProvider.cs

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs

…der.cs Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

cvium · 2019-04-20T15:36:37Z

Emby.Server.Implementations/Data/SqliteUserRepository.cs

+            {
+                // If the user password is the sha1 hash of the empty string, remove it
+                if (!string.Equals(user.Password, "DA39A3EE5E6B4B0D3255BFEF95601890AFD80709", StringComparison.Ordinal)
+                    || !string.Equals(user.Password, "$SHA1$DA39A3EE5E6B4B0D3255BFEF95601890AFD80709", StringComparison.Ordinal))


This should've been &&

LogicalPhallacy and others added 4 commits January 30, 2019 23:35

added submodule dir to gitignore

49d9649

Upgrade crypto provider, retarget better framework

4519ce2

Merge pull request #9 from jellyfin/master

8bf88f4

Yanking in latest changes

sha256 with salt auth and sha1 interop

05bbf71

JustAMan suggested changes Feb 12, 2019

View reviewed changes

EraYaN added backend Related to the server backend feature Adding a new feature, or substantial improvements on existing functionality labels Feb 12, 2019

Bond-009 requested changes Feb 12, 2019

View reviewed changes

Bond-009 reviewed Feb 12, 2019

View reviewed changes

Emby.Server.Implementations/Emby.Server.Implementations.csproj Outdated Show resolved Hide resolved

cvium mentioned this pull request Feb 12, 2019

Do not hash empty strings #876

Closed

LogicalPhallacy added 2 commits February 12, 2019 22:24

fixed gitignore fail

1dc5a62

fixed nul user check to be first per justaman

1ffd443

cvium reviewed Feb 13, 2019

View reviewed changes

LogicalPhallacy and others added 3 commits February 13, 2019 00:33

Minor fixes re:PR870, added null checks from PR876

77602af

Update Emby.Server.Implementations/Library/DefaultAuthenticationProvi…

9e58e31

…der.cs fix to styling Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

Update Emby.Server.Implementations/Library/DefaultAuthenticationProvi…

d8e6808

…der.cs fix to styling Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

Bond-009 reviewed Feb 13, 2019

View reviewed changes

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs Outdated Show resolved Hide resolved

Bond-009 requested changes Feb 13, 2019

View reviewed changes

JustAMan reviewed Feb 13, 2019

View reviewed changes

LogicalPhallacy and others added 2 commits February 18, 2019 00:31

Apply suggestions from code review

9f3aa2c

Adding minor stylistic suggestions from Bond-009 Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

added justaman notes, fixed new bug from emty has removals

48e7274

JustAMan suggested changes Feb 18, 2019

View reviewed changes

little fixes for JustAMan

56e3063

Bond-009 requested changes Feb 19, 2019

View reviewed changes

minor changes and return to netstandard

6bbb968

hawken93 suggested changes Feb 20, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs Outdated Show resolved Hide resolved

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs Outdated Show resolved Hide resolved

merging with master to clear merge conflict

a0d31a4

made newlines into linux newlines

098de6b

hawken93 approved these changes Feb 20, 2019

View reviewed changes

JustAMan suggested changes Feb 20, 2019

View reviewed changes

Emby.Server.Implementations/Cryptography/CryptographyProvider.cs Show resolved Hide resolved

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs Outdated Show resolved Hide resolved

fixed logic flip in auth empty check and fixed crypto algo choice

edba82d

JustAMan approved these changes Feb 28, 2019

View reviewed changes

hawken93 reviewed Feb 28, 2019

View reviewed changes

Emby.Server.Implementations/Library/UserManager.cs Outdated Show resolved Hide resolved

Bond-009 reviewed Feb 28, 2019

View reviewed changes

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs Outdated Show resolved Hide resolved

Bond-009 requested changes Feb 28, 2019

View reviewed changes

minor style fixes

2c26517

MalteT mentioned this pull request Mar 5, 2019

[feature] Option to configure disabling of users after failed login attempts #1045

Closed

Bond-009 requested changes Mar 5, 2019

View reviewed changes

Emby.Server.Implementations/Library/UserManager.cs Outdated Show resolved Hide resolved

MediaBrowser.Model/Cryptography/PasswordHash.cs Outdated Show resolved Hide resolved

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs Outdated Show resolved Hide resolved

Bond-009 reviewed Mar 5, 2019

View reviewed changes

MediaBrowser.Model/Cryptography/PasswordHash.cs Outdated Show resolved Hide resolved

Minor fixes to address style issues

bef665b

Bond-009 requested changes Mar 7, 2019

View reviewed changes

Bond-009 and others added 3 commits March 7, 2019 02:41

Apply suggestions from code review

c31b0b3

more minor fixes before I do larger fixes Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

more fixes for perf and style

8f4895e

made hashset static and readonly

dfb1d70

JustAMan approved these changes Mar 7, 2019

View reviewed changes

MediaBrowser.Model/Cryptography/ICryptoProvider.cs Outdated Show resolved Hide resolved

Bond-009 reviewed Mar 7, 2019

View reviewed changes

Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs Outdated Show resolved Hide resolved

Update Emby.Server.Implementations/Library/DefaultAuthenticationProvi…

f486f59

…der.cs Co-Authored-By: LogicalPhallacy <44458166+LogicalPhallacy@users.noreply.github.com>

Bond-009 approved these changes Mar 7, 2019

View reviewed changes

Bond-009 merged commit ae0ecc1 into jellyfin:master Mar 7, 2019

WWWesten mentioned this pull request Apr 12, 2019

Users of the app can't authorize on JF server jellyfin/jellyfin-androidtv#87

Closed

cvium reviewed Apr 20, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better default authentication #870

Better default authentication #870

Better default authentication #870

Better default authentication #870

Conversation

PLEASE NOTE THAT WHEN YOU RUN A BUILD WITH THIS PR YOU CANNOT GO BACK WITHOUT A BACKUP OF YOUR USERS.DB

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment