WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 2 months ago

#16483 new defect (bug)

Visibility: password-protected exposes multiple pages

Reported by: monkeyhouse Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0.4
Component: Security Keywords: dev-feedback needs-testing has-patch early
Focuses: Cc:

Description

  1. password protect a page ('protected') with a password
  2. password protect another page ('thistoo') with the SAME password
  3. visit 'protected' and enter the password. Page is visible
  4. visit 'thistoo'; expected: prompt for password. What happens: Page is visible

Regardless of whether someone with a password has the right to try it in as many pages as they want (and would therefore successfully see the page if the passwords were the same), the user should still be prompted on a page-by-page basis. Global authentication to multiple pages is possible with user accounts and roles. It should not be possible with visibility: password-protected pages.

Attachments (8)

16483.diff (2.6 KB) - added by solarissmoke 7 years ago.
Make passwords post-specific
16483.2.diff (2.9 KB) - added by garyc40 7 years ago.
modified solarissmoke's patch a bit: no notice when not called inside a loop
16483.3.diff (2.1 KB) - added by SergeyBiryukov 5 years ago.
16483.4.diff (2.9 KB) - added by SergeyBiryukov 5 years ago.
16483.5.diff (2.9 KB) - added by SergeyBiryukov 5 years ago.
Refreshed
16483.6.diff (2.4 KB) - added by voldemortensen 16 months ago.
16483.7.diff (3.6 KB) - added by voldemortensen 14 months ago.
16483.8.diff (2.8 KB) - added by voldemortensen 14 months ago.

Download all attachments as: .zip

Change History (45)

@solarissmoke
7 years ago

Make passwords post-specific

#1 @solarissmoke
7 years ago

  • Keywords has-patch dev-feedback added

I agree that this shouldn't happen - although plain text passwords in cookies aren't really going to prevent someone who is determined ;)

Here's one possible patch. It may cause some issues because previously (and since WP 1.0.0) get_the_password_form() didn't require a post. I can't find any instances in core that don't have a post (or implicit post global) set, but I might have missed something.

@garyc40
7 years ago

modified solarissmoke's patch a bit: no notice when not called inside a loop

#2 @MikeHansenMe
5 years ago

  • Cc mdhansen@… added
  • Keywords needs-refresh added

This problem still exists in 3.5-RC1-22924. Patch needs refresh.

#3 @SergeyBiryukov
5 years ago

  • Keywords needs-testing added; needs-refresh removed

#4 follow-up: @MikeHansenMe
5 years ago

16483.3.diff does not seem to let me log in to the page at all even with the correct password.

#5 in reply to: ↑ 4 @SergeyBiryukov
5 years ago

Replying to MikeHansenMe:

16483.3.diff does not seem to let me log in to the page at all even with the correct password.

Thanks, fixed in 16483.4.diff. Missed [19728] in the previous patch.

#6 @SergeyBiryukov
5 years ago

  • Component changed from General to Security
  • Milestone changed from Awaiting Review to 3.6

Related: #19797

@SergeyBiryukov
5 years ago

Refreshed

#7 @ryan
5 years ago

  • Milestone changed from 3.6 to Future Release

This has been discussed a number of times and there are many who consider this a feature. Punting back to Future Release since it requires negotiations.

#8 follow-up: @F J Kaiser
4 years ago

I just had a use case/question on WP.SE where this "feature" was actually wanted. But I agree that it is the opposite of ideal.

Keep in mind that #20308 will allow to query by has_password as well as by post_password. So this ticket will likely go against the new feature for WP_Query arguments.

#9 in reply to: ↑ 8 @nacin
4 years ago

Replying to F J Kaiser:

Keep in mind that #20308 will allow to query by has_password as well as by post_password. So this ticket will likely go against the new feature for WP_Query arguments.

Yes and no. There are two options:

  1. Use one cookie not keyed to a post, which means multiple posts can be accessed at once after entering a single password, but a post with a different password cannot be accessed without overwriting the cookie. (Current.)
  2. Use a cookie keyed to each post, which means multiple posts can be viewed even if they don't have the same password. Every post causes a prompt even if they have the same password. (Proposed.)

