Safe MYSql Queries

Darrell3831 · Worker Joined: Feb 18, 2004 Posts: 244

In another thread about input validators,

Only registered users can see links on this board!
Get registered or login to the forums!

, we were talking about how some people can use or 'hijack' a poorly written SQL query to extract infromation from your database.

Could someone please show me the difference between a poorly written SQL query and a well written one?

I'm writing the administrative side of a module that does a lot of queries. I'd like to learn this and fix things now rather than after I've been hacked.

This is probably the best function that I have to show you how I am doing my queries. I started with php Dec. 26 of last year. I'm only at it for two months. I have close to a month tied up in this project already and I'd like to be sure I'm on the right track.

Code:

/* Delete a course */
function deleteCourse($cid, $title, $ok=0) {
global $prefix, $db;

if ($ok==1) {
 // Find out how many student enrollments this course has
 $enrolled = $db->sql_query("select eid from ".$prefix."_university_enrollment WHERE cid='$cid'");
 while (list($eid) = $db->sql_fetchrow($enrolled)) {
 // Now delete the enrollment(s)
 $db->sql_query("delete from ".$prefix."_university_enrollment WHERE eid='$eid'");
 }
 // Find out what students just became orphened
 $orphaned1 = $db->sql_query("SELECT s.* FROM ".$prefix."_university_students s LEFT JOIN ".$prefix."_university_enrollment e ON s.sid=e.sid WHERE e.sid IS NULL");
 while (list($sid) = $db->sql_fetchrow($orphaned1)) {
 // Since they arent enrolled in anything make them inactive. This helps admins fine them.
$db->sql_query("update ".$prefix."_university_students set status='0' WHERE sid='$sid'");
 }
 //Find out how many faculty members have rights to administer this course
 $rights = $db->sql_query("select rid from ".$prefix."_university_rights WHERE cid='$cid'");
 while (list($rid) = $db->sql_fetchrow($rights)) {
 // Now delete their access rights
 $db->sql_query("delete from ".$prefix."_university_rights WHERE rid='$rid'");
 }
 // Find out how many faculty members just became orphened
 $orphaned2 = $db->sql_query("SELECT f.* FROM ".$prefix."_university_faculty f LEFT JOIN ".$prefix."_university_rights r ON f.fid=r.fid WHERE r.fid IS NULL");
 while (list($temp_fid) = $db->sql_fetchrow($orphaned2)) {
 // Since they are not administering anything make them inactive. This helps admins find them.
 $db->sql_query("update ".$prefix."_university_faculty set status='0' WHERE fid='$temp_fid'");
 }
 // Find out how many lessons this course has
 $lessons = $db->sql_query("select lid from ".$prefix."_university_lessons WHERE cid='$cid'");
 while (list($lid) = $db->sql_fetchrow($lessons)) {
 // Find out how many questions this lesson has
 $questions = $db->sql_query("select qid from ".$prefix."_university_quizzes WHERE lid='$lid'");
 while (list($qid) = $db->sql_fetchrow($questions)) {
 // Now delete the question(s)
 $db->sql_query("delete from ".$prefix."_university_quizzes WHERE qid='$qid'");
 }
 // Now delete the lesson(s)
 $db->sql_query("delete from ".$prefix."_university_lessons where lid='$lid'");
 }
 //Now delete the course
 $db->sql_query("delete from ".$prefix."_university_courses where cid='$cid'");

 header("Location: admin.php?op=listCourses");
} else {
 include("header.php");
 universityMenu();
 OpenTable();
 echo "<CENTER>"._UNIVERSITY_ACTION_PERMENANT."</CENTER>"
 ."<center>"._COURSE_DELETE." $title "
.""._COURSE_DELETE_WARNING." "
."[ <a href=\"admin.php?op=listCourses\">"._UNIVERSITY_NO."</a> | <a href=\"admin.php?op=deleteCourse&cid=$cid&ok=1\">"._UNIVERSITY_YES."</a> ]</center>";
 CloseTable();
 displayCopyright();
include("footer.php");
}
}

Suggestions and comments are appreciated.

Thanks,
Darrell

Darrell3831 · Worker Joined: Feb 18, 2004 Posts: 244

I should mention that this function isent finished yet. I'm still working on the whole administrative side of the module.

