אל תשתמשו ב-child_process.exec כדי להריץ פקודות ב-node שלכם

זו סיפורה של פרצת אבטחה שאני הייתי אחראי לה. אני חושב שמהסיפור הזה אפשר ללמוד לא מעט. לא מזמן כתבתי npm module שאפילו הוזכר פה. ה-npm module הוא קטן ונחמד ועוסק באכיפת סטנדרטים לשמות של בראנצ'ים בגיט. עד כאן הכל טוב ויפה, אך כשצייצתי עליו בטוויטר, עוד לפני שפרסמתי אותו באופן רשמי. פנה אלי לירן טל שאולי אתם מכירים אותו כמחבר הספר Essential Node.js Security ובכלל כתורם גדול לקהילת הקוד הפתוח. לירן הראה לי בעית אבטחה פוטנציאלית שיש בקוד שלי.

בקוד היתה פונקציה קצת מסורבלת ולא ממש הכרחית שבדקה את שם הבראנץ'. הקוד שבתוכה נראה בערך כך:

const exec = require('child_process').execSync;
branch = exec('git symbolic-ref HEAD 2> NUL || git rev-parse --short HEAD 2> NUL');

מה הבעיה עם הקוד החביב הזה? השימוש ב-execSync. וכאן נכנסת פרצת אבטחה שהכרתי בשפות אחרות אבל פחות ב-node. הפרצה הזו נגרמת מהכנסת נתונים ישירות ל-execSync. מה ש-execSync זה פשוט לפתוח SHELL ולהריץ פקודות. באופן תיאורטי, אם למשל יש לי משהו כמו הקוד הזה:

...
app.get('/', (req, res) => {
  child_process.exec(
    'gzip ' + req.query.file_path);
  res.send('/* gzipped data */')
})
...

שאותו לקחתי מהדוגמה המצוינת שיש באתר hacker noon. הקוד הזה בגדול יושב לו בשרת node express ומה שהוא עושה זה לקבל פרמטר בשם file_path, לקחת את שם הקובץ שיש בו ולהחזיר אותו כקובץ zip. משהו שבהחלט אפשר לראות באתרים. המשתמש מכניס משהו כזה

: example.com/?file_path="some_image.jpg"  

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

example.com/?file_path="some_image.jpg; rm -rf /"

מה שירוץ בפועל זה:

gzip some_image.jpg; rm -rf /

כי child_process.exec יריץ את כל מה שיתנו לו כולל רווחים וכולל מה שבא לו. אז נכון, בסקריפט הקטן והחביב שלי לא היה שימוש במשתנים. אבל, וזה אבל משמעותי, למה להכניס ראש בריא למיטה חולה? ואם בעתיד אני או מישהו אחר שיתרום לשם קוד, יכניס לשם משתנים? הפתרון הטוב ביותר הוא מראש לא להכנס לפינות ומראש לא להשתמש ב-child_process.exec לשום דבר. במה כן להשתמש? למשל ב-execFile או ב-execFileSync אם אתם צריכים פעולה סינכרונית. גם כאן אנחנו יכולים להריץ איזו פקודה שבא לנו, אבל כאן כל הארגומנטים חייבים לבוא בתור מערך. תכניס רווח ותקבל error.

app.get('/', (req, res) => {
  child_process.execFileSynch('gzip', [req.query.file_path]);
  res.send('/* gzipped data */')
})
...

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

טוב, אז אנחנו כבר יודעים להשתמש ב-execFileSynch או ב-execFile. במקום ב-exec. אבל מה הפתרון האמיתי? הפתרון האמיתי הוא להשתמש כמובן ב-static code analysis, במקרה שלנו ב-eslint, על מנת להכשיל את הבילד בכל שימוש ב-exec. ניתוח קוד סטטי כזה יבטיח שאנחנו וכל מישהו אחר שיתרום אי פעם קוד לפרויקט שלנו, לא נשתמש בפונקציה שהיא בעייתית מלכתחילה. איך עושים את זה? משתמשים בתוסף של eslint בשם eslint-plugin-security למשל, או כותבים תוסף משלכם. מה שחשוב הוא זה לנסות למנוע את הבעיה מההתחלה 🙂