I wouldn't mind a way to toggle between these states. Really, what we need is a filter on the cookie name, right?

But in both cases, multiple posts having the same password are still linked in some regard: the user has a single password that unlocks one or more posts. Whether they need to enter it again or not doesn't really affect how WP_Query can now query for post passwords. I would be OK with ignoring that API change for the purposes of making a decision here.

#10 @chriscct7
2 years ago

  • Keywords needs-refresh added; has-patch removed
  • Severity changed from minor to normal

#11 @voldemortensen
16 months ago

  • Milestone changed from Future Release to 4.7

I would love to get this into 4.7. This "feature" could have some pretty nasty consequences for an unsuspecting site admin.

#12 @voldemortensen
16 months ago

  • Keywords has-patch added; needs-refresh removed

#13 follow-up: @helen
16 months ago

Could I please have a summary of the following, both for myself and for general reference?

  1. What is the proposed solution in the patch?
  2. How does this affect existing post passwords?
  3. How would original functionality be restored via plugin?
  4. How does this relate to WP_Query changes in #20308 cited above?
  5. How does this affect the REST API? See https://github.com/WP-API/WP-API/issues/1055

#14 in reply to: ↑ 13 @voldemortensen
16 months ago

Replying to helen:

Could I please have a summary of the following, both for myself and for general reference?

  1. What is the proposed solution in the patch?

The proposed solution is to alter the cookie to be post specific, as opposed to password specific. Currently, any posts with the same password can be viewed at the same time.

  1. How does this affect existing post passwords?

As far as I am aware, this would invalidate all current cookies and force everyone to re-authenticate to any protected posts. I consider this a non-issue for a few reasons. Cookies can accidentally be deleted, aren't available across all devices, clearing browser history often deletes them, etc, etc. The are so many cases for cookies disappearing this, to me, doesn't seem like a big deal.

  1. How would original functionality be restored via plugin?

In the proposed solution, it wouldn't.

  1. How does this relate to WP_Query changes in #20308 cited above?

I currently don't know the answer to this question, but I will find out.

  1. How does this affect the REST API? See https://github.com/WP-API/WP-API/issues/1055

After reading the REST API issue, it seems like it would make life a little better for them. It has been noted a couple times that an "ugly nuance of core" is that its cookie based and only based on COOKIEHASH, so only one password protected post can be viewed at a time. This fixes that so multiple password protected posts can be viewed.

#15 follow-up: @voldemortensen
16 months ago

After reading #20308, the only way I can see that effecting anything is if a developer does the following:

Checks the protected posts cookie if it exists.
If it exists, only show posts that have that exact password.

However, the query would still be able to list all posts with the same password. This would just require users to enter the password for each post. I'm struggling to come up with a scenario where this would be the solution though.

Either way, the remedy to this, if its even an issue, could be doing what @nacin suggested and adding a filter to the cookie name. That would allow a plugin to restore this "feature".

#16 in reply to: ↑ 15 @helen
16 months ago

Replying to voldemortensen:

How would original functionality be restored via plugin?

In the proposed solution, it wouldn't.

Either way, the remedy to this, if its even an issue, could be doing what @nacin suggested and adding a filter to the cookie name. That would allow a plugin to restore this "feature".

This should be accounted for in whatever solution is implemented.

This ticket was mentioned in Slack in #core-restapi by helen. View the logs.


16 months ago

#18 @jorbin
15 months ago

In 38603:

Posts: Add filter to allow overriding post_password_required return

Post Passwords are incredibly inflexible. One Password per site at a time and other limitations that can't really be changed without a backwards compatibility break. This adds the ability for sites to change the password behavior such as doing per post passwords or allowing multiple passwords to be set in a browser. The possibilities are YUGE.

Additionally, it allows for a behavior other than returning a html form when a password is needed. This is important for non website use cases (such as in a restful API).

Fixes #38056. See #16483.
Props rmccue.

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


15 months ago

#20 @jorbin
15 months ago

  • Keywords close added

Changing this is backwards compatibility break that I'm not really a fan of.

