| DetailsAffected Software:Login LockDown for WordPress Fixed in Version: 1.5 Issue Type:SQL Injection Original Code: Found Here DescriptionThis week’s code sample comes from the “Login Lockdown” plug-in for WordPress. It’s always interesting when “security” software ends up having serious security flaws…. This patch contained several bug fixes. The first bug fix we see in the patch is the inclusion of nonce checking to prevent CSRF. It’s difficult to detect CSRF vulnerabilities by looking at individual function logic and it’s ok if the reader missed these bugs. CSRF token validation should be done at the framework level and including CSRF nonce validation in the logic of every function can quickly become unwieldy. If an application wide CSRF solution cannot be implemented at the framework level,then auditing for CSRF must be done at a function by function level. Personally,I prefer to check for CSRF vulnerabilities by identifying any function that performs a Create,Update,or Delete operation,mapping those functions back to the HTML markup and checking the markup to see if a nonce is passed as part of the POST or GET request. This is of course is done after an extensive audit of the nonce validation code. The bugs that should have been spotted by the spotthevuln reader are the SQL injection and the XSS vulnerabilities in the code. The SQL Injection is pretty straight forward. The “releaseme” POST parameter is taken and is eventually passed directly to a dynamically built SQL statement without any sanitization. The developers fixed the vulnerability by utilizing the WordPress escape logic. Finally,the last line of the code snippet actually contained an XSS vulnerability,echoing a $_SERVER variable without any sanitization. Developers Solution1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
| function print_loginlockdownAdminPage() {
global $wpdb;
$table_name = $wpdb->prefix . "lockdowns";
$loginlockdownAdminOptions = get_loginlockdownOptions();
if (isset($_POST['update_loginlockdownSettings'])) {
+ //wp_nonce check
+ check_admin_referer('login-lockdown_update-options');
if (isset($_POST['ll_max_login_retries'])) {
$loginlockdownAdminOptions['max_login_retries'] = $_POST['ll_max_login_retries'];
}
if (isset($_POST['ll_retries_within'])) {
$loginlockdownAdminOptions['retries_within'] = $_POST['ll_retries_within'];
}
if (isset($_POST['ll_lockout_length'])) {
$loginlockdownAdminOptions['lockout_length'] = $_POST['ll_lockout_length'];
}
if (isset($_POST['ll_lockout_invalid_usernames'])) {
$loginlockdownAdminOptions['lockout_invalid_usernames'] = $_POST['ll_lockout_invalid_usernames'];
}
if (isset($_POST['ll_mask_login_errors'])) {
$loginlockdownAdminOptions['mask_login_errors'] = $_POST['ll_mask_login_errors'];
}
update_option("loginlockdownAdminOptions",$loginlockdownAdminOptions);
?>
<div><p><strong><?php _e("Settings Updated.","loginlockdown");?></strong></p></div>
<?php
}
if (isset($_POST['release_lockdowns'])) {
+ //wp_nonce check
+ check_admin_referer('login-lockdown_release-lockdowns');
if (isset($_POST['releaseme'])) {
$released = $_POST['releaseme'];
foreach ( $released as $release_id ) {
$results = $wpdb->query("UPDATE $table_name SET release_date = now() ".
- "WHERE lockdown_ID = $release_id");
+ "WHERE lockdown_ID = ". $wpdb->escape($release_id) . "");
}
}
update_option("loginlockdownAdminOptions",$loginlockdownAdminOptions);
?>
<div><p><strong><?php _e("Lockdowns Released.","loginlockdown");?></strong></p></div>
<?php
}
$dalist = listLockedDown();
?>
<div>
-<form method="post"action="<?php echo $_SERVER["REQUEST_URI"];?>">
+<form method="post"action="<?php echo esc_attr($_SERVER["REQUEST_URI"]);?>"> |
DetailsAffected Software: Short URL Plugin Fixed in Version: 2.0 Issue Type:SQL Injection Original Code: Found Here DescriptionThis was a vulnerability that affected the Short URL WordPress plugin. The vulnerability is very straightforward and should have been easily detected by a security code reviewer. The vulnerable code section takes attacker controlled data directly from $_POST[‘form_url’],$_POST[‘form_desc’],and $_POST[‘id’] and uses the tainted value immediately in dynamically built SQL statements. One interesting piece of this particular code fix is that the developers chose to implement the code fixes near the assignment of the variable (as opposed to near consumption,in the SQL statement). Another interesting piece of the code fix is the logic for the following conditional: if($action == “delete”){
looks like the devs may have forgotten something  Developers Solution1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
| <?phprequire_once (ABSPATH . 'wp-admin/upgrade-functions.php' ); dbDelta ($sql ); } if (isset ($_POST ['action' ])) { $action = $_POST ['action' ]; if ($action == "create" ){- $add_url = $_POST['form_url'];- $add_desc = $_POST['form_desc'];+ $add_url = $wpdb->escape($_POST['form_url']);+ $add_desc = $wpdb->escape($_POST['form_desc']); if ($add_url == "http://"|| (!$add_url )){ $ERR = $ERR . "<br>You must enter a URL to redirect to!"; } if (!$ERR ){ $wpdb->query ("INSERT INTO $table_name (link_url,link_desc ) VALUES ('$add_url','$add_desc' )" ); $new_url = get_option ("siteurl" ) . "/u/". mysql_insert_id (); $MES = $MES . "<br>The redirect URL has been added. Your new Short URL is:". $new_url; } }if ($action == "edit" ){- $edit_id = $_POST['id'];- $edit_url = $_POST['form_url'];- $edit_desc = $_POST['form_desc'];+ $edit_id = $wpdb->escape($_POST['id']);+ $edit_url = $wpdb->escape($_POST['form_url']);+ $edit_desc = $wpdb->escape($_POST['form_desc']); if ($edit_url == "http://"|| (!$edit_url )){ $ERR = $ERR . "<br>You must enter a URL to redirect to!"; } if (!$ERR ){ $wpdb->query ("UPDATE $table_name SET link_url='$edit_url',link_desc='$edit_desc' WHERE link_id = $edit_id" ); $MES = $MES . "<br>The redirect URL has been modified."; } } if ($action == "delete" ){ $delete_id = $_POST ['id' ]; $wpdb->query ("DELETE FROM $table_name WHERE link_id = '$delete_id'" ); $MES = $MES . "<br>Redirect deleted!"; } if ($action == "clearall" ){ $wpdb->query ("UPDATE $table_name SET link_count=' 0' WHERE link_count > 0" ); $MES = $MES . "<br>Counts have been reset!"; }} ?> <div> <form method="post"> <h2>Short URL Admin</h2> <?php if($ERR){ echo "<p>". $ERR . "</p>";}if ($MES ){ echo "<p>". $MES . "</p>"; } ?> <p>Short URL allows you to create shorter URL's and keeps track of how many times a link has been clicked. It's useful for managing downloads,keeping track of outbound links and for masking URL's. Clicking the Clear All Clicks button will reset the count for each entry. Visit the <a href="<a href="http://www.harleyquine.com/php-scripts/short-url-plugin/%22%3Eplugin">http://www.harleyquine.com/php-scripts/short-url-plugin/">plugin</a>page</a>for more information about this plugin.</p> <h2>Current Redirects</h2><table> <thead> <tr> <th scope="col">Short URL (The URL to use )</th> <th scope="col">Real URL (Where it redirects to )</th> <th scope="col">Notes</th> <th scope="col">Amount of Clicks</th> <th scope="col">Manage</th> </tr> </thead> <tbody id="the-list"> ?> |
DetailsAffected Software:WordPress Fixed in Version:2.1 Issue Type:Defense in Depth Original Code: Found Here DescriptionI found this issue interesting for a couple reasons. Upon first glance,the patch appears to be a defense against SQL Injection and in essence,it is. It seems that the $q[‘cat’] value is controlled by the user and is eventually used to help build a SQL statement. Before the $q[‘cat’] value is used in a SQL statement,it is actually sanitized by the following lines: $q['cat'] = ”.urldecode($q['cat']).”; $q['cat'] = addslashes_gpc($q['cat']);
Once the value is sanitized,it is used to build various SQL statements. Now this particular patch was developed by the WordPress team because they discovered that when a user/attacker passes a “.” (period character) to $q[‘cat’],it would cause a SQL error which would be displayed to the user. While a single period character doesn’t give the attacker the ability to execute arbitrary SQL,it does give the attacker an information disclosure bug. In an academic sense however,the attacker has convinced the database that their provided value should be interpreted as code as opposed to data (ala SQL Injection). The reason the period character slips through is because it is not defined as a special character in the addslashes() php function… this could be useful in other situations. The WordPress prevented the information leak by checking to see if $q[‘cat’] is an integer value. The patch here is a single line fix. Developers Solution1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
| // Category stuff
if ((empty($q['cat'])) || ($q['cat'] == '0') ||
// Bypass cat checks if fetching specific posts
( $this->is_single || $this->is_page )) {
$whichcat='';
} else {
$q['cat'] = ''.urldecode($q['cat']).'';
$q['cat'] = addslashes_gpc($q['cat']);
$join = "LEFT JOIN $wpdb->post2cat ON ($wpdb->posts.ID = $wpdb->post2cat.post_id) ";
$cat_array = preg_split('/[,\s]+/',$q['cat']);
$in_cats = $out_cats = '';
foreach ( $cat_array as $cat ) {
+ $cat = intval($cat);
$in = strstr($cat,'-') ? false:true;
$cat = trim($cat,'-');
if ( $in )
$in_cats .= "$cat,". get_category_children($cat,'',',');
else
$out_cats .= "$cat,". get_category_children($cat,'',',');
}
$in_cats = substr($in_cats,0,-2);
$out_cats = substr($out_cats,0,-2);
if ( strlen($in_cats) >0 )
$in_cats = "AND category_id IN ($in_cats)";
if ( strlen($out_cats) >0 )
$out_cats = "AND category_id NOT IN ($out_cats)";
$whichcat = $in_cats . $out_cats;
$distinct = 'DISTINCT';
}
// Category stuff for nice URIs
global $cache_categories;
if ('' != $q['category_name']) { |
|
