Issue metadata
Sign in to add a comment
|
Security: crosh to chronos with awk injection |
||||||||||||||||||||||
Issue descriptionSplit from bug 766253 : """ Crosh has access to a limited set of command line commands. network_diag has an awk command injection bug. platform2/crosh/network_diag:382 diag_arp(): arp="$(${ARP} -an | awk '/('${ip}').*'${ifc}'$/ { print $4 }')" Run that with ip=.)/{}BEGIN{system(sprintf("echo%c<base64>|base64%c-d|sh",32,32))}# It uses sprintf %d 32 and base64 for spaces, because crosh splits arguments with spaces. But this awk is actually only reached when the ip belongs to the network of some interface: ip & netmask = network ip. The binary and is done in do_netmask () { local -a ip=($(do_address_parts "$1")) local -a mask=($(do_address_parts "$2")) local -a ret for part in ${!ip[@]}; do ret+=("$((ip[part] & mask[part]))") Which will break if ip[part] is not a number. Surprisingly, bash allows something like $(( a=5 )). And this modifies the variables outside the parenthesis! So craft an ip like this: 192.168.ip[3]=0,8.)/{}BEGIN... That's for the network address 192.168.8.0. It splits into 4 parts. 3rd part is ip[3]=0,8 so it overwrites the garbage in the 4th part to 0 and then evaluates to 8. And now 4th part successfully evaluates as 0! The exploit also uses network_diag to get the actual network address of wlan0. Code in crosher.js. """ vapier@: As the owner of crosh / network_diag, can you PTAL or delegate? Thanks.
,
Sep 18 2017
this script has been with the project since the beginning. i don't think i've ever reviewed it. maybe time to rework it quite a bit :). - diag_arp is only used by diag_route - diag_route is only used by diag_ping - diag_ping is used by diag_nameservers (but with a list from system resolv.conf), and used by diag_connectivity. - diag_connectivity is only used by diag_webget - diag_webget is only by used diag_run - diag_run is reachable directly from the network_diag command line from crosh (oops) - network_diag --route <badness>
,
Sep 19 2017
i've posted a simple CL to cut off the bug: https://chromium-review.googlesource.com/671878 but of course, all of those piles of shill scripts shouldn't be running with these privs in the first place, and we're going to just keep fighting fires here. i count roughly ~3k lines of shell across ~10 programs. so i've opened issue 766379 to track converting all of these to a reduced environment.
,
Sep 19 2017
,
Sep 19 2017
Drive-by: It looks like awk has a -S or --sandbox flag that prevents system() et. al. from working. You might specify it. A better idea, if chromeos is maintaining its own copies of utilities, would be just to patch all this unsafe stuff out. I doubt anything is using it besides the bad guys.
,
Sep 19 2017
gawk has a --sandbox, but POSIX awk doesn't, and we use mawk for speed/size in images we aren't maintaining our own copies of things, but adding a USE flag to the mawk ebuild to patch it out sounds like something i can get into Gentoo wrt general usage, i know Gentoo uses gawk a decent amount, and those in turn have used system()
,
Sep 19 2017
Thanks. Pity they didn't make it safe by default, e.g. you'd have to specify --no-sandbox to use system() in the few places it was need.
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/e759936a8922d70969670fa3dba01e132be03994 commit e759936a8922d70969670fa3dba01e132be03994 Author: Mike Frysinger <vapier@chromium.org> Date: Tue Sep 19 20:26:54 2017 crosh: network_diag: validate hostnames/IPs If you typo an address to probe, you get a stream of resolution errors if it's an invalid DNS name. Do some sanity checking up front so the user knows what's going on vs getting even more confused. BUG= chromium:766271 TEST=`network_diag 127.0.0.1;` throws a nice error now Change-Id: Iee02f197748187441de452dd806bba991d945fcf Reviewed-on: https://chromium-review.googlesource.com/671878 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Mattias Nissler <mnissler@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/e759936a8922d70969670fa3dba01e132be03994/crosh/network_diag
,
Sep 19 2017
,
Sep 19 2017
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 19 2017
Approved for 61 and 62, unless this is causing significant user impact in the field we should not need it in 60.
,
Sep 19 2017
looks like issue 766253 is tracking M61+, so i'll just follow that
,
Sep 20 2017
The parent bug is what matters, but I'm tagging this with Pri and Severity just for completeness. It's very important to get an owner for 766379, because I suspect that vapier is correct that we'll just be fighting fires here for a long time otherwise.
,
Sep 20 2017
,
Sep 20 2017
@palmer: -ENOPERM on bug 766253 :(
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/37c9c51785c86ded813600439bb1e31138b38364 commit 37c9c51785c86ded813600439bb1e31138b38364 Author: Mike Frysinger <vapier@chromium.org> Date: Wed Sep 20 16:45:32 2017 crosh: network_diag: validate hostnames/IPs If you typo an address to probe, you get a stream of resolution errors if it's an invalid DNS name. Do some sanity checking up front so the user knows what's going on vs getting even more confused. BUG= chromium:766271 TEST=`network_diag 127.0.0.1;` throws a nice error now Change-Id: Iee02f197748187441de452dd806bba991d945fcf (cherry picked from commit e759936a8922d70969670fa3dba01e132be03994) Reviewed-on: https://chromium-review.googlesource.com/673923 Reviewed-by: Brian Norris <briannorris@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/37c9c51785c86ded813600439bb1e31138b38364/crosh/network_diag
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/6e8fecb1554c8c802b5ef36ccdd3ff3231e0e90a commit 6e8fecb1554c8c802b5ef36ccdd3ff3231e0e90a Author: Mike Frysinger <vapier@chromium.org> Date: Wed Sep 20 16:45:34 2017 crosh: network_diag: validate hostnames/IPs If you typo an address to probe, you get a stream of resolution errors if it's an invalid DNS name. Do some sanity checking up front so the user knows what's going on vs getting even more confused. BUG= chromium:766271 TEST=`network_diag 127.0.0.1;` throws a nice error now Change-Id: Iee02f197748187441de452dd806bba991d945fcf (cherry picked from commit e759936a8922d70969670fa3dba01e132be03994) Reviewed-on: https://chromium-review.googlesource.com/673924 Reviewed-by: Brian Norris <briannorris@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/6e8fecb1554c8c802b5ef36ccdd3ff3231e0e90a/crosh/network_diag
,
Sep 20 2017
posted CL to actually fix the awk abuse: https://chromium-review.googlesource.com/675706 after that, i'll close this out and deal with follow up issues: - issue 767182 : mawk+sandbox - issue 766379 : sandbox shill scripts
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/b0de69a847cab5cd8452c872f8327b289a3c07c0 commit b0de69a847cab5cd8452c872f8327b289a3c07c0 Author: Mike Frysinger <vapier@chromium.org> Date: Fri Sep 22 00:19:55 2017 crosh: network_diag: make ip matching a bit more robust The current way we match the IP address doesn't escape the regex which means the dots are allowed to match any character. This could lead to extra/incorrect matches in the arp table which confuses the diagnostic logic (thinks we have an arp entry when we really don't). There's a similar issue when it comes to matching device names, but that doesn't come up as often as all our interfaces are alphanumeric (with perhaps dashes or underscores thrown in). We might as well be correct at any rate :). BUG= chromium:766271 TEST=`network_diag --wifi` still works Change-Id: Iac32cc6d80547a63d24417bbef85d1cb620e0f38 Reviewed-on: https://chromium-review.googlesource.com/675706 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/b0de69a847cab5cd8452c872f8327b289a3c07c0/crosh/network_diag
,
Sep 22 2017
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/a958f17cf6a226dcd4a3768252a94cc1d0c315b3 commit a958f17cf6a226dcd4a3768252a94cc1d0c315b3 Author: Mike Frysinger <vapier@chromium.org> Date: Fri Sep 22 07:45:36 2017 crosh: network_diag: make ip matching a bit more robust The current way we match the IP address doesn't escape the regex which means the dots are allowed to match any character. This could lead to extra/incorrect matches in the arp table which confuses the diagnostic logic (thinks we have an arp entry when we really don't). There's a similar issue when it comes to matching device names, but that doesn't come up as often as all our interfaces are alphanumeric (with perhaps dashes or underscores thrown in). We might as well be correct at any rate :). BUG= chromium:766271 TEST=`network_diag --wifi` still works Change-Id: Iac32cc6d80547a63d24417bbef85d1cb620e0f38 (cherry picked from commit b0de69a847cab5cd8452c872f8327b289a3c07c0) Reviewed-on: https://chromium-review.googlesource.com/677920 Reviewed-by: Mattias Nissler <mnissler@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/a958f17cf6a226dcd4a3768252a94cc1d0c315b3/crosh/network_diag
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/ec1a5329eb3bba56866488687909ab401042be1b commit ec1a5329eb3bba56866488687909ab401042be1b Author: Mike Frysinger <vapier@chromium.org> Date: Fri Sep 22 07:45:37 2017 crosh: network_diag: make ip matching a bit more robust The current way we match the IP address doesn't escape the regex which means the dots are allowed to match any character. This could lead to extra/incorrect matches in the arp table which confuses the diagnostic logic (thinks we have an arp entry when we really don't). There's a similar issue when it comes to matching device names, but that doesn't come up as often as all our interfaces are alphanumeric (with perhaps dashes or underscores thrown in). We might as well be correct at any rate :). BUG= chromium:766271 TEST=`network_diag --wifi` still works Change-Id: Iac32cc6d80547a63d24417bbef85d1cb620e0f38 (cherry picked from commit b0de69a847cab5cd8452c872f8327b289a3c07c0) Reviewed-on: https://chromium-review.googlesource.com/677919 Reviewed-by: Mattias Nissler <mnissler@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/ec1a5329eb3bba56866488687909ab401042be1b/crosh/network_diag
,
Sep 22 2017
,
Sep 25 2017
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
,
Sep 28 2017
All done with merging.
,
Nov 6 2017
,
Nov 13 2017
,
Nov 13 2017
,
Jan 22 2018
,
Jan 23 2018
,
Mar 27 2018
,
Apr 25 2018
,
Sep 13
Bulk verify old fixed bugs...
,
Jan 4
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by mea...@chromium.org
, Sep 18 2017