Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ehrlich sql fixes #11

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Ehrlich sql fixes #11

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 21, 2018

First of code security fixes. 90% of the code/files in body_composition and clinical_notes folder are fixed. All of it should in theory work, but likely will fail an integration test or two. Wanted to make sure I got the git workflow down though before proceeding further.


sqlStatement($query, array(rbvalue('form_body_type'), form2db($_POST['form_height']), form2db($_POST['form_weight']), form2db($_POST['form_bmi']),
form2db($_POST['form_bmr']), form2db($_POST['form_impedance']), form2db($_POST['form_fat_pct']), form2db($_POST['form_fat_mass']), form2db($_POST['form_ffm']),
form2db($_POST['form_tbw']), form2db($_POST['form_other']), $formid ));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change all the form2db() calls to trim()
And remove the form2db() function (note this function is using a legacy method that openemr used to use to prevent sql injection.


$newid = sqlInsert($query, array(rbvalue('form_body_type'), form2db($_POST['form_height']), form2db($_POST['form_weight']), form2db($_POST['form_bmi']),
form2db($_POST['form_bmr']), form2db($_POST['form_impedance']), form2db($_POST['form_fat_pct']), form2db($_POST['form_fat_mass']),
form2db($_POST['form_ffm']), form2db($_POST['form_tbw']), form2db($_POST['form_other'])));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change all the form2db() calls to trim()
And remove the form2db() function (note this function is using a legacy method that openemr used to use to prevent sql injection.

body_type = ?, height = ?, weight = ?, bmi = ?, bmr = ?, impedance = ?,
fat_pct = ?, fat_mass = ?, ffm = ?, tbw = ?, other = ? WHERE id = ?";

sqlStatement($query, array(rbvalue('form_body_type'), form2db($_POST['form_height']), form2db($_POST['form_weight']), form2db($_POST['form_bmi']),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets fixup the rbvalue function to work with binding. In that function:
Change:

return "'$tmp'";

to:

return "$tmp";

(note I just removed the single quotes)

@@ -135,13 +120,13 @@ function rbinput($name, $value, $desc, $colname)
<html>
<head>
<?php html_header_show();?>
<link rel=stylesheet href="<?php echo $css_header;?>" type="text/css">
<link rel=stylesheet href="<?php echo attr($css_header);?>" type="text/css">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't html escape full paths (sometimes weird things can happen). So, revert above line change.

<script language="JavaScript">
</script>
</head>

<body <?php echo $top_bg_line;?> topmargin="0" rightmargin="0" leftmargin="2" bottommargin="0" marginwidth="2" marginheight="0">
<form method="post" action="<?php echo $rootdir ?>/forms/body_composition/new.php?id=<?php echo $formid ?>"
<form method="post" action="<?php echo att($rootdir) ?>/forms/body_composition/new.php?id=<?php echo attr($formid) ?>"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't html escape full paths (sometimes weird things can happen). Leave $rootdir alone, but the attr() around $formid is correct.

<?php echo rbinput('form_body_type', 'Standard', 'Standard', 'body_type') ?>&nbsp;
<?php echo rbinput('form_body_type', 'Athletic', 'Athletic', 'body_type') ?>&nbsp;
<?php echo text('form_body_type', 'Standard', 'Standard', 'body_type') ?>&nbsp;
<?php echo text('form_body_type', 'Athletic', 'Athletic', 'body_type') ?>&nbsp;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note rbinput is a function, so revert changes in above 2 lines and change the rbinput function to:

function rbinput($name, $value, $desc, $colname)
{
    global $row;
    $ret  = "<input type='radio' name='" . attr($name) . "' value='" . attr($value) . "'";
    if ($row[$colname] == $value) {
        $ret .= " checked";
    }
    $ret .= " />" . text($desc);
    return $ret;
}

</td>
<td align='center' nowrap>
<?php
if ($scale_file_age >= 0) {
echo "<font color='blue'>This reading was taken $scale_file_age minutes ago.</font>\n";
echo "<font color='blue'>This reading was taken" . text($scale_file_age) . "minutes ago.</font>\n";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just missing a space after taken and before minutes:

echo "<font color='blue'>This reading was taken " . text($scale_file_age) . " minutes ago.</font>\n";

// destination = ? rbvalue('destination')
WHERE id = ?";

sqlStatement($query, array($_POST['form_history'], $_POST['form_examination'], $_POST['form_plan'], rbvalue('fu_required'), $fu_timing, $formid));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets fixup the rbvalue function to work with binding. In that function:
Change:

return "'$tmp'";

to:

return "$tmp";

(note I just removed the single quotes)

@@ -137,7 +130,7 @@ function cbcell($name, $desc, $colname)

<body <?php echo $top_bg_line;?> topmargin="0" rightmargin="0" leftmargin="2"
bottommargin="0" marginwidth="2" marginheight="0">
<form method="post" action="<?php echo $rootdir ?>/forms/clinical_notes/new.php?id=<?php echo $formid ?>"
<form method="post" action="<?php echo attr($rootdir) ?>/forms/clinical_notes/new.php?id=<?php echo attr($formid) ?>"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't html escape full paths (sometimes weird things can happen). Leave $rootdir alone, but the attr() around $formid is correct.

@@ -18,9 +18,9 @@
// as published by the Free Software Foundation; either version 2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this script, fixup the rbinput function:

function rbinput($name, $value, $desc, $colname)
{
    global $row;
    $ret  = "<input type='radio' name='" . attr($name) . "' value='" . attr($value) . "'";
    if ($row[$colname] == $value) {
        $ret .= " checked";
    }
    $ret .= " />" . text($desc);
    return $ret;
}

@bradymiller
Copy link
Owner

bradymiller commented Aug 24, 2018

hi @danora24 ,
Reviewed the code. Very nice work for your first security hardening script (this one had a couple odd things going on that made it a bit more complicated). After you address my issues, it will be ready for the codebase.
thanks!
-brady

@@ -181,7 +172,7 @@ function cbcell($name, $desc, $colname)
<td nowrap>
<input type='text' name='fu_timing' size='10' style='width:100%'
title='When to follow up'
value='<?php echo addslashes($row['followup_timing']) ?>' />
value='<?php echo text($row['followup_timing']) ?>' />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use attr() here since it is within an html attribute

formHeader("Form: contacts");
?>
<html><head>
<?php html_header_show();?>
<link rel=stylesheet href="<?php echo $css_header;?>" type="text/css">
</head>
<body <?php echo $top_bg_line;?> topmargin=0 rightmargin=0 leftmargin=2 bottommargin=0 marginwidth=2 marginheight=0>
<body <?php echo attr($top_bg_line);?> topmargin=0 rightmargin=0 leftmargin=2 bottommargin=0 marginwidth=2 marginheight=0>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't escape this. this is html that is set in interface/globals.php script.

$results = sqlQ($sql);
$sql = "select * from codes where code = ? ORDER BY id";
$results = sqlQ($sql,array(add_escape_custom($_POST['cpt_code'])));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the add_escape_custom() wrapper.
(note this is a function that will sql escape variables; but binding is better)

$sql = "SELECT name from form_evaluation_checks where foreign_id = '" . add_escape_custom($this->id) . "'";
$results = sqlQ($sql);
$sql = "SELECT name from form_evaluation_checks where foreign_id = ?";
$results = sqlQ($sql, array(add_escape_custom($this->id)));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the add_escape_custom() wrapper.
(note this is a function that will sql escape variables; but binding is better)

$sql = "INSERT INTO form_evaluation_checks set foreign_id='" . add_escape_custom($this->id) . "', name = '" . add_escape_custom($check) . "'";
sqlQuery($sql);
$sql = "INSERT INTO form_evaluation_checks set foreign_id= ?, name = ?";
sqlQuery($sql, array(add_escape_custom($this->id), add_escape_custom($check)));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the add_escape_custom() wrapper.
(note this is a function that will sql escape variables; but binding is better)

$sql = "SELECT name from form_evaluation_checks where foreign_id = '" . add_escape_custom($id) . "'";
$results = sqlQ($sql);
$sql = "SELECT name from form_evaluation_checks WHERE foreign_id = ?";
$results = sqlQ($sql, array(add_escape_custom($id)));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the add_escape_custom() wrapper.
(note this is a function that will sql escape variables; but binding is better)

$data2 = array();
while ($row = sqlFetchArray($results)) {
$data2[] = $row['name'];
$data2[] = text($row['name']);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally better to only html escape stuff when it it outputted to avoid mangling variables and double escaping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant