התגוננות מהזרקת SQL

ארכיטקט

שלום לכולם בעקבות המאמר של מייקרוסופט עשיתי מתודה פשוטה שמטהרת נתוני קלט משתמש, אם יש למישהו רעיונות לשיפור/הוספה יתקבל בברכה.

/// <summary>
            /// מטהר נתוני קלט מהמשתמש על מנת למנוע הזרקת SQL מסיר מילים שמורות ותווים מיוחדים
            /// </summary>
            /// <param name="input">הכנס כאן את נתוני הקלט מהמשתמש</param>
            internal static void SanitizeInputData(ref string input)
            {
                //תווים מפסיקים ומפרידים
                input = input.Replace("'", "");
                input = input.Replace("--", "");
                input = input.Replace(";", "");
                input = input.Replace(@"""", "");
                //אופרטורים
                input = input.Replace("or", "");
                input = input.Replace("and", "");
                input = input.Replace("=", "");
                input = input.Replace(">", "");
                input = input.Replace("<", "");
                input = input.Replace("like", "");
                //פסוקיות ומילים שמורות
                input = input.Replace("select", "");
                input = input.Replace("union", "");
                input = input.Replace("join", "");
                input = input.Replace("delete", "");
                input = input.Replace("drop", "");
                input = input.Replace("update", "");
                input = input.Replace("insert", "");
                input = input.Replace("merge", "");
                input = input.Replace("alter", "");
                input = input.Replace("execute", "");
                input = input.Replace("exec", "");
                //פונקציות מסוכנות של SQL
                input = input.Replace("sp_", "");
                //עוד משהו??? כל המוסיף מוסיפין לו מן השמים

            }

פורסם במקור בפורום CODE613 ב12/06/2015 17:23 (+03:00)

ארכיטקט

משיטוט באינטרנט הבנתי שזאת לא סוגיה פשוטה

יש כאן דוגמא למוצר בתשלום שמאתר הזרקות. זה שייך לחברה הזו

מה אתם אומרים?

פורסם במקור בפורום CODE613 ב13/06/2015 23:31 (+03:00)

softs

ארכיטקט, תן לי לחסוך לך עבודה,
יש פתרון פשוט ויעיל שעדיין לא מצאתי לו פירכא רק שהוא דורש משמעת מהמפתח,
כל פרמטר סטרינג לפני שרשור לשאילתה הרי צריך לעטוף בגרשיים אז פשוט תמיד מעבירים את המשתנה דרך פונקציה כזו,
שהיא ממילא מנטרלת כל סוג של Injection.

public static string SqlEscape(string str, bool national = true)
{
    return (national ? "N" : string.Empty) + "'" + str.Replace("'", "''") + "'";
}

פורסם במקור בפורום CODE613 ב14/06/2015 00:02 (+03:00)

dovid

למה לא להשתמש בSqlParameter?

פורסם במקור בפורום CODE613 ב14/06/2015 11:50 (+03:00)

אבי נתן

סנן גם את זה ||

פורסם במקור בפורום CODE613 ב15/06/2015 22:29 (+03:00)

ארכיטקט

@דוד ל.ט.

למה לא להשתמש בSqlParameter?

זה נכון כשאתה יודע מראש מה מצפה לך, אבל קורה לפעמים שאתה צריך לתכנן פרוייקט אתלטי עם גמישות ברמה גבוהה מאוד, שהמידע מה אתה עומד לקבל מהמשתמש הוא לא צפוי (אתה יכול לקבל ממנו שמות של טבלאות, שדות, ערכים, המספר בלתי ידוע, מספר ה Join בלתי ידוע מראש) כמובן המשתמש הפשוט לא באמת יודע שהוא שולח לך בעצם חומר נפץ שממנו אתה מרכיב SQL אבל ההאקר וודאי יידע או עלול לדעת.

התיכנון של פרוייקט כזה אם נצטרך לעשות case לכל אפשרות ואפשרות, כבר עדיף להעסיק סינים שיביאו את המידע מהדטה בייס

לכן הפתרון האידיאלי הוא להרכיב SQL מהתחלה ועד הסוף על בסיס סטרינגי, בצורה שהפרוייקט יהיה תחזיק (=בר תחזוקה) חכם, קריא, גמיש, קומפקטי, רזה, וכו.

פורסם במקור בפורום CODE613 ב16/06/2015 08:13 (+03:00)

clickone

@ארכיטקט

@דוד ל.ט.
למה לא להשתמש בSqlParameter?

זה נכון כשאתה יודע מראש מה מצפה לך, אבל קורה לפעמים שאתה צריך לתכנן פרוייקט אתלטי עם גמישות ברמה גבוהה מאוד, שהמידע מה אתה עומד לקבל מהמשתמש הוא לא צפוי (אתה יכול לקבל ממנו שמות של טבלאות, שדות, ערכים, המספר בלתי ידוע, מספר ה Join בלתי ידוע מראש) כמובן המשתמש הפשוט לא באמת יודע שהוא שולח לך בעצם חומר נפץ שממנו אתה מרכיב SQL אבל ההאקר וודאי יידע או עלול לדעת.

התיכנון של פרוייקט כזה אם נצטרך לעשות case לכל אפשרות ואפשרות, כבר עדיף להעסיק סינים שיביאו את המידע מהדטה בייס

לכן הפתרון האידיאלי הוא להרכיב SQL מהתחלה ועד הסוף על בסיס סטרינגי, בצורה שהפרוייקט יהיה תחזיק (=בר תחזוקה) חכם, קריא, גמיש, קומפקטי, רזה, וכו.

אבל בהחלט כשבונים את הסטרינג של הSQL ניתן להשתמש בSqlParameter,עם שמות רצים P1, P2 וכו'.
גם בנושא של שמות טבלאות אתה יכול לעטוף בסוגריים מרובעים, וכו'.
אני באופן אישי משתמש בWHERE בפונקצייה ש"עוטפת" את הסטרינג וככה אין לי חשש מהזרקה.

ד"א, אהבתי את המונח "תחזיק (=בר תחזוקה)" הייתי שולח אותו לאקדמיה ללשון <img src="{SMILIES_PATH}/icon_e_smile.gif" alt=":-)" title="מחייך" />

פורסם במקור בפורום CODE613 ב16/06/2015 09:51 (+03:00)

dovid

אפשר לבנות דינמי לחלוטין. בשביל הפרמטרים SqlParameter. בשביל שמות הטבלאות והעמודות וכו', אפשר פשוט לבדוק מול מילון שנטען מINFORMATION_SCHEMA. כמו"כ המתודה SqlCommandBuilder.QuoteIdentifier מבצעת ניקוי לסטרינג שמייצג זיהוי של DB כמו טבלה/עמודה.

פורסם במקור בפורום CODE613 ב16/06/2015 11:50 (+03:00)

softs

אם אתה בונה דינאמי ה SqlParameter פרמטר לא מגן עליך יותר מהשיטה שהצגתי, למעשה זה בדיוק מה שהוא עושה.
ה best practice זה להשתמש בפרוצדורות להכל ולא לבנות שום דבר דינאמי ואז כבר יש תועלת ב SqlCommand לפרמטרים שהם OUT וכיו"ב

פורסם במקור בפורום CODE613 ב19/06/2015 00:46 (+03:00)

אליעזר 0

אינני בקיא בתחום אבל בספר asp.net mvc 4 כותבים שלא כדאי לנסות לטהר קלט לבד כי בטוח נכשלים. ויש להשתמש בספריות עזר מוכנות המיועדות לכך.

פורסם במקור בפורום CODE613 ב19/06/2015 09:10 (+03:00)

softs

@אליעזר

אינני בקיא בתחום אבל בספר asp.net mvc 4 כותבים שלא כדאי לנסות לטהר קלט לבד כי בטוח נכשלים. ויש להשתמש בספריות עזר מוכנות המיועדות לכך.

כנראה שהספר מתכוון לספריות ייעודיות ולא של מייקרוסופט, כי SqlParameter של מיקרוסופט עושה בדיוק את זה - עיי"ש:

ברור שאם תנסה לטהר קלט לבד אז זה ידרוש ממך מאמצים רבים ויצירתיות וחרדות בכל שדרוג גירסה של SQL (לך תדע איזה מילה שמורה חדשה דחפו לך), ולכן צריך לחשוב על דרך איך לסתום את החור ולא איך לרדוף אחרי העכבר.

הדרך של החלפת גרשיים מאושרת ב OWASP לתחזוקת מערכות קיימות, למרות זאת היא מושמצת בקביעות ולא באופנה, שזה דבר שלא תמיד יש לו סיבה אמיתית בעולם התוכנה - לראיה - לא מצאתי, וגם המתנגדים שראיתי לא מביאים אף שיטה לעקוף את זה, כולם מביאים את אותה דוגמה מ MYSQL של הסלאש שאינה תקפה ב SQL SERVER וגם אותה אפשר למנוע בקלות.
אדרבה יביא מישהו POC של עקיפה של זה, עד אז אני ישן בשקט.

פורסם במקור בפורום CODE613 ב19/06/2015 10:10 (+03:00)

dovid

@softs

אם אתה בונה דינאמי ה SqlParameter פרמטר לא מגן עליך יותר מהשיטה שהצגתי, למעשה זה בדיוק מה שהוא עושה.

יש מעלה מאוד גדולה להשתמש בקוד של אחרים ולא לכתוב את אותו הקוד לבד.
המעלות הם חיסכון בהקף הקוד, בתחזוקה, ובקריאות - כל מתכנת שמח למצוא מתודה מוכרת מאשר מימוש פרטי של עמית.
ויש עוד סיבה, שהיא הימנעות משגיאות. להסתכל בוד המקור של מקרוסופט ולהעתיק זה בטוח מבחינת זה, אבל "לטהר קלט לבד" הכוונה לממש את זה עם אי אלו הבדלים (נניח לאפשר איזשהיא תגית נוספת או לאסור יותר), וזה מ-א-ו-ד מסוכן מבחינת אבטחה, כי בן אדם טועה כשהוא כותב קוד לעיתים קרובות. זו כוונת הספר. הוא מביא שם אף דוגמה של מפתחי stackoverflow שמימשו escape לבד ונפלו עם שכחה קטנה, שהביאה פריצה.

פורסם במקור בפורום CODE613 ב19/06/2015 11:54 (+03:00)

softs

@דוד ל.ט.

@softs
אם אתה בונה דינאמי ה SqlParameter לא מגן עליך יותר מהשיטה שהצגתי, למעשה זה בדיוק מה שהוא עושה.

יש מעלה מאוד גדולה להשתמש בקוד של אחרים ולא לכתוב את אותו הקוד לבד.
המעלות הם חיסכון בהקף הקוד, בתחזוקה, ובקריאות - כל מתכנת שמח למצוא מתודה מוכרת מאשר מימוש פרטי של עמית.

ראשון ראשון, ואחרון אחרון:
חיסכון בהיקף הקוד הוא דווקא שימוש ב escape כי השימוש ב SqlParameter הופך קוד לארוך יותר
קוד שלי לשליפת טבלה נראה כך:

'
Dim tbl As DataTable = DB.GetDatatable("SELECT * FROM ReportHeader WHERE CustomerName=" & DB.SqlEscape(name))

ואני גם חושב שהוא גם קריא יותר.
לגבי היכרות עם תשתיות, ומתכנתים ששמחים למצוא מימושים מוכרים, אני חושב שזה זניח ביחס לעבודה של להיכנס לקוד של מישהו אחר.
מה גם שממתודות "לא מוכרות" של אחרים למדתי הרבה מאוד, אז לא תמיד זה שלילי. . .
@דוד ל.ט.

ויש עוד סיבה, שהיא הימנעות משגיאות. להסתכל בוד המקור של מקרוסופט ולהעתיק זה בטוח מבחינת זה, אבל "לטהר קלט לבד" הכוונה לממש את זה עם אי אלו הבדלים (נניח לאפשר איזשהיא תגית נוספת או לאסור יותר), וזה מ-א-ו-ד מסוכן מבחינת אבטחה, כי בן אדם טועה כשהוא כותב קוד לעיתים קרובות. זו כוונת הספר. הוא מביא שם אף דוגמה של מפתחי stackoverflow שמימשו escape לבד ונפלו עם שכחה קטנה, שהביאה פריצה.

לא ברור למי מופנית התשובה שלך, ראית שכתבתי שאני תומך בכזו שיטה? אני חושב שהסברתי מספיק טוב מה ההבדל בין escape ל sanitizing(טיהור).
טיהור קלט לבד זה אסור, מפתחי stackoverfolw כנראה שמימשו לבד טיהור ולא escape, בשיטה של escape אתה לא מנסה לעלות על כל הפרצות האפשריות אלא מוודא שבטוח הקלט ייכנס כסטרינג לשאילתה.

פורסם במקור בפורום CODE613 ב24/06/2015 18:16 (+03:00)

ארכיטקט

ומה רע בפונקציה כזו, שכל תפקידה הוא רק לאמת ששם עמודה חוקי נשלח, לא נראה לי שאפשר לטעות בזה.

public bool AssertValidColumnName(string columnName) {
            //ביטוי רגולרי המאפשר אך ורק אותיות ומספרים וקו תחתי וסוגריים מרובעות ונקודה ללא רווחים 
            //ואם יש רווחים צריך לעטוף את השם של העמודה בסוגריים מרובעות לכאורה //TODO:
            Regex regx = new Regex(@"^[a-zA-Z0-9/_/.[\]]*$", RegexOptions.IgnoreCase);
            return regx.IsMatch(columnName);
        }

פורסם במקור בפורום CODE613 ב15/12/2016 11:21 (+02:00)

clickone

ארכיטקט, לא נראה לי שsofts התכוון שיש בזה רע.
אלא שהכי פשוט (וכנראה הכי זריז) זה לוודא שהקלט נכנס כסטרינג לשאילתא.
כי בפונקצייה הזו אתה רץ בלופ על כל העמודות.

עריכה:
במחשבה שנייה, אתה צודק במצב של מספר. שם לא יועיל לעטוף את הערך במרכאות.
אולי דרך נוספת היא לעטוף את העמודות ב[]. (סוגריים מרובעים אפשר לשים גם על שממות עמודות ללא רווח. אבל לא חובה)

פורסם במקור בפורום CODE613 ב15/12/2016 13:45 (+02:00)

ארכיטקט

@ClickOne

אלא שהכי פשוט (וכנראה הכי זריז) זה לוודא שהקלט נכנס כסטרינג לשאילתא

לא כל כך מדוייק, הכל תלוי באיזה חלק אתה נותן שליטה ליוזר, ניתן להזריק SQL על ידי הוספת ";" ולאחר מכן פקודה כך:
כשרוצים להרוויח דינמיות גבוהה עלות ההגנה עולה.

select * from contacts 
>>; drop database

פורסם במקור בפורום CODE613 ב17/12/2016 19:49 (+02:00)

תחומים - פורום חרדי מקצועי

התגוננות מהזרקת SQL