Issue metadata
Sign in to add a comment
|
CVE-2018-12232 CrOS: Vulnerability reported in Linux kernel |
|||||||||||||||||||||||
Issue descriptionVOMIT (go/vomit) has received an external vulnerability report for the Linux kernel. Advisory: CVE-2018-12232 Details: http://vomit.googleplex.com/advisory?id=CVE/CVE-2018-12232 CVSS severity score: 7.1/10.0 Description: In net/socket.c in the Linux kernel through 4.17.1, there is a race condition between fchownat and close in cases where they target the same socket file descriptor, related to the sock_close and sockfs_setattr functions. fchownat does not increment the file descriptor reference count, which allows close to set the socket to NULL during fchownat's execution, leading to a NULL pointer dereference and system crash. This bug was filed by http://go/vomit Please contact us at vomit-team@google.com if you need any assistance.
,
Aug 7
#1: For the record, I am really opposed to handling DoS attacks this way for Chrome OS kernels. Chrome OS kernels are widely used outside Chromebooks nowadays, and those use cases do care about DoS a lot more than for Chromebooks. Updating severity according to guidelines for Chrome OS kernel releases, published at https://docs.google.com/document/d/162La0RHeMUtO6QMdTG8WP5frAUgXSoYYFR45ztGXUN0/edit?usp=sharing
,
Aug 7
,
Aug 7
Upstream commit is 6d8c50dc("socket: close race condition between sock_close() and sockfs_setattr()"). The fix is present in 4.14, and not in older kernels.
The commit introducing the race condition 86741ec25("net: core: Add a UID field to struct sock.") has been cherry-picked into the 4.4 kernel as well. This patch applies cleanly on 4.4.
,
Aug 7
Commit 6741ec25("net: core: Add a UID field to struct sock.") has been cherry-picked into chromeos-4.4, but not into upstream 4.4.y. Fix not needed in upstream stable releases, but in chromeos-4.4.
,
Aug 7
,
Aug 7
I guess the fix 91717ffc9057 ("socket: close race condition between sock_close() and sockfs_setattr()") should be cherry-picked to branches remotes/origin/release-R68-10718.B-chromeos-4.14 and remotes/origin/release-R67-10575.B-chromeos-4.14 .
Also we seem to need a fix in branch remotes/origin/release-R65-10323.B-chromeos-4.4 as well.
Guenter (groeck@), are you working on this issue? If not please feel free to assign it to me.
,
Aug 7
edjee@: I'll handle R68 and R69, then hand off to you.
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/b5977f05bff49451922de9e7ddd19679e61bdb57 commit b5977f05bff49451922de9e7ddd19679e61bdb57 Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Wed Aug 08 04:45:03 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1165495 Reviewed-by: Zubin Mithra <zsm@chromium.org> [modify] https://crrev.com/b5977f05bff49451922de9e7ddd19679e61bdb57/net/socket.c
,
Aug 8
,
Aug 8
This bug requires manual review: M69 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), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 8
,
Aug 8
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e73eb2215343df75d060a2cc4b290ebd030b218e commit e73eb2215343df75d060a2cc4b290ebd030b218e Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Wed Aug 08 17:38:16 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1166124 [modify] https://crrev.com/e73eb2215343df75d060a2cc4b290ebd030b218e/net/socket.c
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/069f1fb01b0e63827e8321150d176a3e2e22eff6 commit 069f1fb01b0e63827e8321150d176a3e2e22eff6 Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Wed Aug 08 17:38:18 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1166126 [modify] https://crrev.com/069f1fb01b0e63827e8321150d176a3e2e22eff6/net/socket.c
,
Aug 9
Merge approved M69.
,
Aug 9
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/68599bf5be2bc91cbc395ee415106ea4fbcda093 commit 68599bf5be2bc91cbc395ee415106ea4fbcda093 Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu Aug 09 17:04:41 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1166125 [modify] https://crrev.com/68599bf5be2bc91cbc395ee415106ea4fbcda093/net/socket.c
,
Aug 9
,
Aug 9
Fixed for R68 and later.
,
Aug 9
Thanks a lot, Guenter. I'll work on this for R67 and R65 from now on.
,
Aug 9
Guenter, I've prepared the following cherrypicks. CL:1169983 (for branch release-R67-10575.B-chromeos-4.14) CL:1169984 (for branch release-R66-10452.B-chromeos-4.14) CL:1169985 (for branch release-R65-10323.B-chromeos-4.4) Do I need to talk to the milestone owners (TPMs)? Given that these milestones are only for COS now, I wonder if they're interested.
,
Aug 9
CLs should all be +2'd. Re milestone owners - I don't know. Maybe ask bhthompson@ for advice ?
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/45440158aa19d315ed6aea42f8d12929effdbf49 commit 45440158aa19d315ed6aea42f8d12929effdbf49 Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri Aug 10 17:37:32 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1165495 Reviewed-by: Zubin Mithra <zsm@chromium.org> (cherry picked from commit b5977f05bff49451922de9e7ddd19679e61bdb57) Reviewed-on: https://chromium-review.googlesource.com/1169983 Trybot-Ready: Edward Jee <edjee@google.com> Commit-Queue: Edward Jee <edjee@google.com> Tested-by: Edward Jee <edjee@google.com> [modify] https://crrev.com/45440158aa19d315ed6aea42f8d12929effdbf49/net/socket.c
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/9e8d00792d96e8c0e64845951643889f1a66583b commit 9e8d00792d96e8c0e64845951643889f1a66583b Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri Aug 10 17:37:33 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1165495 Reviewed-by: Zubin Mithra <zsm@chromium.org> (cherry picked from commit b5977f05bff49451922de9e7ddd19679e61bdb57) Reviewed-on: https://chromium-review.googlesource.com/1169984 Trybot-Ready: Edward Jee <edjee@google.com> Commit-Queue: Edward Jee <edjee@google.com> Tested-by: Edward Jee <edjee@google.com> [modify] https://crrev.com/9e8d00792d96e8c0e64845951643889f1a66583b/net/socket.c
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/9f2ecb7c9cd4b2886853e2feba31f539dde25a4d commit 9f2ecb7c9cd4b2886853e2feba31f539dde25a4d Author: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri Aug 10 17:37:35 2018 UPSTREAM: socket: close race condition between sock_close() and sockfs_setattr() fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor <shankarapailoor@gmail.com> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 6d8c50dcb029872b298eea68cc6209c866fd3e14) BUG= chromium:871731 TEST=Run POC Change-Id: Ied1e0561ab0c300ef329919457c5b09451cf7645 Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1165495 Reviewed-by: Zubin Mithra <zsm@chromium.org> (cherry picked from commit b5977f05bff49451922de9e7ddd19679e61bdb57) Reviewed-on: https://chromium-review.googlesource.com/1169985 Trybot-Ready: Edward Jee <edjee@google.com> Commit-Queue: Edward Jee <edjee@google.com> Tested-by: Edward Jee <edjee@google.com> [modify] https://crrev.com/9f2ecb7c9cd4b2886853e2feba31f539dde25a4d/net/socket.c
,
Aug 10
Thanks!
,
Aug 11
,
Nov 17
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 |
||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||
Comment 1 by allenwebb@chromium.org
, Aug 7Owner: groeck@chromium.org
Status: Assigned (was: Untriaged)