Skip to content

Commit

Permalink
Fix XSS vulnerability in typeahead-enabled text entry fields
Browse files Browse the repository at this point in the history
This commit makes several changes in various places to prevent HTML tags from
being interpreted in typeahead text fields.  The two places in the tool that use
such fields are the search-by-username field in User Management, as well as the
search-by-username field in Logs.  However, since the latter only included
usernames of accounts that had already been approved to use the tool, it was a
much less severe vector than the user management one.

Note that this commit makes changes to the Bootstrap JavaScript code - I made
the necessary changes in the lib/bootstrap/js/bootstrap.js file, tested there,
then re-minified the file (including my changes) to lib/bootstrap/js/
bootstrap.min.js - this minified file remains the version served by the tool to
clients, but of course you can examine the changes I made in the original
bootstrap.js file as well.

The change in bootstrap.js is actually fairly minor - it only adds a little bit
of extra processing to the typeahead system to turn HTML entities back into
their appropriate characters when selecting an option with HTML entities from
the typeahead dropdown.

Primarily, patching this hole involved moving the source data typeahead was
using out of inline HTML and into a <script> block in footer.tpl, which
necessitated some minor modifications to \BootstrapSkin::displayInternalFooter()
to allow passing this data to footer.tpl.  Furthermore, a new function (in
functions.php), getTypeaheadSource(), generates the appropriate JavaScript
source for the typeahead data from an array of usernames as strings - the output
of this function is what is intended to be passed to displayInternalFooter().

One major caveat to this fix as is: If a name contains characters that are
converted to HTML entities - let's say, "<b>Test</b>", then typing "<" or such
will not cause typeahead to complete the rest of the name, since it only sees
the name with those characters converted to HTML entities.  That being said,
typing "Test" *would* still complete to "<b>Test</b>", and when you select
"<b>Test</b>" from the typeahead completion dropdown, the HTML entities would
convert back to regular characters so the text entry field is correctly filled
with "<b>Test</b>".
  • Loading branch information
FastLizard4 authored and stwalkerster committed Jan 16, 2016
1 parent 3ad4cbb commit c34b12c
Show file tree
Hide file tree
Showing 9 changed files with 44 additions and 18 deletions.
9 changes: 4 additions & 5 deletions acc.php
Expand Up @@ -1461,18 +1461,17 @@

$smarty->assign("limit", $limit);
$smarty->assign("page", $page);

$activeUsers = User::getAllUsernames(gGetDb(), true);

$smarty->assign("jsuserlist", $activeUsers);

$smarty->assign("logs", $logs);


$smarty->assign("filterUser", $filterUser);
$smarty->assign("filterAction", $filterAction);
$smarty->display("logs/main.tpl");

$tailscript = getTypeaheadSource(User::getAllUsernames(gGetDb(), true));

BootstrapSkin::displayInternalFooter();
BootstrapSkin::displayInternalFooter($tailscript);
die();
}
elseif ($action == "reserve") {
Expand Down
21 changes: 21 additions & 0 deletions functions.php
Expand Up @@ -391,3 +391,24 @@ function reattachOAuthAccount(User $user)
throw new TransactionException($ex->getMessage(), "Connection to Wikipedia failed.", "alert-error", 0, $ex);
}
}

/**
* Generates the JavaScript source for XSS-safe typeahead autocompletion for usernames. This output is expected to be
* passed as the $tailscript argument to \BootstrapSkin::displayInternalFooter().
*
* @param $users string[] Array of usernames as strings
* @return string
*/
function getTypeaheadSource($users) {
$userList = "";
foreach ($users as $v) {
$userList .= "\"" . htmlentities($v) . "\", ";
}
$userList = "[" . rtrim($userList, ", ") . "]";
$tailscript = <<<JS
$('.username-typeahead').typeahead({
source: {$userList}
});
JS;
return $tailscript;
}
8 changes: 7 additions & 1 deletion includes/BootstrapSkin.php
Expand Up @@ -73,10 +73,14 @@ public static function displayPublicFooter()
$smarty->display("footer.tpl");
}


/**
* Prints the internal interface footer to the screen.
*
* @param string|null $tailscript JavaScript to append to the page, usually so it can call jQuery
* @throws Exception
*/
public static function displayInternalFooter()
public static function displayInternalFooter($tailscript = null)
{
global $smarty;

Expand Down Expand Up @@ -128,6 +132,8 @@ function($arg)
$smarty->assign("onlineusers", $emptystring);
}

$smarty->assign("tailscript", $tailscript);

$smarty->display("footer.tpl");
}

Expand Down
2 changes: 1 addition & 1 deletion lib/bootstrap/js/bootstrap.js
Expand Up @@ -1874,7 +1874,7 @@
, select: function () {
var val = this.$menu.find('.active').attr('data-value')
this.$element
.val(this.updater(val))
.val(this.updater($('<div>').html(val).text()))
.change()
return this.hide()
}
Expand Down
2 changes: 1 addition & 1 deletion lib/bootstrap/js/bootstrap.min.js

Large diffs are not rendered by default.

9 changes: 6 additions & 3 deletions templates/footer.tpl
Expand Up @@ -26,8 +26,11 @@
$(function () {
$("[rel='popover']").popover();
});
</script>

</body>
</script>{if $tailscript}
<script type="text/javascript">
{$tailscript}
</script>
{/if}
</body>
</html>

2 changes: 1 addition & 1 deletion templates/logs/form.tpl
Expand Up @@ -2,7 +2,7 @@
<form class="form-inline" method="get" action="acc.php">
<input type="hidden" name="action" value="logs" />
<input type="hidden" name="page" value="1" />
<input type="text" id="inputUsername" placeholder="All users" {include file="usermanagement/jsuserlist.tpl"} name="filterUser" value="{$filterUser|escape}"/>
<input type="text" id="inputUsername" class="username-typeahead" placeholder="All users" data-provide="typeahead" data-items="10" name="filterUser" value="{$filterUser|escape}"/>
<!--<input type="text" id="inputAction" placeholder="Filter by Action" name="filterAction" value="{$filterAction|escape}" />-->
<select id="inputAction" name="filterAction">
<option value="">All log actions</option>
Expand Down
1 change: 0 additions & 1 deletion templates/usermanagement/jsuserlist.tpl

This file was deleted.

8 changes: 3 additions & 5 deletions users.php
Expand Up @@ -490,14 +490,12 @@
false);

// assign to user
$userListData = User::getAllUsernames(gGetDb());
$smarty->assign("jsuserlist", $userListData);
$smartydatalist = $smarty->fetch("usermanagement/jsuserlist.tpl");
$tailscript = getTypeaheadSource(User::getAllUsernames(gGetDb()));

echo <<<HTML
<div class="row-fluid">
<form class="form-search">
<input type="text" class="input-large" placeholder="Jump to user" $smartydatalist name="usersearch">
<input type="text" class="input-large username-typeahead" placeholder="Jump to user" data-provide="typeahead" data-items="10" name="usersearch">
<button type="submit" class="btn">Search</button>
</form>
</div>
Expand Down Expand Up @@ -631,5 +629,5 @@ function showUserList($data, $level)
echo "</div></div></div>";
}

BootstrapSkin::displayInternalFooter();
BootstrapSkin::displayInternalFooter($tailscript);
die();

0 comments on commit c34b12c

Please sign in to comment.