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

Auth: Port VMaNGOS implementation of TOTP for 1.12 #513

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

insunaa
Copy link
Contributor

@insunaa insunaa commented Jul 4, 2023

🍰 Pullrequest

This PR prevents realmd from requiring a valid token response from the client if the client is older than 1.12.3, as it was only implemented in 2.4.3. Checking specifically for 1.12.3 and not for 2.4.3 because a TBC server may possibly allow other builds than just 2.4.3 and it shouldn't be possible to use these to circumvent a token that would otherwise be enforced with 2.4.3

@killerwife
Copy link
Contributor

Vanilla client has a keypad that we are using for token. Was not aware it was broken. This keypad is then not present in tbc.

@insunaa
Copy link
Contributor Author

insunaa commented Jul 4, 2023

How do the implementations differ? Because if the token field is filled with a token and a vanilla client tries to log in, it just fails with a connection error.

Edit: OK PIN is indeed implemented clientside, but the logon-challenge never sends a PIN request and I don't know if it checks for pin reply

Edit: we also don't have any handlers for pin reply (cmd 181 and on pin entry then cmd 96)

Edit3: VMaNGOS has PIN implemented. I'll check their implementation to see where we go wrong

@insunaa
Copy link
Contributor Author

insunaa commented Jul 4, 2023

@killerwife I added the VMaNGOS code for TOTP authentication using the PIN keypad (tho I disabled the random scrambling of it. it's hard enough as it is to type 6 digits on that keypad within 30 seconds, to do it with a scrambled keypad is a huge pain)

How do I properly credit VMaNGOS?

pkt << uint32(0);
pkt << uint64(0);
pkt << uint64(0);
uint32 gridSeedPkt = m_gridSeed = static_cast<uint32>(0/*urand()*/);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as 0 instead of urand() (neither of which really need a static_cast, but the compiler will optimize that out anyway) means that the keypad on the client is not scrambled. Adding any other number (or urand()) here will scramble the keypad, making text entry more difficult for no real benefit. It's not a physical keypad where an attacker could look at your finger movements to determine the code you're entering, after all. If an attacker can see your screen, you have bigger problems.

Left it in in case someone sees the need to enable scrambling, tho.

@insunaa insunaa changed the title Auth: only check token for post 1.12.3 clients Auth: Port VMaNGOS implementation of TOTP for 1.12 Jul 4, 2023
@insunaa
Copy link
Contributor Author

insunaa commented Oct 21, 2023

Any progress on this?
Nothing in this PR should be controversial or blocking

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