Critical bug in builtin authentication handler

User avatar
sorcerykid
Member
 
Posts: 219
Joined: Fri Aug 26, 2016 15:36
In-game: Nemo

Critical bug in builtin authentication handler

by sorcerykid » Sun Feb 12, 2017 23:29

This past December, right after migrating JT2 to a dedicated hosting provider, I noticed serious lag spikes every time that a player would login. I spent hours trying to localize the problem, until I noticed that it stemmed from excessive I/O operations to auth.txt. As you can see, this file is literally 44 megabytes in size and encompasses over 110,000 lines of text.

Your phone or window isn't wide enough to display the code box. If it's a phone, try rotating it to landscape mode.
Code: Select all
% ls auth.txt
-rw-rw-r-- 1 minetest minetest 44712993 Feb 10 15:53 auth.txt

% wc -l auth.txt
110022 auth.txt


Needless to say, a flat file database of this size is going to be horribly inefficient. But even more concerning is the decision to use the SHA1 algorithm for user passwords, which is overkill for any computer game. A single hash including salt consists of 369 BYTES of data. Here is the record for my account "publicworks":

Your phone or window isn't wide enough to display the code box. If it's a phone, try rotating it to landscape mode.
Code: Select all
% grep publicworks auth.txt
publicworks:#1#+OriAAAAAAAAAAAAAAAAAA#fv92vwRTpxQYffOQdaTdl6CYzWnrCTTRJV3RQWvUpYHBSXwnwJAGmjRFMUsozLRwUvKONgwDyCH5S9Fs/VWhtzzG30uNVfNORaduM9u1Oo9IRLSMe9WH1FlxIXNh+NTluWeSE93mTmrg+POIVsYvfZBBVKeZoFtVJrmIDqnJxd8s8LOoc8FU0eg1HFTx0NUtgVBasPY5AJsIDqkzNkjsva9QFMfSy9YmNTYasbNuotDAG7DmY2iOQpsDZUAR5HdZGvcxDr+d+uKpVPNod3BxbBHTvpZlACXTMbHUt97Nrs589lKU9W2X6TOq48zIK9GjAoT7SuAIiskg/pjV6NCX3g:water,interact,shout,lava,basic_privs:1482206476


Now, imagine storing passwords for hundreds of thousands of accounts just like this one. Since most users probably only have passwords consisting of 5-8 characters, the resulting hash is 40 TIMES longer than the original plain-text string. That is unjustifiably excessive.

Of course, the file format is only partially to blame. It is the builtin authentication handler that is the real problem. Check this out: Whenever a new user joins a server, create_auth( ) is automatically invoked:

Your phone or window isn't wide enough to display the code box. If it's a phone, try rotating it to landscape mode.
Code: Select all
        create_auth = function(name, password)
                minetest.log( "action", "Adding password entry for player " .. name )
                assert(type(name) == "string")
                assert(type(password) == "string")
                core.log('info', "Built-in authentication handler adding player '"..name.."'")
                core.auth_table[name] = {
                        password = password,
                        privileges = core.string_to_privs(core.setting_get("default_privs")),
                        last_login = os.time(),
                }
                save_auth_file()
        end,


Immediately afterward, record_login( ) is invoked to update the login timestamp, which had just been initialized by the previous function:

Your phone or window isn't wide enough to display the code box. If it's a phone, try rotating it to landscape mode.
Code: Select all
        record_login = function(name)
                assert(type(name) == "string")
                assert(core.auth_table[name]).last_login = os.time()
                save_auth_file()
        end,


Notice the senseless redundancy. Together these functions end up rewriting the exact same 110,000 lines of text, TWO TIMES OVER in the case of new users, causing the server thread to hang almost indefinitely, particularly if there is a growing queue of new players logging in. I think it's concerning how such poor quality code ever made it past inspection, and even moreso why it persisted for well over a year through multiple version changes.

Numerous high-traffic servers including Extreme Survival Minetest Server, Just Test II, Cash's World, and undoubtedly countless others have suffered due to this issue being unresolved. Most server operators probably just rack it up to an operating system deficiency, a faulty hard drive, lack of memory, or some other external cause.

Clearly something has to change with the builtin authentication handler, because this is an absolutely unacceptable implementation for any type of production environment. I've already got an interim fix in place on my own server. I disabled all writing and reading of the auth.txt file until startup and shutdown via a new auth.commit( ) function or upon operator request using an "auth_commit" chat command. After deploying this fix, the lag dropped from upwards of 10 seconds to a mere 0.1 seconds. That's a 100x performance improvement.

All of the patched files are included as an attachment, with a readme describing the issue and a resolution.

I really hope the core developer team will re-examine their existing quality control measures, because critical bugs like this (and even this) should have never made their way into a "stable" build.
Attachments
minetest0414-auth-patch.zip
(21.29 KiB) Downloaded 207 times
 

843jdc
Member
 
Posts: 207
Joined: Tue Jan 19, 2016 16:46
GitHub: jdc843
IRC: jdc843
In-game: 843jdc

Re: Critical bug in builtin authentication handler

by 843jdc » Mon Feb 13, 2017 00:30

I had extreme lag when I had my server running in a virtual machine, using Windows 10 as a host, with the server stored on a SATA2 hard disk drive.
The lag slightly lessened when I ran the server under Fedora but still on the same drive.

I have many areas to examine to find where the problem is: Host OS, the virtual machine, the Redis server, etc. I was guessing that the problem was because it appears that the server does not handle player logins as a separate thread, fully bringing in the new player once log in is complete and media files have been sent to the player. We can't have players being killed as they log in and wait for media files huh? haha

