Disable users with breached passwords by dstufft · Pull Request #4541 · pypi/warehouse

dstufft · 2018-08-12T00:07:53Z

This pull request effectively disables a user if their password appears in the HIBP data until they reset their password. Whenever the user attempts to authenticate, we will check their password using the HIBP breached password service, and if it comes back as a positive hit we will disable their password, preventing any future attempts to authenticate, send them an email with information, and finally return an error ultimately preventing them from authenticating.

While we can't iterate over our database and look for any users that have compromised credentials, this will effectively do the same thing since once a password has appeared in the HIBP data, it will no longer be usable to log into PyPI.

This will be in effect both for users authenticating using the log in form in the UI and for users uploading files to PyPI.

This is a fairly abrupt interruption to the user's flow-- they are attempting to do something and we forcibly prevent them and require them to do something else first. However this is considered to be an acceptable trade off in this case, because these password are known to the general public, and can be used in credential stuffing attacks on PyPI. This was recently the cause of a fairly large security issue on NPM, where reused credentials by one of the users for a popular package were used to upload malicious packages (more information).

This branch is currently working, however it still needs:

A plaintext (non HTML) error message when authenticating via Basic Auth.
Tests
A solution for local development. We currently have all users set to have the password, password, which doesn't work because it has been compromised. Likely we should just have a no-op breached password service in local development.
Maybe give a different error when we're failing during authentication versus when changing the password? Consistency in error message makes it easier to google the problem, but the error message is more generic and harder to know exactly what just happened.

I'm submitting this PR now, so that people can start to review the messaging that we're sending to users. Particularly the emails that i've written (plain text and HTML) and the additional information I've added to the FAQ.

Fixes #4471.

dstufft · 2018-08-12T00:09:19Z

One additional thing, we probably want to announce this change to pypi-announce and to distutils-sig whenever we merge it. We probably want to get some messaging sorted out for that as well.

dstufft · 2018-08-12T02:17:49Z

Ok, besides the two open questions:

Is the wording in the emails/FAQ OK? Do we need to adjust either of them?
Do we want separate error messages for failing during authentication (logging in, uploading files) and changing passwords? Currently this PR does not have separate messages, under the assumption it's better to have a singular message to make it easier to google.

this is ready to go.

This does add a setting:

BREACHED_PASSWORDS=warehouse.accounts.NullPasswordBreachedService
BREACHED_PASSWORDS=warehouse.accounts.HaveIBeenPwnedPasswordBreachedService

However the default for that option is the HIBP service, so we do not need to change anything in production. The local development environment has that setting to enable the NullPasswordBreachedService so that we can still use the password password in development.

brainwane · 2018-08-12T03:32:16Z

Thanks. Will take a look tomorrow.

hugovk

Some language suggestions based on https://web.archive.org/web/20180410101124/https://material.io/guidelines/style/writing.html and https://plainlanguage.gov/guidelines/words/use-simple-words-phrases/

hugovk · 2018-08-12T05:12:38Z

+
+# How do you know this?
+
+We utilize a free security service from HaveIBeenPwned. When registering,


Use plain English "use" instead of "utilize".

hugovk · 2018-08-12T05:15:04Z

+
+We utilize a free security service from HaveIBeenPwned. When registering,
+authenticating, or updating your password, we generate a SHA1 hash of your password and
+use the first five (5) characters of the hash to determine if the password has been


Just "5" instead of "five (5)".

"determine" - > "decide"

"has been compromised" - > "is compromised"

hugovk · 2018-08-12T05:32:35Z

+{% block content %}
+# What?
+
+During your recent attempt to log in or upload to PyPI, we noticed that your password


Can delete "that".

hugovk · 2018-08-12T05:33:44Z

+# What?
+
+During your recent attempt to log in or upload to PyPI, we noticed that your password
+has appeared in public data breaches. To protect you and other users, we have


"has appeared in" - > "appears in"

hugovk · 2018-08-12T05:39:43Z

+
+During your recent attempt to log in or upload to PyPI, we noticed that your password
+has appeared in public data breaches. To protect you and other users, we have
+preemptively reset your password and you will no longer be able to log in or upload to


"you will no longer be able to" - > "you can no longer" or "now you can not"

hugovk · 2018-08-12T05:56:41Z

            If you receive an error message saying that "This password has appeared in a breach or has otherwise been compromised and cannot be used", you should change it all other places that you use it as soon as possible.
          </p>
+          <p>
+            If you've received this error while attempting to log in to or upload to PyPI, then your password has been reset and you will no longer be able to log into PyPI until you <a href="{{ request.route_url('accounts.reset-password') }}">reset your password</a>.


"you've" - > "you",

"log in to" - > "log in",

"you will no longer be able to" - > "you cannot"

I didn't like the way this sentence read changing you've to you, but I expanded it out to "you have".

hugovk · 2018-08-12T05:59:13Z

