Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
66043003:
Put app shim IPC socket in a temporary directory. (Mac) (Closed)
|
Created:
7 years, 1 month ago by jackhou1 Modified:
6 years, 10 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPut app shim IPC socket in a temporary directory. (Mac)
The path to the App Shim Socket must be less than 104 characters. Since the
socket is inside the user data dir, the path is frequently too long in tests. It is
also very likely to be too long for any users that have a long username.
In this CL, the socket is always placed at:
"<DIR_TEMP>/XXXXXX/App Shim Socket"
where XXXXXX is replaced with a random suffix by mkdtemp.
A symlink to the socket is placed in "<udd>/App Shim Socket" (the old socket
path). The shim reads this symlink to find the actual socket.
BUG=240554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250051
Patch Set 1 #Patch Set 2 : Always use short socket path #
Total comments: 22
Patch Set 3 : Change to /tmp/chrome-<hash of udd>/App Shim Socket #
Total comments: 12
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Delete folder unconditionally. #Patch Set 6 : Use PostTask to delete file. #
Total comments: 4
Patch Set 7 : Sync and rebase #Patch Set 8 : Address comments #
Total comments: 6
Patch Set 9 : Sync and rebase #Patch Set 10 : Sync and rebase #Patch Set 11 : Always delete directory in tmp, and create with mkdtemp #Patch Set 12 : should_try_long_socket_path_ #Patch Set 13 : Put App Shim Socket in "/tmp/chrome-<hash of UDD>/", make a symlink to it at "<UDD>/App Shim Socket… #
Total comments: 32
Patch Set 14 : Address comments #
Total comments: 5
Patch Set 15 : Add test, update comments. #Patch Set 16 : Sync and rebase #
Total comments: 2
Patch Set 17 : Sync and rebase #Patch Set 18 : Use PathService(DIR_TEMP) instead of "/tmp" #
Total comments: 8
Patch Set 19 : Address comments #Patch Set 20 : Sync, rebase, git cl format #Messages
Total messages: 45 (0 generated)
I've tested that this works. I.e. running a version of chromium built without this CL, and starting a shim that includes this CL, the shim connects and works. It currently just fails if the path is already taken. Do you think we should have it try different paths if that happens?
Nice - I really like that you're getting rid of the specialised testing code. We'll definitely need a security expert to have a look at this, but I think pointing out that Spotlight and other Mac processes are using a similar technique will help that a lot. https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:58: PathService::Get(chrome::DIR_USER_DATA, &socket_path); nit: check return value https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:59: socket_path = socket_path.Append(app_mode::kAppShimSocketName); should this be part of GetShortSocketPath? Then in a revision or two, we could put kAppShimSocketName into an anonymous namespace https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_mac.mm:48: LOG(ERROR) << "Couldn't get user data directory while creating App Shim " while we are here.. let's ditch this LOG and just return. Chrome would have already died due to a CHECK(..) in chrome::GetUserDataDir() called from ChromeBrowserMainParts::PreCreateThreadsImpl() https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_mac.mm:59: app_mode::GetShortSocketPath(udd_plus_socket_name); Ah, so this is just /tmp/TWFuIGlzIGRpc3Rpbm which a domain socket. I think we should put it in something like /tmp/TWFuIGlzIGRpc3Rpbm/Chrome\ App\ Shim\ Socket Otherwise (a) the user has no information about what this file in /tmp is for; and (b) I think OSX doesn't properly enforce permissions on the socket file itself, but (I think!) should still enforce it on the parent folder. and be sure to create /tmp/TWFuIGlzIGRpc3Rpbm/ with restricted privileges (mode 700, etc.). It also needs to be a scoped folder. One other concern... One might `sudo rm -rf /tmp/*`. Heck, I'm tempted to do it now because I jst noticed puppet::mac has gone berserk and made 2000 tmpXXXX files in there :|. But I don't think this is an issue. Actually, I just found out that Spotlight does just what this is doing, so +1 /tmp $ sudo ls -l /tmp/launchd-817.3J2EoS/ total 0 srwx------ 1 _spotlight wheel 0 13 Nov 09:39 sock And there's an idea for preventing (the super improbable) name collisions, you could use a path like /tmp/chrome-PID-TWFuIGlzIGRpc3Rpbm/sock Do you think it's worthwhile? https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/chrome_main... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/chrome_main... apps/app_shim/chrome_main_app_mode_mac.mm:149: base::FilePath socket_path_; can this be an argument to CreateChannelAndSendLaunchApp()? https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/chrome_main... apps/app_shim/chrome_main_app_mode_mac.mm:297: if (socket_path_ != udd_plus_socket_name) { I think we shouldn't try this if we ever successfully connected either. Maybe just a bool member function like `should_retry_connect_at_m32_socket_path` (or something shorter..) set false on the next line, and on a successful connect. https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... File chrome/common/mac/app_mode_common.h (right): https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.h:130: // IPC::kMaxSocketNameLength imposes a limit to the length of the path to a nit: I'd probably start this with `Due to kernel API restraints..` https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.h:131: // socket. The app shim socket is stored in the user's home directory, so in nit: update comment (i.e. since there's no sockets in the home dir any more) https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.h:135: base::FilePath GetShortSocketPath(const base::FilePath& full_path); maybe full_path -> user_data_dir? https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.mm:49: if (base::Base64Encode(base::SHA1HashString(full_path.value()), under what cases can it return false? I got curious and answered it myself... modp_b64_encode never returns MODP_B64_ERROR so the check in Base64Encode is pointless, and base::Base64Encode should really return `void`. I guess maybe NOTREACHED() before the final return? https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.mm:51: ReplaceChars(short_path, "/", "_", &short_path); I think short_path will have a guaranteed length - maybe DCHECK_EQ?
https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:58: PathService::Get(chrome::DIR_USER_DATA, &socket_path); On 2013/11/21 11:12:05, tapted wrote: > nit: check return value Done. https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:59: socket_path = socket_path.Append(app_mode::kAppShimSocketName); On 2013/11/21 11:12:05, tapted wrote: > should this be part of GetShortSocketPath? > > Then in a revision or two, we could put kAppShimSocketName into an anonymous > namespace Changed it so that GetShortSocketPath takes the user data dir. The shim gets the user data dir from its Plist. I think it's so that, if there are multiple Chrome instances using different user data dirs, it connects to the right one. Though we might be able to make the app_mode_loader set the user data dir so that PathService returns the same path as the Plist. https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_mac.mm:48: LOG(ERROR) << "Couldn't get user data directory while creating App Shim " On 2013/11/21 11:12:05, tapted wrote: > while we are here.. let's ditch this LOG and just return. Chrome would have > already died due to a CHECK(..) in chrome::GetUserDataDir() called from > ChromeBrowserMainParts::PreCreateThreadsImpl() Done. https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/app_shim_ho... apps/app_shim/app_shim_host_manager_mac.mm:59: app_mode::GetShortSocketPath(udd_plus_socket_name); On 2013/11/21 11:12:05, tapted wrote: > Ah, so this is just /tmp/TWFuIGlzIGRpc3Rpbm which a domain socket. I think we > should put it in something like /tmp/TWFuIGlzIGRpc3Rpbm/Chrome\ App\ Shim\ > Socket > > Otherwise (a) the user has no information about what this file in /tmp is for; > and (b) I think OSX doesn't properly enforce permissions on the socket file > itself, but (I think!) should still enforce it on the parent folder. > > and be sure to create /tmp/TWFuIGlzIGRpc3Rpbm/ with restricted privileges (mode > 700, etc.). It also needs to be a scoped folder. > > One other concern... One might `sudo rm -rf /tmp/*`. Heck, I'm tempted to do it > now because I jst noticed puppet::mac has gone berserk and made 2000 tmpXXXX > files in there :|. But I don't think this is an issue. > > Actually, I just found out that Spotlight does just what this is doing, so +1 > /tmp > > $ sudo ls -l /tmp/launchd-817.3J2EoS/ > total 0 > srwx------ 1 _spotlight wheel 0 13 Nov 09:39 sock > > And there's an idea for preventing (the super improbable) name collisions, you > could use a path like > > /tmp/chrome-PID-TWFuIGlzIGRpc3Rpbm/sock > > Do you think it's worthwhile? Yeah, I think this is a good idea. I've changed the directory to just hash the UDD, so /tmp/chrome-<hash>/ is kind of a proxy UDD, and "App Shim Socket" goes in there. https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/chrome_main... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/chrome_main... apps/app_shim/chrome_main_app_mode_mac.mm:149: base::FilePath socket_path_; On 2013/11/21 11:12:05, tapted wrote: > can this be an argument to CreateChannelAndSendLaunchApp()? Done. https://codereview.chromium.org/66043003/diff/30001/apps/app_shim/chrome_main... apps/app_shim/chrome_main_app_mode_mac.mm:297: if (socket_path_ != udd_plus_socket_name) { On 2013/11/21 11:12:05, tapted wrote: > I think we shouldn't try this if we ever successfully connected either. Maybe > just a bool member function like `should_retry_connect_at_m32_socket_path` (or > something shorter..) set false on the next line, and on a successful connect. Done. https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... File chrome/common/mac/app_mode_common.h (right): https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.h:130: // IPC::kMaxSocketNameLength imposes a limit to the length of the path to a On 2013/11/21 11:12:05, tapted wrote: > nit: I'd probably start this with `Due to kernel API restraints..` Done. https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.h:131: // socket. The app shim socket is stored in the user's home directory, so in On 2013/11/21 11:12:05, tapted wrote: > nit: update comment (i.e. since there's no sockets in the home dir any more) Done. https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.h:135: base::FilePath GetShortSocketPath(const base::FilePath& full_path); On 2013/11/21 11:12:05, tapted wrote: > maybe full_path -> user_data_dir? Done. https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.mm:49: if (base::Base64Encode(base::SHA1HashString(full_path.value()), On 2013/11/21 11:12:05, tapted wrote: > under what cases can it return false? > > I got curious and answered it myself... modp_b64_encode never returns > MODP_B64_ERROR so the check in Base64Encode is pointless, and base::Base64Encode > should really return `void`. > > I guess maybe NOTREACHED() before the final return? Done. https://codereview.chromium.org/66043003/diff/30001/chrome/common/mac/app_mod... chrome/common/mac/app_mode_common.mm:51: ReplaceChars(short_path, "/", "_", &short_path); On 2013/11/21 11:12:05, tapted wrote: > I think short_path will have a guaranteed length - maybe DCHECK_EQ? Done.
https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:52: // (IPC::kMaxSocketNameLength). To circumvent this, we use a short path in nit: circumvent sounds dodgy, perhaps "accommodate" :) https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:59: if (!file_util::CreateDirectory(socket_path.DirName())) hm - I think we should try to remove this folder when Chrome exits. There may even be things set up to complain in buildbots if there is stuff lying around after a clean exit. Also you might be able to reduce the branching or make things more symmetric by going straight to `CreateDirectory` in the first condition rather than `DirectoryExists`. https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:208: const base::FilePath& socket_path) { nit: indenting https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:210: channel_ = new IPC::ChannelProxy(handle, IPC::Channel::MODE_NAMED_CLIENT, ooh - this is not refcounted/scoped. Should we make channel_ a scoped_ptr? https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:292: // On failure, try to connect to the long socket path. nit: maybe a TODO - remove this around m35?
https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:52: // (IPC::kMaxSocketNameLength). To circumvent this, we use a short path in On 2013/11/22 03:29:43, tapted wrote: > nit: circumvent sounds dodgy, perhaps "accommodate" :) Done. https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:59: if (!file_util::CreateDirectory(socket_path.DirName())) On 2013/11/22 03:29:43, tapted wrote: > hm - I think we should try to remove this folder when Chrome exits. There may > even be things set up to complain in buildbots if there is stuff lying around > after a clean exit. > > Also you might be able to reduce the branching or make things more symmetric by > going straight to `CreateDirectory` in the first condition rather than > `DirectoryExists`. I'm just deleting the directory on shutdown if we were the one to create it. Is it ok to delete it on the UI thread if it's during shutdown? If not, is there a way to have it delete on the file thread? https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:208: const base::FilePath& socket_path) { On 2013/11/22 03:29:43, tapted wrote: > nit: indenting Done. https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:210: channel_ = new IPC::ChannelProxy(handle, IPC::Channel::MODE_NAMED_CLIENT, On 2013/11/22 03:29:43, tapted wrote: > ooh - this is not refcounted/scoped. Should we make channel_ a scoped_ptr? Done. https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:292: // On failure, try to connect to the long socket path. On 2013/11/22 03:29:43, tapted wrote: > nit: maybe a TODO - remove this around m35? Done.
https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:59: if (!file_util::CreateDirectory(socket_path.DirName())) On 2013/11/22 04:25:00, jackhou1 wrote: > On 2013/11/22 03:29:43, tapted wrote: > > hm - I think we should try to remove this folder when Chrome exits. There may > > even be things set up to complain in buildbots if there is stuff lying around > > after a clean exit. > > > > Also you might be able to reduce the branching or make things more symmetric > by > > going straight to `CreateDirectory` in the first condition rather than > > `DirectoryExists`. > > I'm just deleting the directory on shutdown if we were the one to create it. Is > it ok to delete it on the UI thread if it's during shutdown? If not, is there a > way to have it delete on the file thread? I'd probably delete it unconditionally - otherwise any Chrome crash would put it there and it would never go away again. For deleting on the UI thread... I don't actually know - does a post task not get flushed? void IPC::ChannelFactory::Close() kinda gets around it just by doing ::unlink so doesn't get checked by thread_restrictions. https://codereview.chromium.org/66043003/diff/220001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/220001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:58: if (file_util::CreateDirectory(socket_path.DirName())) { pretty sure this succeeds if the directory already exists (but it might not be writable) https://codereview.chromium.org/66043003/diff/220001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:60: } else { (so I think this line can be removed)
https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/120001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:59: if (!file_util::CreateDirectory(socket_path.DirName())) On 2013/11/22 05:04:31, tapted wrote: > On 2013/11/22 04:25:00, jackhou1 wrote: > > On 2013/11/22 03:29:43, tapted wrote: > > > hm - I think we should try to remove this folder when Chrome exits. There > may > > > even be things set up to complain in buildbots if there is stuff lying > around > > > after a clean exit. > > > > > > Also you might be able to reduce the branching or make things more symmetric > > by > > > going straight to `CreateDirectory` in the first condition rather than > > > `DirectoryExists`. > > > > I'm just deleting the directory on shutdown if we were the one to create it. > Is > > it ok to delete it on the UI thread if it's during shutdown? If not, is there > a > > way to have it delete on the file thread? > > I'd probably delete it unconditionally - otherwise any Chrome crash would put it > there and it would never go away again. > > For deleting on the UI thread... I don't actually know - does a post task not > get flushed? void IPC::ChannelFactory::Close() kinda gets around it just by > doing ::unlink so doesn't get checked by thread_restrictions. Done. I thought PostTask didn't work, but I was doing it wrong. Using PostTask now, also destroying the factory beforehand. https://codereview.chromium.org/66043003/diff/220001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/220001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:58: if (file_util::CreateDirectory(socket_path.DirName())) { On 2013/11/22 05:04:31, tapted wrote: > pretty sure this succeeds if the directory already exists (but it might not be > writable) Done. https://codereview.chromium.org/66043003/diff/220001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:60: } else { On 2013/11/22 05:04:31, tapted wrote: > (so I think this line can be removed) Done.
lgtm, but we should get some security eyes on it https://codereview.chromium.org/66043003/diff/310001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/310001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:64: directory_in_tmp_ = socket_path.DirName(); since this can become "." if socket_path.empty() [("".DirName() == ".")] [even though we "know" that's impossible due to the way base64 works] I guess we should do something here to stop us recursively deleting "." (hypothetically.. like someone substitutes a new implementation of base64?). Perhaps `CHECK(!socket_path.empty());` on the line above. https://codereview.chromium.org/66043003/diff/310001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:67: return; nit: maybe `directory_in_tmp_.clear()` before return?
https://codereview.chromium.org/66043003/diff/310001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/310001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:64: directory_in_tmp_ = socket_path.DirName(); On 2013/11/22 06:50:43, tapted wrote: > since this can become "." if socket_path.empty() [("".DirName() == ".")] [even > though we "know" that's impossible due to the way base64 works] I guess we > should do something here to stop us recursively deleting "." (hypothetically.. > like someone substitutes a new implementation of base64?). > > > Perhaps `CHECK(!socket_path.empty());` on the line above. Done. https://codereview.chromium.org/66043003/diff/310001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:67: return; On 2013/11/22 06:50:43, tapted wrote: > nit: maybe `directory_in_tmp_.clear()` before return? Done.
palmer, could you please do security review
https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:31: base::DeleteFile(path, true); Might this follow symlinks? https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:292: // On failure, try to connect to the long socket path. What's the "long" socket path? Is it the one in $HOME? Why not try that one first? https://codereview.chromium.org/66043003/diff/390001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/390001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:49: if (base::Base64Encode(base::SHA1HashString(user_data_dir.value()), A cryptographic hash of a predictable input is still predictable. What's the goal here? I don't see any enforcement that the pathname does not already exist, and that when created it is owned by the right user and has the right permissions. Doing things in /tmp is dangerous. Can't we do $HOME/.chrome-socket or something? How often is $HOME too long?
I tried to apply this diff locally against a fresh ToT tree, and I get: ~/chromium/src $ git apply ~/Desktop/appshim-tmp.diff error: patch failed: apps/app_shim/app_shim_host_manager_browsertest_mac.mm:83 error: apps/app_shim/app_shim_host_manager_browsertest_mac.mm: patch does not apply ~/chromium/src $ vim apps/app_shim/app_shim_host_manager_browsertest_mac.mm Rebase?
On 2013/12/10 20:23:09, Chromium Palmer wrote: > I tried to apply this diff locally against a fresh ToT tree, and I get: > > ~/chromium/src $ git apply ~/Desktop/appshim-tmp.diff > error: patch failed: apps/app_shim/app_shim_host_manager_browsertest_mac.mm:83 > error: apps/app_shim/app_shim_host_manager_browsertest_mac.mm: patch does not > apply > ~/chromium/src $ vim apps/app_shim/app_shim_host_manager_browsertest_mac.mm > > Rebase? Done.
Thanks for the rebase; works now. This does not LGTM yet, but there's a standard API that will get you there. So, we have a straightforward name-squatting vulnerability: /tmp $ ll [...] drwxrwxrwx 3 nobody wheel 102B Dec 12 13:36 chrome-49MZYW5vt21izDRwSbUwb9R9yg0= /tmp $ ll chrome-49MZYW5vt21izDRwSbUwb9R9yg0\=/ total 0 srwxr-xr-x@ 1 palmer wheel 0B Dec 12 13:36 App Shim Socket So, we'd want to check that the directory has the right owner and permissions before creating a socket in it, but since Mac OS X lacks openat(2) (?!?!), so we can't do my favorite way of doing it. (Which is: open(2) the directory; fstat(2) it; check its ownership and permissions; then if they are good, openat the socket file using the directory FD.) I suggest using https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...: Use mkdtemp with an appropriate name template.
> I suggest using > https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...: > Use mkdtemp with an appropriate name template. Oops, I meant mkstemp; you want to avoid that race.
On 2013/12/12 22:02:49, Chromium Palmer wrote: > > I suggest using > > > https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...: > > Use mkdtemp with an appropriate name template. > > Oops, I meant mkstemp; you want to avoid that race. It looks like mkstemp only creates files. In Patch 11, the directory is deleted first, then created using mkdtemp. Is this okay? Unfortunately, we can't use mkdtemp to generate a random path because the shim needs to be able to find the same path.
So, the goal is to create a path shorter than 128 characters, and we've found
that "/Users/Octavia Butler/Library/Application Support/Google Chrome/Profile
1/App Shim Socket" (or whatever) is often too long. Therefore, we want to use
/tmp/$(hash "Octavia Butler/Library/Application Support/Google Chrome/Profile
1")/"App Shim Socket" to ensure that the path is short enough, and yet also
predictable by the other processes that need to find it.
But, since we can't use an atomic/race-free function like mkstemp (sadly,
mkdtemp doesn't fit the bill, and there's no openat(2) on OS X), I'm still
really anxious about using a world-writable directory like /tmp.
Can we use /Users/Octavia Butler/$(hash "Octavia Butler/Library/Application
Support/Google Chrome/Profile 1")/Shim ? Hopefully, users have short enough user
names?
Alternately, jln suggested getting atomic creation by just doing
mkstemp("/tmp/AppShimXXXXXX"). Then, to solve the predictability problem, there
could be a symlink in "/Users/Octavia Butler/Library/Application Support/Google
Chrome/Profile 1". Processes needing the socket would not open the file through
the symlink; they'd use readlink(2) to discover the true file, open it, then
fstat(2) it to make sure its ownership and permissions were right before using
the file. I hope I am not misrepresenting jln's proposal; CCing him in case I
have.
So, the goal is to create a path shorter than 128 characters, and we've found
that "/Users/Octavia Butler/Library/Application Support/Google Chrome/Profile
1/App Shim Socket" (or whatever) is often too long. Therefore, we want to use
/tmp/$(hash "Octavia Butler/Library/Application Support/Google Chrome/Profile
1")/"App Shim Socket" to ensure that the path is short enough, and yet also
predictable by the other processes that need to find it.
But, since we can't use an atomic/race-free function like mkstemp (sadly,
mkdtemp doesn't fit the bill, and there's no openat(2) on OS X), I'm still
really anxious about using a world-writable directory like /tmp.
Can we use /Users/Octavia Butler/$(hash "Octavia Butler/Library/Application
Support/Google Chrome/Profile 1")/Shim ? Hopefully, users have short enough user
names?
Alternately, jln suggested getting atomic creation by just doing
mkstemp("/tmp/AppShimXXXXXX"). Then, to solve the predictability problem, there
could be a symlink in "/Users/Octavia Butler/Library/Application Support/Google
Chrome/Profile 1". Processes needing the socket would not open the file through
the symlink; they'd use readlink(2) to discover the true file, open it, then
fstat(2) it to make sure its ownership and permissions were right before using
the file. I hope I am not misrepresenting jln's proposal; CCing him in case I
have.
Here's jln, CC'd. I forgot to mention this in my previous comment: I'd really like to keep this in $HOME, if possible, since it's the least tricky path forward.
Just to add some extra tidbits I've collected along the way (don't really have a strong opinion on the security implications ;). w.r.t. likelihood of an error, the length limit is 104 characters (including a null terminator) [see `man 4 unix` on mac]. `getconf LOGIN_NAME_MAX` returns 255 characters on Mac, but when creating an account, the `account name` text field on OSX beeps at me when I get to 80 characters. "/Users/80-char-username/" is 88 characters, so 15 characters remain for a hash in this case. The base64 sha1 hash sits at 28 characters, so we'd need to trim it to support an 80 character username and $HOME. We get even less room if we want to have a leading "." or something descriptive to associate it with Chrome so a user doesn't come along and go "What's this crap in my home folder. Delete." w.r.t. mkdtemp vs mkstemp, I was trying to figure out why there isn't a race-condition-free version of mkdtemp and found http://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html whcih says "The directory created by mkdtemp cannot clash with temporary files or directories created by other users. This is because directory creation always works like open with O_EXCL.". I haven't assumed that this is also true on Mac, or that this avoid all security problems, but it might be a factor (security folks might know better). w.r.t. /tmp vs $HOME, there is already precedent on Mac for non-root sockets being created in /tmp (cf. /var/run for root processes). E.g. when I `ls -l /tmp/launchd-*` I get /tmp/launchd-699.fiAMAl: total 0 srwx------ 1 tapted wheel 0 6 Dec 16:24 sock= That's owned by my `launchd` process. There are also a few permission-denieds, for socket folders belonging to users `_spotlight`, `_securityagent`, and `root`. Again, not claiming there are no other security problems with this (in particular, because we are not currently adding a randomized suffix), but this might be another factor to consider. But then that `readlink` approach (cool idea!) could also give us back the randomized-suffix. Lastly, another (weak) argument against $HOME. Some tools don't expect to find domain socket inodes under $HOME. E.g. http://crbug.com/308676 cropped up when someone tried to pkzip a Chrome profile. There are some reports on the Internet of the socket we currently put under $HOME breaking people's backup scripts too. All considered, my personal non-security-expert opinion is that there are enough things wrong with $HOME, that the actual socket should go under /tmp. It doesn't feel right to pick some arbitrary upper bound on the user name length if it is less than 80 characters, which we would need to do if we wanted to make the socket filename descriptive in any way.
I don't think it's possible to use mkstemp to create a socket. bind(2) fails if the file already exists. IIUC, mkdtemp returns false if it did not create the directory. Would it be correct to assume that, if it did create the directory, a socket in that directory is safe to use? If so, I think the only concern is that another process could repeatedly create that directory and prevent chrome from creating it. To work around that, could we use mkdtemp with "/tmp/chrome-<hash>.XXXXXX" and put a symlink in the UDD pointing to that?
Well, let's go with requiring mkdtemp to succeed (or to have succeeded and then ensuring that the directory is still owned by the user and still mode 0700), and then assuming that only a high-privilege local user can abuse the race condition between creation of the directory and the creation of files/directories/sockets inside it.
tapted, PTAL
Make sure you update the CL description. Also, there might be some comments from palmer@ in Patch Set 8 that need a double-check. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:220: // Test where a symlink already exists in the UDD. nit: maybe expand `UDD` -> `user data dir` - I think I've only ever seen `udd` in review comments ;) https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:243: // Test that existing symlinks are deleted. Can this be double-checked in ~AppShimHostManagerBrowserTest() like `EXPECT_FALSE(base::PathExists(..))` or similar? (You might need to expose something in the TestApi) https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:7: #include <unistd.h> (remind me..) what's unistd for? I think mkdtemp is in stdlib.h, but I'm not in front of a Mac right now. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:40: .Append("chrome-" + udd_hash + ".XXXXXX"); nit: pretty sure this will fit on the line above https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:43: void DeleteFile(base::FilePath path) { nit: const reference for |path|? https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:65: base::Bind(&DeleteFile, directory_in_tmp_)); We should delete the symlink in the user data dir too, so we don't leave it hanging. Maybe a new (static!) function taking two arguments (the symlink could probably be derived again from PathService, but feels safer to have it passed in since this happens during shutdown. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:80: // |mkdtemp| replaces trailing X's randomly and creates the directory. nit: |mkdtemp| -> mkdtemp(). https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:81: if (!mkdtemp(const_cast<char*>(directory_string.c_str()))) nit: if you use directory_string.data() (or &directory_string[0] if we still have an ancient compiler that doesn't have .data() yet), you should be able to avoid the const_cast https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:82: return; Maybe `PLOG(ERROR) << directory_string` before this? Seems like an appropriate case for logging. Otherwise, perhaps some comment, since it's not too clear that a failure here is an unlikely system error we don't really have control over. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:88: base::FILE_PERMISSION_USER_MASK != dir_mode) { nit: I'd maybe swap dir_mode and FILE_PERMISSION_USER_MASK to avoid yoda-style (which tends to only get used for things like CHECK_EQ). (and.. somewhat weird checking equality against something called FOO_MASK rather than using it as a mask, but I think it's fine here). https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:89: return; This could perhaps have a NOTREACHED() before it -- mkdtemp should have already returned NULL unless "something weird" happened. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:97: // Create a symlink to the socket in the UDD. This is a bit subtle - maybe a wordier comment, like "This lets the shim process started from Finder find the actual socket path by following the symlink with ::readlink()" (also nit: UDD -> user data dir.) https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:206: CreateChannelAndSendLaunchApp(symlink_path); nit: maybe a comment here: ~"if the path in the user data dir is not a symlink, try connecting directly" https://codereview.chromium.org/66043003/diff/520001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/520001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:52: ~(base::FILE_PERMISSION_WRITE_BY_OTHERS); ugh - why does this enum exist.. The comment says "Bits ans [sic] masks of the file permission". I'd find CHECK_EQ(0755, socket_mode) a lot more readable - maybe I'm just old school :p. But also, are we sure this is guaranteed? If you run `umask 0007` from Terminal, then run Chrome with the full path to the binary in the app bundle, will it fail? -- inheriting that umask might make the mode 0750 instead.
tapted, PTAL palmer, sorry I missed some comments from earlier, added replies here. https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:31: base::DeleteFile(path, true); On 2013/12/10 20:04:23, Chromium Palmer wrote: > Might this follow symlinks? The comments in file_util.h says this does not follow symlinks. https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/390001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:292: // On failure, try to connect to the long socket path. On 2013/12/10 20:04:23, Chromium Palmer wrote: > What's the "long" socket path? Is it the one in $HOME? Why not try that one > first? The new implementation decides based on whether the one in $HOME is a symlink. https://codereview.chromium.org/66043003/diff/390001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/390001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:49: if (base::Base64Encode(base::SHA1HashString(user_data_dir.value()), On 2013/12/10 20:04:23, Chromium Palmer wrote: > A cryptographic hash of a predictable input is still predictable. What's the > goal here? The new implementation no longer requires it to be predictable, but I've still used a hash to get generate the template passed into mkdtemp as it makes the folders easier to identify. I'm happy to leave out the hash if you prefer. > I don't see any enforcement that the pathname does not already exist, and that > when created it is owned by the right user and has the right permissions. The new implementation checks that the sockets parent directory is writable by the current process, and has mode 0700. Is that sufficient? The permissions on the socket is affected by umask (see tapted's comment in app_mode_common.mm). https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:220: // Test where a symlink already exists in the UDD. On 2014/01/03 13:06:54, tapted wrote: > nit: maybe expand `UDD` -> `user data dir` - I think I've only ever seen `udd` > in review comments ;) Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:243: // Test that existing symlinks are deleted. On 2014/01/03 13:06:54, tapted wrote: > Can this be double-checked in ~AppShimHostManagerBrowserTest() like > `EXPECT_FALSE(base::PathExists(..))` or similar? (You might need to expose > something in the TestApi) This checks that an existing file at the symlink path gets deleted and replaced with the symlink. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:7: #include <unistd.h> On 2014/01/03 13:06:54, tapted wrote: > (remind me..) what's unistd for? I think mkdtemp is in stdlib.h, but I'm not in > front of a Mac right now. The man page says to include unistd.h (unless I'm reading it wrong). https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag... https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:40: .Append("chrome-" + udd_hash + ".XXXXXX"); On 2014/01/03 13:06:54, tapted wrote: > nit: pretty sure this will fit on the line above Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:43: void DeleteFile(base::FilePath path) { On 2014/01/03 13:06:54, tapted wrote: > nit: const reference for |path|? Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:65: base::Bind(&DeleteFile, directory_in_tmp_)); On 2014/01/03 13:06:54, tapted wrote: > We should delete the symlink in the user data dir too, so we don't leave it > hanging. > > Maybe a new (static!) function taking two arguments (the symlink could probably > be derived again from PathService, but feels safer to have it passed in since > this happens during shutdown. Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:80: // |mkdtemp| replaces trailing X's randomly and creates the directory. On 2014/01/03 13:06:54, tapted wrote: > nit: |mkdtemp| -> mkdtemp(). Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:81: if (!mkdtemp(const_cast<char*>(directory_string.c_str()))) On 2014/01/03 13:06:54, tapted wrote: > nit: if you use directory_string.data() (or &directory_string[0] if we still > have an ancient compiler that doesn't have .data() yet), you should be able to > avoid the const_cast Using &directory_string[0] as .data() also returns a const char*. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:82: return; On 2014/01/03 13:06:54, tapted wrote: > Maybe `PLOG(ERROR) << directory_string` before this? Seems like an appropriate > case for logging. Otherwise, perhaps some comment, since it's not too clear that > a failure here is an unlikely system error we don't really have control over. Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:88: base::FILE_PERMISSION_USER_MASK != dir_mode) { On 2014/01/03 13:06:54, tapted wrote: > nit: I'd maybe swap dir_mode and FILE_PERMISSION_USER_MASK to avoid yoda-style > (which tends to only get used for things like CHECK_EQ). (and.. somewhat weird > checking equality against something called FOO_MASK rather than using it as a > mask, but I think it's fine here). Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:89: return; On 2014/01/03 13:06:54, tapted wrote: > This could perhaps have a NOTREACHED() before it -- mkdtemp should have already > returned NULL unless "something weird" happened. Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:97: // Create a symlink to the socket in the UDD. On 2014/01/03 13:06:54, tapted wrote: > This is a bit subtle - maybe a wordier comment, like "This lets the shim process > started from Finder find the actual socket path by following the symlink with > ::readlink()" (also nit: UDD -> user data dir.) Done. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:206: CreateChannelAndSendLaunchApp(symlink_path); On 2014/01/03 13:06:54, tapted wrote: > nit: maybe a comment here: ~"if the path in the user data dir is not a symlink, > try connecting directly" Done. https://codereview.chromium.org/66043003/diff/520001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/520001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:52: ~(base::FILE_PERMISSION_WRITE_BY_OTHERS); On 2014/01/03 13:06:54, tapted wrote: > ugh - why does this enum exist.. The comment says "Bits ans [sic] masks of the > file permission". I'd find CHECK_EQ(0755, socket_mode) a lot more readable - > maybe I'm just old school :p. But also, are we sure this is guaranteed? If you > run `umask 0007` from Terminal, then run Chrome with the full path to the binary > in the app bundle, will it fail? -- inheriting that umask might make the mode > 0750 instead. Done. Also you're right, the sockets permissions depend on umask. However, the directory is always created with 0700. Would we need to set the mode of the socket explicitly?
https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:243: // Test that existing symlinks are deleted. On 2014/01/06 05:29:52, jackhou1 wrote: > On 2014/01/03 13:06:54, tapted wrote: > > Can this be double-checked in ~AppShimHostManagerBrowserTest() like > > `EXPECT_FALSE(base::PathExists(..))` or similar? (You might need to expose > > something in the TestApi) > > This checks that an existing file at the symlink path gets deleted and replaced > with the symlink. Is it possible to have a regression test for Chrome deleting all these temporaries on shutdown too? If it crashes, we are out of luck, but otherwise leaving cruft in /tmp would be a pretty annoying bug to have. https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:7: #include <unistd.h> On 2014/01/06 05:29:52, jackhou1 wrote: > On 2014/01/03 13:06:54, tapted wrote: > > (remind me..) what's unistd for? I think mkdtemp is in stdlib.h, but I'm not > in > > front of a Mac right now. > > The man page says to include unistd.h (unless I'm reading it wrong). > https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag... right you are - must be a BSD thing :/ https://codereview.chromium.org/66043003/diff/520001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/520001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:52: ~(base::FILE_PERMISSION_WRITE_BY_OTHERS); On 2014/01/06 05:29:52, jackhou1 wrote: > On 2014/01/03 13:06:54, tapted wrote: > > ugh - why does this enum exist.. The comment says "Bits ans [sic] masks of the > > file permission". I'd find CHECK_EQ(0755, socket_mode) a lot more readable - > > maybe I'm just old school :p. But also, are we sure this is guaranteed? If you > > run `umask 0007` from Terminal, then run Chrome with the full path to the > binary > > in the app bundle, will it fail? -- inheriting that umask might make the mode > > 0750 instead. > > Done. > > Also you're right, the sockets permissions depend on umask. However, the > directory is always created with 0700. Would we need to set the mode of the > socket explicitly? I don't think we need to set the socket inode permissions separately (parent folder in mode 0700 should be sufficient), but the security folks will know better. https://codereview.chromium.org/66043003/diff/620001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/620001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:114: // This may be called multiple times at startup to try to connect to different nit: I don't think this second sentence is true any more - can just drop it. https://codereview.chromium.org/66043003/diff/620001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.h (right): https://codereview.chromium.org/66043003/diff/620001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.h:131: // and are owned by the user. actually, it's not checking ownership (yet?). https://codereview.chromium.org/66043003/diff/620001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/620001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:46: CHECK(base::PathIsWritable(socket_path)); You might want something stronger than this, but I'm not sure what - security folks might know for sure.. E.g. there is file_util::VerifyPathControlledByUser(..), but nothing seems to use it. Maybe the randomness of mkdtemp is enough.. But a directory squatter could still make a world-writable folder in /tmp
https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/520001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:243: // Test that existing symlinks are deleted. On 2014/01/06 07:31:28, tapted wrote: > On 2014/01/06 05:29:52, jackhou1 wrote: > > On 2014/01/03 13:06:54, tapted wrote: > > > Can this be double-checked in ~AppShimHostManagerBrowserTest() like > > > `EXPECT_FALSE(base::PathExists(..))` or similar? (You might need to expose > > > something in the TestApi) > > > > This checks that an existing file at the symlink path gets deleted and > replaced > > with the symlink. > > Is it possible to have a regression test for Chrome deleting all these > temporaries on shutdown too? If it crashes, we are out of luck, but otherwise > leaving cruft in /tmp would be a pretty annoying bug to have. Done. https://codereview.chromium.org/66043003/diff/620001/apps/app_shim/chrome_mai... File apps/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/66043003/diff/620001/apps/app_shim/chrome_mai... apps/app_shim/chrome_main_app_mode_mac.mm:114: // This may be called multiple times at startup to try to connect to different On 2014/01/06 07:31:28, tapted wrote: > nit: I don't think this second sentence is true any more - can just drop it. Done. https://codereview.chromium.org/66043003/diff/620001/chrome/common/mac/app_mo... File chrome/common/mac/app_mode_common.mm (right): https://codereview.chromium.org/66043003/diff/620001/chrome/common/mac/app_mo... chrome/common/mac/app_mode_common.mm:46: CHECK(base::PathIsWritable(socket_path)); On 2014/01/06 07:31:28, tapted wrote: > You might want something stronger than this, but I'm not sure what - security > folks might know for sure.. E.g. there is > file_util::VerifyPathControlledByUser(..), but nothing seems to use it. > > Maybe the randomness of mkdtemp is enough.. But a directory squatter could still > make a world-writable folder in /tmp We'll see what the security folks think. I suspect VerifyPathControlledByUser can only be used to check the parent directory anyway, since /tmp is world-writable, and the socket might be writeable by others if the umask is not 0022.
nice - lgtm (% any concerns the security folks might raise)
palmer, PTAL
Ping.
On 2014/01/14 03:14:55, jackhou1 wrote: > Ping. Oops, I forgot to address some comments jln made in the email: > Are you aware of the issues outlined at https://b.corp.google.com/issue?id=4366214 ? > The TL;DR is: > - When you create a file in /tmp, make sure you don't follow symlinks (i.e. don't create /tmp/XXXXXXX, because you could end up creating /path/to/user/foo/list_of_script_to_run/blah I think this only applies to following symlinks inside of /tmp? Since /tmp is itself a symlink to /private/tmp on OSX. In this CL we only create a directory directly under /tmp. > - When you create a file in /tmp, make sure it doesn't already exist (O_EXCL is your friend). According to the documentation tapted mentioned (http://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html), mkdtemp cannot clash with an existing file or directory.
On 2014/01/14 06:12:35, jackhou1 wrote: > On 2014/01/14 03:14:55, jackhou1 wrote: > > Ping. > > Oops, I forgot to address some comments jln made in the email: > > > Are you aware of the issues outlined at > https://b.corp.google.com/issue?id=4366214 ? > > The TL;DR is: > > - When you create a file in /tmp, make sure you don't follow symlinks (i.e. > don't create /tmp/XXXXXXX, because you could end up creating > /path/to/user/foo/list_of_script_to_run/blah > > I think this only applies to following symlinks inside of /tmp? Since /tmp is > itself a symlink to /private/tmp on OSX. In this CL we only create a directory > directly under /tmp. > > > - When you create a file in /tmp, make sure it doesn't already exist (O_EXCL > is your friend). > > According to the documentation tapted mentioned > (http://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html), > mkdtemp cannot clash with an existing file or directory. It looks like you're using mkdtemp, so indeed you're fine. mkdtemp uses O_EXCL | O_CREAT. Since, O_EXCL | O_CREAT implies O_NOFOLLOW, this is ok. I didn't do a careful review, so let me know if you're creating / touching files in /tmp without mkdtemp. For the record, even mkdtemp is not safe on Linux because of /tmp cleaners. This shouldn't be an issue on OSX.
> It looks like you're using mkdtemp, so indeed you're fine. mkdtemp uses O_EXCL | > O_CREAT. Since, O_EXCL | O_CREAT implies O_NOFOLLOW, this is ok. I'm trusting jln's superior knowledges on this, so LGTM. Note in that the internal bug for another issue, jln suggested shm_open for this type of problem.
On 2014/01/24 03:07:34, Chromium Palmer wrote: > > It looks like you're using mkdtemp, so indeed you're fine. mkdtemp uses O_EXCL > | > > O_CREAT. Since, O_EXCL | O_CREAT implies O_NOFOLLOW, this is ok. > > I'm trusting jln's superior knowledges on this, so LGTM. Note in that the > internal bug for another issue, jln suggested shm_open for this type of problem. Modulo OSX doing anything funky of course. 2013, temporary files are still a hard problem :)
Thanks for the reviews!
thakis, please review for OWNERS in chrome/common/mac/
https://codereview.chromium.org/66043003/diff/860001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/860001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:39: return base::FilePath("/tmp").Append("chrome-" + udd_hash + ".XXXXXX"); This doesn't lgtm. Temporary files should go in PathService::Get(DIR_TEMP), or somewhere in ~/Library/Applications. Why does the path have to be 100+ characters long?
tapted, thakis, PTAL It seems fine to use PathService(DIR_TEMP) instead of /tmp. The temp directory appears to have a consistent length, though I haven't found any documentation confirming it. This still creates a new directory with mkdtemp and random name, so I think the security implications are unchanged. https://codereview.chromium.org/66043003/diff/860001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/860001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:39: return base::FilePath("/tmp").Append("chrome-" + udd_hash + ".XXXXXX"); On 2014/01/24 16:49:18, Nico wrote: > This doesn't lgtm. Temporary files should go in PathService::Get(DIR_TEMP), or > somewhere in ~/Library/Applications. > > Why does the path have to be 100+ characters long? This contained "chrome" and a hash of the UDD to make the folder easy to identify in /tmp. Now that its using PathService(DIR_TEMP), this is not really relevant.
https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:241: EXPECT_TRUE(base::CreateSymbolicLink(base::FilePath("/tmp"), symlink_path_)); does this need to be updated? https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:36: DCHECK(PathService::Get(base::DIR_TEMP, &temp_dir)); DCHECK->CHECK https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:39: DCHECK(temp_dir.value().length() < 90); nit: DCHECK_GT(90, ..) might be the preferred way to do this https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:40: return temp_dir.Append("XXXXXX"); How many characters are spare with the lengths we've been seeing for NSTemporaryDirectory and the shorter socket inode? It might be good to use 'chrome-XXXXXX' or similar for this, just so the folder owner is identifiable.
https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_browsertest_mac.mm (right): https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_browsertest_mac.mm:241: EXPECT_TRUE(base::CreateSymbolicLink(base::FilePath("/tmp"), symlink_path_)); On 2014/01/31 07:04:12, tapted wrote: > does this need to be updated? Done. https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... File apps/app_shim/app_shim_host_manager_mac.mm (right): https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:36: DCHECK(PathService::Get(base::DIR_TEMP, &temp_dir)); On 2014/01/31 07:04:12, tapted wrote: > DCHECK->CHECK Done. https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:39: DCHECK(temp_dir.value().length() < 90); On 2014/01/31 07:04:12, tapted wrote: > nit: DCHECK_GT(90, ..) might be the preferred way to do this Done. https://codereview.chromium.org/66043003/diff/990001/apps/app_shim/app_shim_h... apps/app_shim/app_shim_host_manager_mac.mm:40: return temp_dir.Append("XXXXXX"); On 2014/01/31 07:04:12, tapted wrote: > How many characters are spare with the lengths we've been seeing for > NSTemporaryDirectory and the shorter socket inode? It might be good to use > 'chrome-XXXXXX' or similar for this, just so the folder owner is identifiable. Done. There's around 40 spare characters.
lgtm although i'm uncertain of what the behaviour should be if `PathService::Get(base::DIR_TEMP` actually fails. Most things calling it seem to pretend that PathService::Get(DIR_TEMP.. returns `void`. Apple docs just say NSTemporaryDirectory fails "If no such directory is currently available" - sandboxed processes maybe.
lgtm On 2014/01/31 08:58:17, tapted wrote: > lgtm although i'm uncertain of what the behaviour should be if > `PathService::Get(base::DIR_TEMP` actually fails. Most things calling it seem to > pretend that PathService::Get(DIR_TEMP.. returns `void`. Apple docs just say > NSTemporaryDirectory fails "If no such directory is currently available" - > sandboxed processes maybe.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/66043003/1060001
Message was sent while issue was closed.
Change committed as 250051 |