None of the input validators are written. Also, only admins can get to this part of the site and they will not be able to access all functions. I will be adding tests to each function bassed on access rights retrieved from the faculty database.

I might be employing this static variable talked about somewhere else since I will be making so many calls to that function, whatever I call it...

if(hasPermission(current_user))

Thanks,
Darrell

chatserv · Posted: Sun Feb 22, 2004 11:50 am

It looks good, the only thing i would add is variable validation:

Code:

/* Delete a course */
function deleteCourse($cid, $title, $ok=0) {
global $prefix, $db;

if ($ok==1) {
// Find out how many student enrollments this course has
$enrolled = $db->sql_query("select eid from ".$prefix."_university_enrollment WHERE cid='$cid'");
while (list($eid) = $db->sql_fetchrow($enrolled)) {
$eid = intval($eid);
// Now delete the enrollment(s)
$db->sql_query("delete from ".$prefix."_university_enrollment WHERE eid='$eid'");
}
// Find out what students just became orphened
$orphaned1 = $db->sql_query("SELECT s.* FROM ".$prefix."_university_students s LEFT JOIN ".$prefix."_university_enrollment e ON s.sid=e.sid WHERE e.sid IS NULL");
while (list($sid) = $db->sql_fetchrow($orphaned1)) {
$sid = intval($sid);
// Since they arent enrolled in anything make them inactive. This helps admins fine them.
$db->sql_query("update ".$prefix."_university_students set status='0' WHERE sid='$sid'");
}
//Find out how many faculty members have rights to administer this course
$rights = $db->sql_query("select rid from ".$prefix."_university_rights WHERE cid='$cid'");
while (list($rid) = $db->sql_fetchrow($rights)) {
$rid = intval($rid);
// Now delete their access rights
$db->sql_query("delete from ".$prefix."_university_rights WHERE rid='$rid'");
}
// Find out how many faculty members just became orphened
$orphaned2 = $db->sql_query("SELECT f.* FROM ".$prefix."_university_faculty f LEFT JOIN ".$prefix."_university_rights r ON f.fid=r.fid WHERE r.fid IS NULL");
while (list($temp_fid) = $db->sql_fetchrow($orphaned2)) {
$temp_fid = intval($temp_fid);
// Since they are not administering anything make them inactive. This helps admins find them.
$db->sql_query("update ".$prefix."_university_faculty set status='0' WHERE fid='$temp_fid'");
}
// Find out how many lessons this course has
$lessons = $db->sql_query("select lid from ".$prefix."_university_lessons WHERE cid='$cid'");
while (list($lid) = $db->sql_fetchrow($lessons)) {
$lid = intval($lid);
// Find out how many questions this lesson has
$questions = $db->sql_query("select qid from ".$prefix."_university_quizzes WHERE lid='$lid'");
while (list($qid) = $db->sql_fetchrow($questions)) {
$qid = intval($qid);
// Now delete the question(s)
$db->sql_query("delete from ".$prefix."_university_quizzes WHERE qid='$qid'");
}
// Now delete the lesson(s)
$db->sql_query("delete from ".$prefix."_university_lessons where lid='$lid'");
}
//Now delete the course
$db->sql_query("delete from ".$prefix."_university_courses where cid='$cid'");

header("Location: admin.php?op=listCourses");
} else {
include("header.php");
universityMenu();
OpenTable();
echo "<CENTER>"._UNIVERSITY_ACTION_PERMENANT."</CENTER>"
."<center>"._COURSE_DELETE." $title "
.""._COURSE_DELETE_WARNING." "
."[ <a href=\"admin.php?op=listCourses\">"._UNIVERSITY_NO."</a> | <a href=\"admin.php?op=deleteCourse&cid=$cid&ok=1\">"._UNIVERSITY_YES."</a> ]</center>";
CloseTable();
displayCopyright();
include("footer.php");
}
}

Darrell3831 · Worker Joined: Feb 18, 2004 Posts: 244

Thank you!

I understand what you mean about intval(). I'm glad to hear that I'm not writing the queries in an unsafe manner. Being mostly self taught is rough. Very Happy

I'm happy for your suggestions.

Eventually, at the top of that function will be the test to see if the user is even allowed to be using it or not.