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

[4.2] Spell jacking protection/hardening #39241

Closed
wants to merge 4 commits into from

Conversation

brianteeman
Copy link
Contributor

All html5 input fields default to have spellcheck enabled. The exception for obvious reasons is the password field. You wouldn't want that to be spellchecked for hopefully obvious reasons.

However in Joomla, and virtually every other system, the show password button converts the password input to a regular text input.

In the last few months it has been disclosed that both Google and Microsoft provide (optional) advanced spellcheckers that will transmit the contents of any input with spellcheck enabled over the net so that it can be spellchecked. This means that as soon as you click on the show password button that your password would be transmitted over the net in plain text.

Now that this has been publically disclosed it is fairly certain that some bad actors will, or already have, create browser add-ins that will do the same while pretending to do something very innocent instead.

You can find out more about this by searching for spell-jacking. To the best of my knowledge to date only lastpass have implemented protection for this.

It is a suprisingly simple thing to fix. It is just a one line addition to the existing javascript passwordview.es6.js so that when the password field is changed to a text field the attribute spellcheck = off is added.

Testing

As this PR changes the javascript you will need to either use a prebuilt package or npm run build:js

Then go to any password field that uses the show password button eg the site or admin login module. Inspect the password field and it will look like this
<input name="passwd" id="mod-login-password" type="password" class="form-control input-full valid form-control-success" required="required" autocomplete="current-password" aria-invalid="false">

Then click show password and inspect again.

Before this pr the code after pressing show password will look like this
<input name="passwd" id="mod-login-password" type="text" class="form-control input-full valid form-control-success" required="required" autocomplete="current-password" aria-invalid="false">

After this pr the code will be changed to look like this.
<input name="passwd" id="mod-login-password" type="text" class="form-control input-full valid form-control-success" required="required" autocomplete="current-password" aria-invalid="false" spellcheck="false">

This was first reported privately to the JSST but they instructed me to create a public pr.

Tagging @nikosdion as I don't remember if any of the webauth code has a "show password" function that is not using the core js that is modified here

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Nov 18, 2022
@HLeithner
Copy link
Member

I'm asking my self if it would be better to add this directly to the password HTML tag when it's type password already.

Without knowing the implementation details of the browser it's possible that between you change the type and set the spell attribute, the browser already sends the text?

And I think it doesn't hurt to have it always set on a password field.

@nikosdion
Copy link
Contributor

@brianteeman WebAuthn's proper name is “Passwordless Authentication with W3C Web Authentication”. As a result no, it does not use passwords and that's they entire point of its existence. It is Joomla embracing a post-password a.k.a. passwordless future long before it was hip and mainstream.

@brianteeman
Copy link
Contributor Author

I'm asking my self if it would be better to add this directly to the password HTML tag when it's type password already.

There is no need. It is part of the spec for the password input type specification. If there was a need for it then all the security references to spell-jacking would suggest it - they don't

@nikosdion I was 99% certain just wanted to check that it wasn't being used for secrets

To the best of my knowledge to date only lastpass have implemented protection for this.

I see contao have implemented this https://github-com.translate.goog/contao/contao/pull/5317

@nikosdion
Copy link
Contributor

@brianteeman WebAuthn does NOT communicate secrets to the browser. In fact, it does not even store them in the database! We only store the information which is 100% safe to freely leak to the world: a public key. The private key is only ever stored in the authenticator itself, typically in a hardened, purpose-built chip. The only thing we send to the browser is a bunch of random bytes it asks the authenticator to sign with its private key and which we have stored in the session. The result is sent to us and we check it against the public key and the session stored random bytes. If it checks out, we've mathematically proven the authenticator matches the public key we had recorded on registration and we can log in the user. That's why I have been saying that WebAuthn is infinitely more secure than passwords. If someone gets ahold of the database they can't subvert WebAuthn but they can and will crack passwords (with more or less difficulty, as I've been presenting for well over a decade).

Regarding this PR, may I please draw your attention to the very different system semantics of a password vs a regular input field.

Password fields are special. Their contents are only made available to the owner application (browser) and the JavaScript running directly on the page. They cannot be copied from, their values are not accessible to browser extensions or even the system accessibility layer. They are protected.

Regular input fields, on the other hand, are a free-for-all. Parent / child tabs can read their values (on older browsers; newer Chromium and WebKit builds forbid it), browser extensions can read their values, the system accessibility layer can read their values — which includes any application which has received accessibility permissions. All it takes is a malicious browser extension or system application with accessibility permissions to steal their contents.

Just having the possibility to reveal passwords (convert the password field to a regular text input) is a security risk, regardless of spelljacking. If you really care about the user's security remove this stupid feature altogether, or at the very least make it optional with a Global Configuration option that's DISABLED BY DEFAULT.

@brianteeman
Copy link
Contributor Author

That's really a different issue and off topic for this pr. Happy to have that discussion but it's not for here

@nikosdion
Copy link
Contributor

@brianteeman Let me quote verbatim what you wrote in your PR description:

Now that this has been publically disclosed it is fairly certain that some bad actors will, or already have, create browser add-ins that will do the same while pretending to do something very innocent instead.

Malicious developers won't be creating fake spell-checker browser extensions anytime soon because a. they are not given an API for that and b. they have already created malicious extensions which trawl form data to steal passwords. Just in the last tow months Google flushed several dozens of malicious Chrome extensions which were stealing from data, many of them with tens of thousands of downloads or more.

By having the ability to convert the password field to a regular text field we throw the users right into the clutches of the malicious browser extension developers you proclaim in your PR description you will protect them against.

Even worse, you are now asserting that you are protecting them against malicious extensions (even though you actually don't), which will lead to complacency among users, therefore put them in even more real danger than they already are.

Even worse than that, you are completely ignoring the fact that mobile devices' keyboard applications are already doing web calls to spell-check password fields converted to text fields and they even "remember" what you've typed. When it's a password field they are communicated it's a password field (Android) or the system keyboard is used (iOS) with the spell-check and work remembering features disabled. Therefore, your PR does NOT address spell-jacking on mobile devices at all, which is very much on-topic with your PR as you are asserting that it's protecting against spell-jacking.

I believe that security of the feature you are touching in your ostensibly security-oriented PR is very much on-topic. If you disagree, so be it. I have publicly stated the facts which lead me to mark this PR with 👎 .

@HLeithner
Copy link
Member

have you seen the order contao is doing it, beside that it's better to not set it later since we are able to do it in html we should do it as early as possible.

I don't want to discuss the possibility to show the password here, that's definitive the wrong place.

@nikosdion
Copy link
Contributor

@HLeithner Okay, since actual password security is deemed off-topic on a this security whitewashing PR, I opened a new issue with the real security issue that needs to be solved for password security to be restored for Joomla users: #39250

@sandewt
Copy link
Contributor

sandewt commented Dec 24, 2022

I have tested this item 🔴 unsuccessfully on 2986a4e

The input.setAttribute('spellcheck', 'false');statement is not executed


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39241.

@brianteeman brianteeman deleted the spell-jacking branch December 24, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants