New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Sep 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Security

Blocking:
issue 766253



Sign in to add a comment
Security: crosh to chronos with awk injection
Project Member Reported by meacer@chromium.org, Sep 18 Back to list
Split 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.
 
crosher.js
3.2 KB View Download
Blocking: 766253
Comment 2 Deleted
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>
Comment 4 Deleted
Comment 5 Deleted
Comment 6 Deleted
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.
Cc: vapier@chromium.org harpreet@chromium.org briannorris@chromium.org benchan@chromium.org kirtika@chromium.org cernekee@chromium.org
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.
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()
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.
Project Member Comment 12 by bugdroid1@chromium.org, Sep 19
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

Labels: Merge-Request-62 Merge-Request-61 Merge-Request-60
Project Member Comment 14 by sheriffbot@chromium.org, Sep 19
Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
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
Labels: -Merge-Request-60 -Merge-Request-61 -Merge-Review-62 Merge-Rejected-60 Merge-Approved-62 Merge-Approved-61
Approved for 61 and 62, unless this is causing significant user impact in the field we should not need it in 60.
looks like  issue 766253  is tracking M61+, so i'll just follow that
Labels: Security_Severity-High Pri-1
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.
Project Member Comment 18 by sheriffbot@chromium.org, Sep 20
Labels: M-61
@palmer: -ENOPERM on  bug 766253  :(
Project Member Comment 20 by bugdroid1@chromium.org, Sep 20
Labels: merge-merged-release-R62-9901.B
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

Project Member Comment 21 by bugdroid1@chromium.org, Sep 20
Labels: merge-merged-release-R61-9765.B
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

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
Project Member Comment 23 by bugdroid1@chromium.org, Sep 22
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

Status: Fixed
Project Member Comment 25 by bugdroid1@chromium.org, Sep 22
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

Project Member Comment 26 by bugdroid1@chromium.org, Sep 22
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

Project Member Comment 27 by sheriffbot@chromium.org, Sep 22
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 28 by sheriffbot@chromium.org, Sep 25
Cc: bhthompson@google.com
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
Labels: -Merge-Approved-61 -Merge-Approved-62
All done with merging.
Labels: CVE-2017-15403
Labels: -Restrict-View-SecurityNotify
Labels: allpublic
Sign in to add a comment