After a discussion with sorcerykid about extreme server lag on my server whenever a player logs in, I believe she is correct. The lag pretty much vanished when I moved the server to my solid state drive and still under the Fedora host. The server's auth.txt file is now on the SSD.

I hope that this problem is fixed because I need to return the server to running under Windows 10 and the SATA2 drive soon.
 

User avatar
rubenwardy
Member
 
Posts: 4500
Joined: Tue Jun 12, 2012 18:11
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy

Re: Critical bug in builtin authentication handler

by rubenwardy » Mon Feb 13, 2017 00:31

Both of these are from old code.
Also, feel free to post bug reports as technical as these straight to the issue tracker
 

Fixerol
Member
 
Posts: 633
Joined: Sun Jul 31, 2011 11:23
IRC: Fixer
In-game: Fixer

Re: Critical bug in builtin authentication handler

by Fixerol » Mon Feb 13, 2017 00:38

Nice! Can you post this on github as a pull request? This will get it moving much faster... main dev and discussion is done over github and in #minetest-dev.
 

843jdc
Member
 
Posts: 207
Joined: Tue Jan 19, 2016 16:46
GitHub: jdc843
IRC: jdc843
In-game: 843jdc

Re: Critical bug in builtin authentication handler

by 843jdc » Mon Feb 13, 2017 00:42

My problem was while using the latest (as of 1-2 weeks ago) 0.4.15-git
I am currently using 0.4.14-git because I didn't want to take the time to recompile MT to run directly on the host OS.
 

User avatar
sorcerykid
Member
 
Posts: 219
Joined: Fri Aug 26, 2016 15:36
In-game: Nemo

Re: Critical bug in builtin authentication handler

by sorcerykid » Mon Feb 13, 2017 00:52

rubenwardy wrote:Both of these are from old code.
Also, feel free to post bug reports as technical as these straight to the issue tracker


Pardon me, but this is not "old code". Check it for yourself, the problematic code is still there:

https://github.com/minetest/minetest/bl ... e/auth.lua

There have been only TWO commits to auth.txt in the past year. Neither of them addressed this issue whatsoever.

No wonder critical bugs like this persist for so long, when even core developers refuse to take legitimate reports seriously.
Last edited by sorcerykid on Mon Feb 13, 2017 00:57, edited 1 time in total.
 

User avatar
rubenwardy
Member
 
Posts: 4500
Joined: Tue Jun 12, 2012 18:11
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy

Re: Critical bug in builtin authentication handler

by rubenwardy » Mon Feb 13, 2017 00:55

my point is that current QA practices do not apply, as this code was merged before current QA practices.

https://github.com/minetest/minetest/bl ... e/auth.lua

I wasn't invalidating the issue. It is an issue.
 

User avatar
sorcerykid
Member
 
Posts: 219
Joined: Fri Aug 26, 2016 15:36
In-game: Nemo

Re: Critical bug in builtin authentication handler

by sorcerykid » Mon Feb 13, 2017 01:01

Ah, okay. That makes sense. I will go ahead and post this on the issue tracker. I mainly reported it here so that other server operators were aware of the bug and could get a patch in place immediately.
 

User avatar
sorcerykid
Member
 
Posts: 219
Joined: Fri Aug 26, 2016 15:36
In-game: Nemo

Re: Critical bug in builtin authentication handler

by sorcerykid » Fri Mar 03, 2017 04:33

I think I should clarify that the patch I supplied above does not make any alterations to the auth.txt file whatsoever. It merely reduces the frequency of disk writes, as that is the bottleneck that leads to server hangs.

With my patch in place, the size of auth.txt is is effectively unlimited as the file is only written on server shutdown or by means of a forced commit. The JT2 server currently has over 174,000 registered players, and the authentication mechanism no longer impairs performance.
 

User avatar
sfan5
Member
 
Posts: 3636
Joined: Wed Aug 24, 2011 09:44
GitHub: sfan5
IRC: sfan5

Re: Critical bug in builtin authentication handler

by sfan5 » Fri Mar 03, 2017 15:07

sorcerykid wrote:No wonder critical bugs like this persist for so long, when even core developers refuse to take legitimate reports seriously.

There are exactly two correct ways to deal with problems you encounter:
  • Create an issue on the issue tracker to make developers aware
  • Make a pull request on Github where you offer your fixed code
Notice how "Write a 50 lines post on the forums" isn't one of them?
Stop dicking around on the forum writing overly long posts that claim a "critical bug" in some part of Minetest and whines about developers not taking you seriously or them supposedly not wanting to fix it. This is exactly the same for the PlayerSAO issue you reported in another thread, which –by the way– contains an incorrect analysis of what the involved code is supposed to do in what circumstances.
If you want your bug fixed get on github, if you don't then just stop.

Now for the content itself:
  • 44MB for an auth database is huge, Clean it out. You don't have 100.000 active players!
  • That's not SHA1 (its hash would be only be 20 bytes), it's the data stored by SRP
  • The usage of SHA1 is not a concerning decision nor unjustifiably excessive, it is the minimum acceptable password security even for a game.
  • even moreso why it persisted for well over a year through multiple version changes」 This is exactly the problem, you can't fix issues you don't know about.
Mods: Mesecons | WorldEdit | Nuke
Minetest builds for Windows (32-bit & 64-bit)
 


Return to Minetest Problems

Who is online

Users browsing this forum: No registered users and 11 guests

cron