r82306 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r82305‎ | r82306 | r82307 >
Date:22:29, 16 February 2011
Author:happy-melon
Status:ok
Tags:
Comment:
Revert r78585. While I definitely think this is an area where there's scope for improvement in clarity and security, I'm no longer convinced that this is the way to proceed.
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/Token.php (deleted) (history)

Diff [purge]

Index: trunk/phase3/includes/Token.php
@@ -1,215 +0,0 @@
2 -<?php
3 -/**
4 - * Copyright © 2010
5 - * http://www.mediawiki.org/
6 - *
7 - * This program is free software; you can redistribute it and/or modify
8 - * it under the terms of the GNU General Public License as published by
9 - * the Free Software Foundation; either version 2 of the License, or
10 - * (at your option) any later version.
11 - *
12 - * This program is distributed in the hope that it will be useful,
13 - * but WITHOUT ANY WARRANTY; without even the implied warranty of
14 - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15 - * GNU General Public License for more details.
16 - *
17 - * You should have received a copy of the GNU General Public License along
18 - * with this program; if not, write to the Free Software Foundation, Inc.,
19 - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
20 - * http://www.gnu.org/copyleft/gpl.html
21 - *
22 - * @file
23 - */
24 -
25 -/**
26 - * CSRF attacks (where a malicious website uses frames, <img> tags, or
27 - * similar, to prompt a wiki user to open a wiki page or submit a form,
28 - * without being aware of doing so) are most easily countered by using
29 - * tokens. For normal browsing, loading the form for a protected action
30 - * sets two copies of a random string: one in the $_SESSION, and one as
31 - * a hidden field in the form. When the form is submitted, it checks
32 - * that a) the set of cookies submitted with the form *has* a copy of
33 - * the session cookie, and b) that it matches. Since malicious websites
34 - * don't have control over the session cookies, they can't craft a form
35 - * that can be instantly submitted which will have the appropriate tokens.
36 - *
37 - * Note that these tokens are distinct from those in User::setToken(), which
38 - * are used for persistent session authentication and are retained for as
39 - * long as the user is logged in to the wiki. These tokens are to protect
40 - * one individual action, and should ideally be cleared once the action is over.
41 - */
42 -class Token {
43 -
44 - /*
45 - * Some punctuation to prevent editing from broken
46 - * text-mangling proxies.
47 - */
48 - const TOKEN_SUFFIX = '+\\';
49 -
50 - /**
51 - * Different tokens for different types of action.
52 - *
53 - * We don't store tokens for some actions for anons
54 - * so they can still do things when they have cookies disabled.
55 - * So either use this for actions which anons can't access, or
56 - * where you don't mind an attacker being able to trigger the action
57 - * anonymously from the user's IP. However, the token is still
58 - * useful because it fails with some broken proxies.
59 - */
60 - const ANONYMOUS = 'Edit';
61 -
62 - /**
63 - * For actions requiring a medium level of protection, or where the
64 - * user will be making repeated actions: this token should not be
65 - * cleared once the action is completed. For instance, a user might
66 - * revert mass vandalism from a user by loading their contribs and
67 - * ctrl+clicking each rollback link. If we cleared the Token from
68 - * session after each rollback, they'd have to reload the contribs
69 - * page each time, which would be annoying.
70 - */
71 - const PERSISTENT = 'Action';
72 -
73 - /**
74 - * For actions requiring a high level of protection, and where the user
75 - * will not be performing multiple sequential actions without reloading
76 - * the form or link. Eg login, block/protect/delete, userrights, etc.
77 - * Callers should clear these tokens upon completion of the action, and
78 - * other callers should expect that they will be cleared.
79 - */
80 - const UNIQUE = 'Unique';
81 -
82 - /**
83 - * String the action which is being protected by the token
84 - * ('edit', 'login', 'rollback', etc)
85 - */
86 - protected $type = self::ANONYMOUS;
87 -
88 - /**
89 - * An instance-specific salt. So if you want to generate a hundred rollback
90 - * tokens for the watchlist, pass a $salt which is unique
91 - * to each revision. Only one token is stored in the session, but it is munged
92 - * with a different salt for each revision, so the required value in the HTML
93 - * is different for each case.
94 - */
95 - protected $salt = '';
96 -
97 - protected $request;
98 -
99 - /**
100 - * Constructor
101 - * @param $salt String an instance-specific salt. @see Token::$salt
102 - * @param $type Token class constant identifier
103 - * @param $request WebRequest most of the time you'll want to get/store
104 - * the tokens in $wgRequest, which is the default.
105 - */
106 - public function __construct( $salt, $type = self::PERSISTENT, WebRequest $request = null ){
107 - global $wgRequest;
108 - $this->type = $type;
109 -
110 - if( is_array( $this->salt ) ) {
111 - $this->salt = implode( '|', $this->salt );
112 - } else {
113 - $this->salt = strval( $salt );
114 - }
115 -
116 - $this->request = $request instanceof WebRequest
117 - ? $request
118 - : $wgRequest;
119 - }
120 -
121 - /**
122 - * Ensure that a token is set in cookies, by setting a new one
123 - * if necessary.
124 - * @param $purge Bool whether to overwrite an existing token in
125 - * session if there is one. This is more secure, but will
126 - * only allow one Token of a particular $action to be used on
127 - * the page (which may itself be a good thing).
128 - * @return String The version of the token which should be included
129 - * in the HTML form/link.
130 - */
131 - public function set( $purge = false ) {
132 - global $wgUser;
133 - if ( $this->type == self::ANONYMOUS && $wgUser->isAnon() ) {
134 - return self::TOKEN_SUFFIX;
135 - }
136 -
137 - if( $purge || $this->get() === null ){
138 - $token = self::generate();
139 - if( session_id() == '' ) {
140 - wfSetupSession();
141 - }
142 - $this->store( $token );
143 - } else {
144 - $token = $this->get();
145 - }
146 -
147 - return md5( $token . $this->salt ) . self::TOKEN_SUFFIX;
148 - }
149 -
150 - /**
151 - * Check whether the copy of the token submitted with a form
152 - * matches the version stored in session
153 - * @param $val String version submitted with the form.
154 - * @return Mixed null if no session token was set, Bool false if there
155 - * was a token but it didn't match, Bool true if it matched correctly
156 - */
157 - public function match( $val ){
158 - global $wgUser;
159 - if( $this->type == self::ANONYMOUS && $wgUser->isAnon() ){
160 - return $val === self::TOKEN_SUFFIX;
161 - }
162 -
163 - if( $this->get() === null ){
164 - return null;
165 - }
166 -
167 - return md5( $this->get() . $this->salt ) . self::TOKEN_SUFFIX === $val;
168 - }
169 -
170 - /**
171 - * Delete the token after use, so it can't be used again. This will
172 - * invalidate all tokens for this Token's action type.
173 - */
174 - public function clear(){
175 - $this->store( null );
176 - }
177 -
178 - /**
179 - * Prepare a new Token for a given action, set it in session, and
180 - * return the value we need to pass in the HTML
181 - * @param $salt String
182 - * @param $type Token class constant identifier
183 - * @return String token string to store in HTML
184 - */
185 - public static function prepare( $salt, $type = self::PERSISTENT ){
186 - $t = new Token( $salt, $type );
187 - return $t->set( false );
188 - }
189 -
190 - /**
191 - * Generate a random token
192 - * @param $salt String Optional salt value
193 - * @return String 32-char random token
194 - */
195 - protected static function generate( $salt = '' ) {
196 - $rand = dechex( mt_rand() ) . dechex( mt_rand() );
197 - return md5( $rand . $salt );
198 - }
199 -
200 - /**
201 - * Set the given token for the given action in the session
202 - * @param $token String
203 - * @param $action String
204 - */
205 - protected function store( $token ){
206 - $this->request->setSessionData( "ws{$this->type}Token", $token );
207 - }
208 -
209 - /**
210 - * Get the token set for a given action
211 - * @return String or null if no token was stored in the session
212 - */
213 - protected function get(){
214 - return $this->request->getSessionData( "ws{$this->type}Token" );
215 - }
216 -}
Index: trunk/phase3/includes/AutoLoader.php
@@ -247,7 +247,6 @@
248248 'TitleArray' => 'includes/TitleArray.php',
249249 'TitleArrayFromResult' => 'includes/TitleArray.php',
250250 'TitleListDependency' => 'includes/CacheDependency.php',
251 - 'Token' => 'includes/Token.php',
252251 'UnlistedSpecialPage' => 'includes/SpecialPage.php',
253252 'UppercaseCollation' => 'includes/Collation.php',
254253 'User' => 'includes/User.php',

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r78585Merge in Token class from my branch; would like some second opinions on the c...happy-melon23:07, 18 December 2010
r78586Lol, I don't think that's part of the standard GPL header... :-D (follow-up r...happy-melon23:09, 18 December 2010
r78599Follow-up r78585: Make Token::PERSISTENT the default, so no need to specify i...happy-melon15:23, 19 December 2010
r78631Revert rollback implementation of r78585, r78599. The way the API is set up,...happy-melon19:03, 20 December 2010

Status & tagging log