Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change webaccess auth file format to accommodate different hashing algorithms #992

Merged
merged 2 commits into from
Aug 18, 2017

Conversation

enbyted
Copy link
Contributor

@enbyted enbyted commented Aug 16, 2017

This pull request fixes issues raised in previous PR (#980) discussion:

  1. Qt4 doesn't support SHA256 thus is has to use SHA1 or MD5
  2. SHA1 and MD5 both have been broken, so Qt5 build uses SHA256
  3. This prevents passwords file from being shared between QLC+ instances with different Qt versions
  4. Current implementation doesn't support salting

Changes:

To fix that I've changed passwords file to use a different format (backwards compatible).
Each line is now in format username:passwordHash:userlevel:hashAlgorithm:salt, where only username and passwordHash is required.

Each field explained:

  • username: the login name
  • passwordHash: hash of string password + salt using hashAlgorithm
  • userLevel: integer expressing user access level as defined in WebAccessUserLevel enum in webaccessauth.h. Default: super admin (100)
  • hashAlgorithm: one of md5, sha1, sha256 (only sha1 is guaranteed to be available). Default: sha1 (Qt4 and Qt5 with QT_CRYPTOGRAPHICHASH_ONLY_SHA1 defined) or sha256 (Qt5). If unknown hash algorithm is used sha1 is attempted
  • salt: random string to prevent dictionary attacks

There is new #define in webaccessauth.cpp - SALT_LENGTH: when creating a new user entry HEX string of given length is generated as salt

Upon saving (i.e. after adding a new user) QLC+ saves row with all five fields (possibly empty salt)

Possible enhancements:

  • There is no way of changing the default hashing algorithm thus there is no way to make QLC+ add new users with selected hash type. This could be a command line option, however I'm not sure if this would be really useful to anyone and I don't want to alter main program's code unless really necessary (webaccess is a library after all).
  • Make generating salt more efficient - currently it's probably 6 allocations of memory for QString (depending on implementation). I figured that this would be so rarely used that readability is more important than efficiency.

Additional:

Not tested under Qt4
Tested only under Windows

Example passwords file:

test:233307f39dbd5f45c4700d5d56202e120e6e0c429c2a08a36fd54a94b33cc8bd:100:sha256:876782d575d1b6cb09404c994f98785e
test1:448ed7416fce2cb66c285d182b1ba3df1e90016d:100:sha1:12345
test256:ecd71870d1963316a97e3ac3408c9835ad8cf0f3c1bc703527c30265534f75ae:10:sha256:
test5:ce6a67d0b5d3fababb07e43d61105717:20:md5:54321

Users (username/password):

  • test/test12345 - super admin (SHA256, generated through web access interface)
  • test1/test - super admin (SHA1)
  • test256/test123 - vc only (SHA256 without salt)
  • test5/test - vc + simple desk (MD5)

Enbyted added 2 commits August 16, 2017 12:04
Introduced salting and different hashing algorithms.
This change should be 100% backwards compatibile.

**Not tested yet - probably contains bugs**
@mcallegari
Copy link
Owner

mcallegari commented Aug 17, 2017

@enbyted thanks a lot for this 👍
First consideration: I wouldn't worry too much about switching from Qt4 to Qt5.
Right now all the QLC+ builds are on Qt5 except for some Linux cases. I do guess though that most of the Linux users download Qt5-based builds from OBS, as most of the Linux distros now bundle Qt5 by default.
In any case the goal is to phase out Qt4 as soon as possible (e.g. QLC+ 5 will require Qt 5.9)
If really the Qt4->Qt5 switch happens, I would say we can require to delete and re-create the password file.

Can this information change/simplify anything in this PR ?

@enbyted
Copy link
Contributor Author

enbyted commented Aug 17, 2017

@mcallegari
Actually, switch from Qt4 to Qt5 will be working out of the box. The other way around will not, unless there is a way to force QLC+ to use SHA1 (edge case in my opinion).

Either way, the way it is implemented currently is the simplest that I can think of that will still build on Qt4 and choose the most secure hash by default and be somewhat compatible with different versions of Qt. (you can always generate the file yourself it's easy)

EDIT:

I've just deployed this change to 5 devices in production, we'll see if anything breaks.

Update: Nothing broken today, seems stable.

@mcallegari
Copy link
Owner

Alright, I haven't tried the code myself yet, but I reviewed it and it looks OK. I'll test it in the next few days.
Thanks again ! 👍

@mcallegari mcallegari merged commit 8150c20 into mcallegari:master Aug 18, 2017
@enbyted
Copy link
Contributor Author

enbyted commented Aug 18, 2017

No problem, if anything goes wrong give me a @ ping here and I'll fix it ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants