Issue metadata
Sign in to add a comment
|
Command injection bug in crash_sender |
||||||||||||||||||||||
Issue description
When passed as input to crash_sender, a specially-crafted .meta file can exploit a command-injection bug in the invocation of the sed command by the get_key_value() function, which is called to parse any lines starting with "upload_" in the file.
An example of a key that exploits the command injection when parsed by the sed command is:
/p;s^.*^setsid${IFS}bash${IFS}<path-to-shell-script>${IFS}\&^ep;/=1
By injecting the -e flag into the sed command, the malicious input can invoke an arbitrary command in the context of the crash_sender script.
Code:
Grabbing lines in the file that start with "upload_": https://cs.corp.google.com/chromeos_public/src/platform2/crash-reporter/crash_sender?rcl=9421db9828e7450460af1fa7348427f192aae045&l=369
Vulnerable sed invocation: https://cs.corp.google.com/chromeos_public/src/platform2/crash-reporter/crash_sender?rcl=9421db9828e7450460af1fa7348427f192aae045&l=241
,
Mar 1 2018
,
Mar 1 2018
Note: repo in bash on CrOS may require doing "export IFS=' '" first if just running the vulnerable script in bash and not in the context of crash_sender.
,
Mar 1 2018
,
Mar 2 2018
FWIW, note that there was a previous attempt to convert crash_sender to C++ per issue 391887 and there's even a CL at: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/208671 Maybe this would be a good time to get that work finished? ;-)
,
Mar 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/9b37bd73d816e8d3b45b62527d74e60826cf6c0c commit 9b37bd73d816e8d3b45b62527d74e60826cf6c0c Author: Mike Frysinger <vapier@chromium.org> Date: Fri Mar 02 21:25:10 2018 crash: crash_sender: handle corrupt reports better If the crash report gets corrupted (like minor disk corruption), it can confuse crash_sender. Add a sanity check so we clear out the report instead. BUG= chromium:817993 TEST=created a corrupt report and crash_sender deleted it TEST=precq passes (autotests) Change-Id: Ib33a2d03dceda5ec71dd3591a2aeff5704b76d67 Reviewed-on: https://chromium-review.googlesource.com/944788 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/9b37bd73d816e8d3b45b62527d74e60826cf6c0c/crash-reporter/crash_sender
,
Mar 2 2018
Is this a good candidate for a merge?
,
Mar 2 2018
After this bakes on ToT, I should say.
,
Mar 2 2018
yeah, the CL i put together was with backports in mind looking at sed closer, it has a --sandbox runtime option. i'll see about adding a configure flag so it's like mawk where the sandbox is always active. i'm not aware of any users of e/r/w commands off hand.
,
Mar 2 2018
This bug requires manual review: We are only 3 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 2 2018
upstream sed patch for configure: https://lists.gnu.org/archive/html/bug-sed/2018-03/msg00001.html i'll see how that progresses before seeing what to land in Gentoo/CrOS
,
Mar 3 2018
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 4 2018
,
Mar 5 2018
,
Mar 5 2018
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/portage-stable/+/84e607abdb70d67ba84f8bab353b9db918df1504 commit 84e607abdb70d67ba84f8bab353b9db918df1504 Author: Mike Frysinger <vapier@chromium.org> Date: Wed Mar 07 04:19:26 2018 sed: upgraded package to upstream Upgraded sys-apps/sed to version 4.4 everywhere. BUG= chromium:817993 , chromium:818527 TEST=precq passes Change-Id: Ia66b09188fadca845e2ab3f68b05ae0fd637d62e Reviewed-on: https://chromium-review.googlesource.com/949523 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Chirantan Ekbote <chirantan@chromium.org> Reviewed-by: Micah Morton <mortonm@chromium.org> [delete] https://crrev.com/168c2647b03a6fb96144b24deb9a76dd095e89a3/sys-apps/sed/files/sed-4.1.5-alloca.patch [modify] https://crrev.com/84e607abdb70d67ba84f8bab353b9db918df1504/sys-apps/sed/Manifest [rename] https://crrev.com/84e607abdb70d67ba84f8bab353b9db918df1504/sys-apps/sed/sed-4.4-r1.ebuild [modify] https://crrev.com/84e607abdb70d67ba84f8bab353b9db918df1504/sys-apps/sed/metadata.xml
,
Mar 7 2018
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/1c54953c675b7a7caa0f079e06ef263f9679e8d6 commit 1c54953c675b7a7caa0f079e06ef263f9679e8d6 Author: Mike Frysinger <vapier@chromium.org> Date: Wed Mar 07 17:59:27 2018 crash: crash_sender: handle corrupt reports better If the crash report gets corrupted (like minor disk corruption), it can confuse crash_sender. Add a sanity check so we clear out the report instead. BUG= chromium:817993 TEST=created a corrupt report and crash_sender deleted it TEST=precq passes (autotests) Change-Id: Ib33a2d03dceda5ec71dd3591a2aeff5704b76d67 (cherry picked from commit 9b37bd73d816e8d3b45b62527d74e60826cf6c0c) Reviewed-on: https://chromium-review.googlesource.com/949543 Reviewed-by: Mattias Nissler <mnissler@chromium.org> Commit-Queue: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/1c54953c675b7a7caa0f079e06ef263f9679e8d6/crash-reporter/crash_sender
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/601f69504be5ef7af00e639da08026138930a211 commit 601f69504be5ef7af00e639da08026138930a211 Author: Mike Frysinger <vapier@chromium.org> Date: Wed Mar 07 17:59:30 2018 crash: crash_sender: handle corrupt reports better If the crash report gets corrupted (like minor disk corruption), it can confuse crash_sender. Add a sanity check so we clear out the report instead. BUG= chromium:817993 TEST=created a corrupt report and crash_sender deleted it TEST=precq passes (autotests) Change-Id: Ib33a2d03dceda5ec71dd3591a2aeff5704b76d67 (cherry picked from commit 9b37bd73d816e8d3b45b62527d74e60826cf6c0c) Reviewed-on: https://chromium-review.googlesource.com/949384 Reviewed-by: Mattias Nissler <mnissler@chromium.org> Commit-Queue: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/601f69504be5ef7af00e639da08026138930a211/crash-reporter/crash_sender
,
Mar 9 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 13 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/29c8fab0561dc26fdff2aab63f8e08754ff01cf1 commit 29c8fab0561dc26fdff2aab63f8e08754ff01cf1 Author: Mike Frysinger <vapier@chromium.org> Date: Thu Mar 15 07:48:36 2018 chromeos: turn on USE=forced-sandbox for mawk & sed We currently always turn this on for mawk via a bashrc, so this doesn't change behavior there. It just makes it easier to sync with Gentoo and drop our local patches (in the future). For sed, this is new behavior. We're disabling support for the e/r/w commands because (1) we don't care about them and have no known legit users and (2) they needlessly expose random shell scripts to attacks. BUG= chromium:817993 , chromium:818527 TEST=precq passes Change-Id: I2173bb02fb271c6c31400efc792ad0b5b01e87b4 Reviewed-on: https://chromium-review.googlesource.com/949524 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Mattias Nissler <mnissler@chromium.org> Reviewed-by: David Schneider <dnschneid@chromium.org> [modify] https://crrev.com/29c8fab0561dc26fdff2aab63f8e08754ff01cf1/profiles/targets/chromeos/package.use
,
May 30 2018
,
Jun 10 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 15 2018
,
Jun 21 2018
Hi, can this be re-Restricted temporarily? I believe that the fix in #14 is insufficient.
,
Jun 21 2018
not sure what you mean. we outright reject invalid metadata keys, and we changed awk/sed so that even if you do manage to inject arbitrary strings, the r/w/e commands are always disabled (--sandbox is always active). that's enabled in R67+ which should be stable everywhere. if you have an attack that works against R67+, please file a new bug for us to investigate.
,
Jun 21 2018
There's a multi-second TOCTOU race in the rejection. You're right that it's not exploitable fully but the crash_sender patch is easily bypassable.
,
Jun 21 2018
no doubt there is a TOCTOU race here with how we parse these reports. this shell script is large and gross. we're in the process of converting it to C++, so i'll make note that we take into account file ownership and fully ingest/process the file in memory rather than constantly going back to disk. issue 391887 and issue 441427 if you want to star them.
,
Jun 21 2018
Understood. Thought it was worth mentioning because it's up to 10 minutes of race, rather than the normal bash <second race. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 Deleted