Skip to content

Commit 9981848

Browse files
committed
security: CSV Formula Injection
This addresses a security issue discovered by Aishwarya Iyer where a User can change their Full Name to a windows formula and when an Agent exports a list of Users containing said User and opens the export file, the formula will be executed on their computer (if it's windows of course). This adds a new validator called `is_formula()` to all text fields disallowing the use of the following characters `= + - @` at the beginning of text. This should mitigate CSV Formula injections for any text field that allows user-input in the system. To further prevent CSV Formula injections this adds an escape mechanism to the Exporter that will escape any content matching the formula regex with a single quote (as mentioned in many posts about this subject).
1 parent bbfff1a commit 9981848

File tree

4 files changed

+38
-3
lines changed

4 files changed

+38
-3
lines changed

Diff for: include/class.export.php

+7-1
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,13 @@ function dump() {
356356
fputs($this->output, chr(0xEF) . chr(0xBB) . chr(0xBF));
357357
fputcsv($this->output, $this->getHeaders(), $delimiter);
358358
while ($row=$this->next())
359-
fputcsv($this->output, $row, $delimiter);
359+
fputcsv($this->output, array_map(
360+
function($v){
361+
if (preg_match('/^[=\-+@].*/', $v))
362+
return "'".$v;
363+
return $v;
364+
}, $row),
365+
$delimiter);
360366

361367
fclose($this->output);
362368
}

Diff for: include/class.forms.php

+25-1
Original file line numberDiff line numberDiff line change
@@ -1300,7 +1300,8 @@ function validateEntry($value) {
13001300
parent::validateEntry($value);
13011301
$config = $this->getConfiguration();
13021302
$validators = array(
1303-
'' => null,
1303+
'' => array(array('Validator', 'is_formula'),
1304+
__('Content cannot start with the following characters: = - + @')),
13041305
'email' => array(array('Validator', 'is_valid_email'),
13051306
__('Enter a valid email address')),
13061307
'phone' => array(array('Validator', 'is_phone'),
@@ -1379,6 +1380,29 @@ function getConfigurationOptions() {
13791380
);
13801381
}
13811382

1383+
function validateEntry($value) {
1384+
parent::validateEntry($value);
1385+
if (!$value)
1386+
return;
1387+
$config = $this->getConfiguration();
1388+
$validators = array(
1389+
'' => array(array('Validator', 'is_formula'),
1390+
__('Content cannot start with the following characters: = - + @')),
1391+
);
1392+
// Support configuration forms, as well as GUI-based form fields
1393+
if (!($valid = $this->get('validator')) && isset($config['validator']))
1394+
$valid = $config['validator'];
1395+
if (!isset($validators[$valid]))
1396+
return;
1397+
$func = $validators[$valid];
1398+
$error = $func[1];
1399+
if ($config['validator-error'])
1400+
$error = $this->getLocal('validator-error', $config['validator-error']);
1401+
if (is_array($func) && is_callable($func[0]))
1402+
if (!call_user_func($func[0], $value))
1403+
$this->_errors[] = $error;
1404+
}
1405+
13821406
function hasSpecialSearch() {
13831407
return false;
13841408
}

Diff for: include/class.user.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ static function fromForm($form, $create=true) {
248248
//Validate the form
249249
$valid = true;
250250
$filter = function($f) use ($thisstaff) {
251-
return !isset($thisstaff) || $f->isRequiredForStaff();
251+
return !isset($thisstaff) || $f->isRequiredForStaff() || $f->isVisibleToStaff();
252252
};
253253
if (!$form->isValid($filter))
254254
$valid = false;

Diff for: include/class.validator.php

+5
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,11 @@ static function is_username($username, &$error='') {
204204
return $error == '';
205205
}
206206

207+
static function is_formula($text, &$error='') {
208+
if (!preg_match('/^[^=\+@-].*$/', $text))
209+
$error = __('Content cannot start with the following characters: = - + @');
210+
return $error == '';
211+
}
207212

208213
/*
209214
* check_ip

0 commit comments

Comments
 (0)