The new filter introduced in [38603] allows for changing the password protected post behavior on any sites that are interested in doing so.

I think we can close this as wontfix. The behavior where one password can open up multiple posts is a feature at this point.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


15 months ago

#22 @helen
15 months ago

  • Keywords close removed

I'm personally fine with breaking existing behavior - it is very unexpected (i.e. a bug) and nothing in screen help or even the codex warns you about the shared password behavior. I think people who are using it on purpose are likely to know WP fairly well and can install a plugin to restore it - the worst that happens in the meantime to their readers is they re-enter that same password which is ostensibly a known password.

If we don't change it, I think we need a review of copy around this feature, so still wouldn't close the ticket.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


14 months ago

#26 @voldemortensen
14 months ago

Please ignore 16483.7.diff. grunt-patch-wordpress did something I didn't expect.

16483.8.diff adds a filter to the cookie name so that previous functionality can be restored.

PoC mu-plugin to restore functionality:

<?php
function voldemortensen_reset_postpass_cookie_name($cookie) {
        return 'wp-postpass_' . COOKIEHASH;
}
add_filter('post_password_cookie', 'voldemortensen_reset_postpass_cookie_name', 10, 3);

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


14 months ago

#28 @helen
14 months ago

  • Keywords 4.8-early added
  • Milestone changed from 4.7 to Future Release

While I still think we should change this behavior at some point, going to punt this, as we would have wanted to do that pre-beta. If somebody would like to open another ticket for reviewing the copy around post passwords, that would be lovely, though I suppose it's been this long so it may not be worth changing for 4.7.

#29 @voldemortensen
14 months ago

I'm not going to play the change milestone game, that's just immature, but I would like to voice my opinion that this is a relatively small change. We are currently making much larger and most likely more impactful changes in the middle of beta. I don't see reason why this shouldn't go in.

#30 @helen
14 months ago

It is too late to introduce a breaking change, especially behavior that goes back many many releases. On top of that, nobody else has picked up on this ticket and I did not and do not personally have bandwidth to usher it in for 4.7.

#31 @voldemortensen
12 months ago

  • Milestone changed from Future Release to 4.8

This ticket was mentioned in Slack in #core by flixos90. View the logs.


7 months ago

#33 @flixos90
7 months ago

  • Keywords early added; 4.8-early removed
  • Milestone changed from 4.8 to Future Release

#34 @voldemortensen
6 months ago

  • Milestone changed from Future Release to 4.9

#35 @partyfrikadelle
6 months ago

You asked for a use-case where the actual behaviour makes sense? ;-) We got a customer, that uses same passwords over several Pages to make a complete "Chapter" available for the Users without having to "log in" again on every page.

I'm pretty unsure if you can call the actual behaviour a real "security risk". If the User just has to enter a password, he ALREADY KNOWS for the Site, i would not call that "secure" actually.

Let's be honest: 2 Parts of the actual Solution make it practically impossible to make it secure. One per-Page-Password will never be really secure. And the Password hashed in the Cookie? Will always be unsecure in my point of view.

The One-Cookie-Per-Post-Solution implies Problems for all Sites, that rely on CDN-Services like Cloudfront, that rely on Cookies and/or Header-Information to cache content. Having individual Cookies for every post will make it impossible to to whitelist Cookies properly AND preventing that a logged-in will get a complete personal cache because of a very specific Cookie-Set.

For preventing the above behaviour even for the normal password-cookie, i set the lifetime of the password-cookie to 1 second. This makes the user to enter the password on every page. Even if you reload the actual one.

<?php
function szs_set_cookie_expire () {
        return time() + 1; // 1 second.
}
add_filter('post_password_expires', 'szs_set_cookie_expire', 99);

But by that way no password-cookie ist sent at the next request and a CDN can response with the cached version as expected.

I would imagine that making the underlying security-problems with using the Post-Passwords more transparent is more important than changing one unsecure behaviour to a practically just as unsecure behaviour.

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


2 months ago

#37 @westonruter
2 months ago

  • Milestone changed from 4.9 to Future Release

Punting because early and we're in 4.9-beta1.

Note: See TracTickets for help on using tickets.