Ticket #5487 (closed defect: fixed)

Opened 5 months ago

Last modified 4 months ago

query.php mistakenly uses is_admin() to check for admin privileges

Reported by: pishmishy Assigned to: pishmishy
Priority: high Milestone: 2.3.2
Component: Security Version: 2.3.1
Severity: major Keywords: query is_admin has-patch dev-feedback
Cc:

Description (Last modified by lloydbudd)

1. Create a draft post 2. Log out 3. Visit http://yourblog.com/index.php/wp-admin/

  • is_admin() spots the wp-admin in the request and returns true
  • query.php uses is_admin() to decide to return future, draft or pending posts

4. Future, draft and pending posts are displayed.

This doesn't require the ' in the request string as reported on Bugtraq.

See http://www.securityfocus.com/archive/1/485252/30/0/threaded

12/22 additional disclosure, with trivial, popular example: http://www.blackhatdomainer.com/how-to-know-today-what-shoemoney-is-going-to-post-tomorrow/

Attachments

5487.patch (0.7 kB) - added by pishmishy on 12/19/07 17:29:09.
New patch improves is_admin()
5487.002.diff (429 bytes) - added by markjaquith on 12/27/07 23:50:28.
Don't we need to fix this too?

Change History

12/19/07 15:37:09 changed by pishmishy

  • status changed from new to assigned.

12/19/07 15:37:36 changed by pishmishy

See line 1172 of query.php for the misuse of is_admin()

12/19/07 16:10:18 changed by pishmishy

  • keywords changed from query is_admin to query is_admin has-patch dev-feedback.

Attached patch replaces is_admin() check with current_user_can('level_10'). Perhaps we could explicitly check the user's capabilities but I wasn't sure from the documentation which capabilities we should be looking at. Instead I've just checked if the user is the administrator or not.

(follow-up: ↓ 11 ) 12/19/07 16:47:19 changed by docwhat

What I did (Wordpress 2.3.1):

  • Logged into wp-admin with Firefox.
  • Created a new post called "DRAFT", with text "DRAFT"
  • I saved it (but did not publish it)
  • I opened another browser (Opera).
  • I tried using the URL you had above (modified for my site) and it does not show me drafts.
  • I tried adding the p=<post number> get argument, but I just get a blank page.

I cannot reproduce this problem.

Will the current_user_can() allow the author (possibly a non-admin) to view the draft post that he/she just wrote?

Ciao!

12/19/07 17:07:44 changed by ryan

We do a current_user_can() check in the block of code already. is_admin() is used to see what context the user is in. Is the user in the admin? I think we need to retain is_admin() and have it check a constant set in admin.php to determine admin context.

12/19/07 17:16:18 changed by ryan

Hmm, the current_user_can() check is just for private posts. I think we need both an is admin user and is in the admin checks.

12/19/07 17:19:27 changed by ryan

Actually, edit-pages.php and edit.php filter the results of the is_admin() query. So I think all we need is a proper is_admin() check and not any cap checks.

12/19/07 17:29:09 changed by pishmishy

  • attachment 5487.patch added.

New patch improves is_admin()

12/19/07 17:30:47 changed by pishmishy

New patch improves is_admin(). Old patch was confused over why is_admin() was used in the first place. Thanks to Austin Matzko from wp-hackers for the idea.

12/19/07 17:37:29 changed by ryan

Looks good to me.

12/19/07 17:56:16 changed by ryan

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [6412]) Better is_admin() check from filosofo and pishmishy. fixes #5487

(in reply to: ↑ 4 ) 12/19/07 21:33:29 changed by docwhat

Replying to docwhat:

What I did (Wordpress 2.3.1):

Finally, someone posted what was missing. The unposted drafts have a date of something really old (1969 or 1999). You have to search back into the archive to find it.

Ciao!

12/21/07 01:28:48 changed by ryan

(In [6442]) Better is_admin() check from filosofo and pishmishy. fixes #5487 for 2.3

12/21/07 03:19:34 changed by lloydbudd

  • milestone changed from 2.4 to 2.3.2.

12/23/07 06:53:47 changed by lloydbudd

  • description changed.

12/27/07 23:50:28 changed by markjaquith

  • attachment 5487.002.diff added.

Don't we need to fix this too?

12/27/07 23:51:25 changed by markjaquith

  • status changed from closed to reopened.
  • resolution deleted.

$wp_query->is_admin (the var) is checked in some places and is still using the old logic instead of the is_admin() function. Shouldn't we fix that too? See patch.

12/28/07 01:04:17 changed by ryan

(In [6509]) Use is_admin. Props markjaquith. see #5487

12/28/07 01:04:31 changed by ryan

(In [6510]) Use is_admin. Props markjaquith. see #5487

12/28/07 21:25:08 changed by ryan

  • status changed from reopened to closed.
  • resolution set to fixed.