+            (
+                None,
+                (
+                    "This password has appeared in a breach or has otherwise "


"has appeared in" - > "appears in"

Delete "otherwise" and trailing fullstop

hugovk · 2018-08-12T06:03:23Z

+            (
+                "http://localhost/help/#compromised-password",
+                (
+                    "This password has appeared in a breach or has otherwise "


"has appeared in" - > "appears in"

Delete "otherwise"

hugovk · 2018-08-12T06:04:52Z

 class HaveIBeenPwnedPasswordBreachedService:
+
+    _failure_message_preamble = (
+        "This password has appeared in a breach or has otherwise been compromised and "


"has appeared in" - > "appears in"

Delete "otherwise" and trailing fullstop

hugovk · 2018-08-12T06:07:29Z

+@implementer(IPasswordBreachedService)
+class NullPasswordBreachedService:
+    failure_message = "This password has appeared in a breach."
+    failure_message_plain = "This password has appeared in a breach."


"has appeared in" - > "appears in"

dstufft · 2018-08-12T07:39:03Z

Addressed the feedback by @hugovk.

brainwane · 2018-08-13T02:48:46Z

+        user_service.update_user(user.id, password="foo")
+        assert user.password != "!"
+
+        # Now we'll actually test our disble function.


Should be "disable"

brainwane · 2018-08-13T02:50:30Z

+    def create_service(cls, context, request):
+        return cls()
+
+    def check_password(self, password, *, tags=None):


I think I don't quite understand under what circumstances we want to use this?

We use it in local development, because we set all users to have the password "password", which appears in breaches. Thus without the NullPasswordBreachedService in local development, we would never be able to log into those users, and would have to generate secure password for them.

brainwane · 2018-08-13T02:51:46Z

@@ -178,6 +178,9 @@ <h3 id="compromised-password">{{ compromised_password() }}</h3>
          <p>
            If you receive an error message saying that "This password has appeared in a breach or has otherwise been compromised and cannot be used", you should change it all other places that you use it as soon as possible.


Do we need to update the wording of the quoted error message here?

Yup, good catch.

dstufft · 2018-08-13T02:57:29Z

Feedback from @brainwane addressed and responded to!

ewdurbin · 2018-08-13T15:41:31Z

+<h3>What should I do?</h3>
+<p>
+  To regain access to your account,
+  <a href="{{ request.route_url('accounts.reset-password') }}">reset your password</a>


I think this should be request.route_url('accounts.request-password-reset')

ewdurbin · 2018-08-13T15:41:49Z

+# What should I do?
+
+To regain access to your account, reset your password on PyPI
+({{ request.route_url('accounts.reset-password') }}). We also recommend that you go to


I think this should be request.route_url('accounts.request-password-reset')

ewdurbin · 2018-08-13T15:42:00Z

+            If you receive an error message saying that "This password appears in a breach or has been compromised and cannot be used", you should change it all other places that you use it as soon as possible.
+          </p>
+          <p>
+            If you have received this error while attempting to log in or upload to PyPI, then your password has been reset and you cannot log in to PyPI until you <a href="{{ request.route_url('accounts.reset-password') }}">reset your password</a>.


I think this should be request.route_url('accounts.request-password-reset')

ewdurbin · 2018-08-13T15:43:44Z

Noticed that we're sending folks to the wrong URL in the emails/FAQ. This route flashes "Invalid token: no token supplied", as indeed no token is supplied.

dstufft · 2018-08-13T23:04:58Z

Going to go ahead and merge this once tests pass. I believe that all of the feedback was addressed. If there are any further changes we can address that in a future PR.

Addressed.

dstufft requested review from brainwane, di, ewdurbin and nlhkabu August 12, 2018 00:07

dstufft force-pushed the disable-users-with-breached-passwords branch 3 times, most recently from 2b33ae2 to ff596b5 Compare August 12, 2018 01:33

dstufft changed the title ~~[WIP] Disable users with breached passwords~~ Disable users with breached passwords Aug 12, 2018

hugovk reviewed Aug 12, 2018

View reviewed changes

brainwane previously requested changes Aug 13, 2018

View reviewed changes

dstufft force-pushed the disable-users-with-breached-passwords branch from f3ea5d8 to b4f937c Compare August 13, 2018 02:56

ewdurbin approved these changes Aug 13, 2018

View reviewed changes

ewdurbin reviewed Aug 13, 2018

View reviewed changes

dstufft added 7 commits August 13, 2018 19:03

Move HIBP check from login view to LoginForm

f9b1127

Prevent login and disable user if using breached password

83dae74

Add a plaintext failure message to the breach service

72dc169

Add a NullPasswordBreachedService to disable breached password checking

856d1d1

Address feedback on language

233ac80

Address feedback

176fb09

Use the correct URL

fb02683

dstufft force-pushed the disable-users-with-breached-passwords branch from b4f937c to fb02683 Compare August 13, 2018 23:03

dstufft merged commit b30d908 into pypi:master Aug 13, 2018

dstufft deleted the disable-users-with-breached-passwords branch August 13, 2018 23:14


		# How do you know this?

		We utilize a free security service from HaveIBeenPwned. When registering,

		@@ -178,6 +178,9 @@ <h3 id="compromised-password">{{ compromised_password() }}</h3>
		<p>
		If you receive an error message saying that "This password has appeared in a breach or has otherwise been compromised and cannot be used", you should change it all other places that you use it as soon as possible.

Conversation

dstufft commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstufft commented Aug 12, 2018

Uh oh!

dstufft commented Aug 12, 2018

Uh oh!

brainwane commented Aug 12, 2018

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstufft commented Aug 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstufft commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewdurbin commented Aug 13, 2018

Uh oh!

dstufft commented Aug 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dstufft commented Aug 12, 2018 •

edited

Loading