Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17779002:
Posix: fix named SHM mappings permissions. (Closed)
|
Created:
7 years, 6 months ago by jln (very slow on Chromium) Modified:
7 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, gavinp+memory_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPosix: fix named SHM mappings permissions.
Make sure that named mappings in /dev/shm/ aren't created with
broad permissions.
BUG=254159
R=mark@chromium.org, markus@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209814
Patch Set 1 #Patch Set 2 : Switch to proper low level functions. #
Total comments: 4
Patch Set 3 : Check uid if opening existing file. (And rebase). #
Total comments: 33
Patch Set 4 : Address Mark's comments. #
Total comments: 4
Patch Set 5 : Address new comments. #
Total comments: 2
Patch Set 6 : Improve comment. #Messages
Total messages: 19 (0 generated)
Markus: PTAL!
umask() is a per-process property and can't really be used safely from a multi-threaded application. I don't think we should do it this way.
On 2013/06/26 00:34:31, Markus (顧孟勤) wrote: > umask() is a per-process property and can't really be used safely from a > multi-threaded application. I don't think we should do it this way. Ohh right (not the smartest choice in POSIX). I was trying to play nice with the high level functions, but we really need to use the low level API here. PTAL!
https://chromiumcodereview.appspot.com/17779002/diff/5001/base/memory/shared_... File base/memory/shared_memory_posix.cc (right): https://chromiumcodereview.appspot.com/17779002/diff/5001/base/memory/shared_... base/memory/shared_memory_posix.cc:163: fd = open(path.value().c_str(), O_RDWR | O_APPEND, file_mode); Do not pass in file_mode. That's just misleading your reviewers. The mode is ignored by open() unless you also set O_CREAT. Having said that, now might be a good time to call fstat() to verify that the permissions are set the way we would like them to be set. You might even want to check the owner, if you are paranoid. Otherwise, there is a race condition where somebody other than the owner could create a world-writable file and trick you into using it for your shared memory. https://chromiumcodereview.appspot.com/17779002/diff/5001/base/memory/shared_... base/memory/shared_memory_posix.cc:180: if (current_size != options.size) { I realize this isn't code that you wrote, but overall this is really silly. We know with 100% certainty that if we get here, the size of the file is going to be zero. We really should simplify the code and get rid of the call to fstat(), get rid of the comparison for sizes, get rid of "fix_size", and always ftruncate() if open(O_EXCL) succeeded. That would make this function a lot more readable.
PTAL! https://codereview.chromium.org/17779002/diff/5001/base/memory/shared_memory_... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/5001/base/memory/shared_memory_... base/memory/shared_memory_posix.cc:163: fd = open(path.value().c_str(), O_RDWR | O_APPEND, file_mode); On 2013/06/26 12:28:22, Markus (顧孟勤) wrote: > Do not pass in file_mode. That's just misleading your reviewers. > > The mode is ignored by open() unless you also set O_CREAT. > > Having said that, now might be a good time to call fstat() to verify that the > permissions are set the way we would like them to be set. You might even want to > check the owner, if you are paranoid. > > Otherwise, there is a race condition where somebody other than the owner could > create a world-writable file and trick you into using it for your shared memory. Done. https://codereview.chromium.org/17779002/diff/5001/base/memory/shared_memory_... base/memory/shared_memory_posix.cc:180: if (current_size != options.size) { On 2013/06/26 12:28:22, Markus (顧孟勤) wrote: > I realize this isn't code that you wrote, but overall this is really silly. > > We know with 100% certainty that if we get here, the size of the file is going > to be zero. We really should simplify the code and get rid of the call to > fstat(), get rid of the comparison for sizes, get rid of "fix_size", and always > ftruncate() if open(O_EXCL) succeeded. > > That would make this function a lot more readable. Unfortunately no, CreateAndOpenTemporaryShmemFile() is another case where fix_size will be true. I added a comment to make this code a bit more clear.
lgtm https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:171: struct stat stat; Ideally, avoid using "stat" as a local symbol, as it shadows "stat(2)". That's just confusing. I usually use "struct stat sb" (for "stat buf"). I have seen this use in other projects, but obviously there are other -- more verbose -- choices that work just as well.
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:171: struct stat stat; On 2013/07/02 02:49:38, Markus (顧孟勤) wrote: > Ideally, avoid using "stat" as a local symbol, as it shadows "stat(2)". That's > just confusing. > > I usually use "struct stat sb" (for "stat buf"). I have seen this use in other > projects, but obviously there are other -- more verbose -- choices that work > just as well. Yeah, I only used it for consistency with the code below. I agree that it's not a great choice.
Mark: could you approve this CL as base/ owner?
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:9: #include <fcntl.h> Really? Again? https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:12: #include <sys/stat.h> REALLY? https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:155: // Make sure that we don't give permissions to access this file Nit: permission. Nit: avoid “we” (only picking on this because I have something else to pick on on this line). https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:160: fd = open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, owner_only); You can declare fd on this line. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:160: fd = open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, owner_only); HANDLE_EINTR. Same around the open on line 170. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:169: // Checking the former happens below. Former? Below? Ick. Why have you separated the problems from how they’re handled? “Attackers could plant a symbolic link…so use O_NOFOLLOW. Attackers could plant a file…so that’s checked below.” https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:171: struct stat stat; Pick a different name. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { getuid or geteuid? https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:418: mode_t old_umask = umask(S_IWGRP | S_IWOTH); Can you restore the old umask with a scoper, in case these EXPECTs ever become ASSERTs? Same in the next test. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:442: "shared_perm_test-" + Uint64ToString(RandUint64()); Random numbers lead to flake. 64 bits are probably “random enough” but why play around with this? Is there something better you can use? Something you can use in addition? https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:453: EXPECT_EQ(0, fstat(shm_fd, &shm_stat)); If the backing file is gone, what are you testing?
Thanks, PTAL! https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:9: #include <fcntl.h> On 2013/07/02 14:31:54, Mark Mentovai wrote: > Really? Again? Oops! I'll update the presubmit script later to detect these. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:12: #include <sys/stat.h> On 2013/07/02 14:31:54, Mark Mentovai wrote: > REALLY? Done. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:155: // Make sure that we don't give permissions to access this file On 2013/07/02 14:31:54, Mark Mentovai wrote: > Nit: permission. > Nit: avoid “we” (only picking on this because I have something else to pick on > on this line). Done. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:160: fd = open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, owner_only); On 2013/07/02 14:31:54, Mark Mentovai wrote: > You can declare fd on this line. Done. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:160: fd = open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, owner_only); On 2013/07/02 14:31:54, Mark Mentovai wrote: > HANDLE_EINTR. Same around the open on line 170. Done. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:171: struct stat stat; On 2013/07/02 14:31:54, Mark Mentovai wrote: > Pick a different name. Done. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { On 2013/07/02 14:31:54, Mark Mentovai wrote: > getuid or geteuid? I had thought about it and I believe it's slightly more correct to use the uid. The euid can be changed to get more (setuid binary) or less (root program) privileges in the fsuid (essentialy the euid), but this check really is about who the current user is. This being said, if fsuid != uid, this API should probably be rethought and not used. I'm happy to add a CHECK_EQ(getuid(), geteuid()). Let me know what you think. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:418: mode_t old_umask = umask(S_IWGRP | S_IWOTH); On 2013/07/02 14:31:54, Mark Mentovai wrote: > Can you restore the old umask with a scoper, in case these EXPECTs ever become > ASSERTs? Same in the next test. I ended up created a class for this, since it's less verbose than using the ScopedClosureRunner. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:442: "shared_perm_test-" + Uint64ToString(RandUint64()); On 2013/07/02 14:31:54, Mark Mentovai wrote: > Random numbers lead to flake. 64 bits are probably “random enough” but why play > around with this? Is there something better you can use? Something you can use > in addition? I think this is the best way to do it. We're creating a file in a "temp" directory and for the coverage of this particular test, we need that file to not exist. Paradoxically, using a random file name like this leads to a much more predictable run than playing around with incremental file names or figuring out whether or not a test-specific name is already used. Especially if you take into account factors such as: - Another instance of the test running concurrently on the machine. - Different user ids creating files in the temporary directory at the same time. I was tempted to check if the path already existed, but then the test would need to have knowledge of the construction of the temporary path. Let me know what you think! https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:453: EXPECT_EQ(0, fstat(shm_fd, &shm_stat)); On 2013/07/02 14:31:54, Mark Mentovai wrote: > If the backing file is gone, what are you testing? The link is gone, but the inode is still there and will only be deleted by the test once it's closed (which in this case will happen once shared_memory is destroyed). I clarified the comment above when deleting the link.
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { Julien Tinnes wrote: > On 2013/07/02 14:31:54, Mark Mentovai wrote: > > getuid or geteuid? > > I had thought about it and I believe it's slightly more correct to use the uid. > > The euid can be changed to get more (setuid binary) or less (root program) > privileges in the fsuid (essentialy the euid), but this check really is about > who the current user is. > > This being said, if fsuid != uid, this API should probably be rethought and not > used. I'm happy to add a CHECK_EQ(getuid(), geteuid()). > > Let me know what you think. I think that this should match the user that would show up as the owner of the file if it had been created by the open call above. Isn’t the fsuid normally the euid? Since you’re assuming that a file created by open with O_CREAT is OK, don’t you want to assure yourself of the same properties if you’re recycling an existing file? https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:442: "shared_perm_test-" + Uint64ToString(RandUint64()); Julien Tinnes wrote: > On 2013/07/02 14:31:54, Mark Mentovai wrote: > > Random numbers lead to flake. 64 bits are probably “random enough” but > why play > > around with this? Is there something better you can use? Something you can use > > in addition? > > I think this is the best way to do it. We're creating a file in a "temp" > directory and for the coverage of this particular test, we need that file to not > exist. > > Paradoxically, using a random file name like this leads to a much more > predictable run than playing around with incremental file names or figuring out > whether or not a test-specific name is already used. > > Especially if you take into account factors such as: > - Another instance of the test running concurrently on the machine. > - Different user ids creating files in the temporary directory at the same time. > > I was tempted to check if the path already existed, but then the test would need > to have knowledge of the construction of the temporary path. > > Let me know what you think! You’re right, I don’t think you should be poking around on the FS. I think you should at least add the pid to the random number, or the pid, time(), and a random number. The other alternative is to use whatever our mkdtemp equivalent is to get your own unique temporary directory, and then stick the file in there. But that’s probably cumbersome because we don’t have full control of the entire path here. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:453: EXPECT_EQ(0, fstat(shm_fd, &shm_stat)); Julien Tinnes wrote: > On 2013/07/02 14:31:54, Mark Mentovai wrote: > > If the backing file is gone, what are you testing? > > The link is gone, but the inode is still there and will only be deleted by the > test once it's closed (which in this case will happen once shared_memory is > destroyed). > I clarified the comment above when deleting the link. Right, but I mean: why are the permissions relevant at that point? What interface can someone use to get an FD to the backing file if they didn’t have one already if we goofed and the permission bits are wrong? Is it /proc/*/fd or something? https://codereview.chromium.org/17779002/diff/32001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/32001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:156: // First, try to create the file. Blank line before this. https://codereview.chromium.org/17779002/diff/32001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:172: // Check that the current user owns the file. And this too. When you have comments interspersed with code like this, you’re breaking things up into conceptual blocks, and the whitespace in between really helps a reader jump to the comment, see what’s going on, and find the breaks between those blocks. But I’d actually declare sb after the comment, since sb’s there for the fstat, and the comment applies to both of them.
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { On 2013/07/02 19:52:56, Mark Mentovai wrote: > Julien Tinnes wrote: > > On 2013/07/02 14:31:54, Mark Mentovai wrote: > > > getuid or geteuid? > > > > I had thought about it and I believe it's slightly more correct to use the > uid. > > > > The euid can be changed to get more (setuid binary) or less (root program) > > privileges in the fsuid (essentialy the euid), but this check really is about > > who the current user is. > > > > This being said, if fsuid != uid, this API should probably be rethought and > not > > used. I'm happy to add a CHECK_EQ(getuid(), geteuid()). > > > > Let me know what you think. > > I think that this should match the user that would show up as the owner of the > file if it had been created by the open call above. Isn’t the fsuid normally the > euid? Since you’re assuming that a file created by open with O_CREAT is OK, > don’t you want to assure yourself of the same properties if you’re recycling an > existing file? The O_CREAT is O_CREAT | O_EXCL: i.e. we guaranteed that we're not opening an existing file. If you think about this code being used, for instance, in a setuid root binary - a canonical case of euid != uid -, one definitely would want to check for the file matching the uid, not the euid. It guarantees security insofar as we're not opening a "privileged" file. Another way to think of this: the uid check works if multiple users are using this same setuid binary concurrently, but a euid check wouldn't. I think the root cause of confusion is that opening an existing file in a shared temp directory is an anti-pattern. It definitely shouldn't be attempted when uid != euid. So I'm now checking for uid != euid as well, let me know if this works. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:442: "shared_perm_test-" + Uint64ToString(RandUint64()); On 2013/07/02 19:52:56, Mark Mentovai wrote: > Julien Tinnes wrote: > > On 2013/07/02 14:31:54, Mark Mentovai wrote: > > > Random numbers lead to flake. 64 bits are probably “random enough” but > > why play > > > around with this? Is there something better you can use? Something you can > use > > > in addition? > > > > I think this is the best way to do it. We're creating a file in a "temp" > > directory and for the coverage of this particular test, we need that file to > not > > exist. > > > > Paradoxically, using a random file name like this leads to a much more > > predictable run than playing around with incremental file names or figuring > out > > whether or not a test-specific name is already used. > > > > Especially if you take into account factors such as: > > - Another instance of the test running concurrently on the machine. > > - Different user ids creating files in the temporary directory at the same > time. > > > > I was tempted to check if the path already existed, but then the test would > need > > to have knowledge of the construction of the temporary path. > > > > Let me know what you think! > > You’re right, I don’t think you should be poking around on the FS. > > I think you should at least add the pid to the random number, or the pid, > time(), and a random number. > > The other alternative is to use whatever our mkdtemp equivalent is to get your > own unique temporary directory, and then stick the file in there. But that’s > probably cumbersome because we don’t have full control of the entire path here. I added the PID. https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:453: EXPECT_EQ(0, fstat(shm_fd, &shm_stat)); On 2013/07/02 19:52:56, Mark Mentovai wrote: > Julien Tinnes wrote: > > On 2013/07/02 14:31:54, Mark Mentovai wrote: > > > If the backing file is gone, what are you testing? > > > > The link is gone, but the inode is still there and will only be deleted by the > > test once it's closed (which in this case will happen once shared_memory is > > destroyed). > > I clarified the comment above when deleting the link. > > Right, but I mean: why are the permissions relevant at that point? What > interface can someone use to get an FD to the backing file if they didn’t have > one already if we goofed and the permission bits are wrong? Is it /proc/*/fd or > something? We're checking that the file was created with the right permissions. I'm deleting the file name as early as possible because it's easier and more readable than to add a scoper, but this shouldn't affect the code that follows Delete() in any way. Let me know if I can make something more clear. https://codereview.chromium.org/17779002/diff/32001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/32001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:156: // First, try to create the file. On 2013/07/02 19:52:56, Mark Mentovai wrote: > Blank line before this. Done. https://codereview.chromium.org/17779002/diff/32001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:172: // Check that the current user owns the file. On 2013/07/02 19:52:56, Mark Mentovai wrote: > And this too. When you have comments interspersed with code like this, you’re > breaking things up into conceptual blocks, and the whitespace in between really > helps a reader jump to the comment, see what’s going on, and find the breaks > between those blocks. > > But I’d actually declare sb after the comment, since sb’s there for the fstat, > and the comment applies to both of them. Done.
LGTM https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { Julien Tinnes wrote: > If you think about this code being used, for instance, in a setuid root binary - > a canonical case of euid != uid -, one definitely would want to check for the > file matching the uid, not the euid. It guarantees security insofar as we're not > opening a "privileged" file. Why is “root” the only example? What mark ran something that was setuid jln? Should the program have access to mark’s files or jln’s in this case? Should it create files as mark or jln? Should mark or jln be able to read its memory? > I think the root cause of confusion is that opening an existing file in a shared > temp directory is an anti-pattern. It definitely shouldn't be attempted when uid > != euid. So I'm now checking for uid != euid as well, let me know if this works. That’s fine. https://codereview.chromium.org/17779002/diff/39001/base/memory/shared_memory... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/17779002/diff/39001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:468: // Neither the group, nor others should be able to read the shared memory Then this comment should be “should have been able to read the shared memory file while it existed on disk”
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { On 2013/07/02 21:27:59, Mark Mentovai wrote: > Julien Tinnes wrote: > > If you think about this code being used, for instance, in a setuid root binary > - > > a canonical case of euid != uid -, one definitely would want to check for the > > file matching the uid, not the euid. It guarantees security insofar as we're > not > > opening a "privileged" file. > > Why is “root” the only example? What mark ran something that was setuid jln? > Should the program have access to mark’s files or jln’s in this case? Should it > create files as mark or jln? Should mark or jln be able to read its memory? Yes, I think my example still works. If there is a setuid mark program available, mark made it available for other users, such as jln to perform some actions with more privileges. Let's say append lines to /home/mark/list_of_reviews_to_do. The setuid program's resources should still be private, to be able to guarantee its integrity. That's why the OS won't let jln debug that setuid binary or even read its memory. So if the setuid program is creating a new shared memory resource, it should be create with mark's uid. But if the setuid program needs to open an existing shared memory resource the safe default is to assume that it doesn't want to use it's additional ambiant authority (as mark) to do so: jln is running the program, it's ok to read jln's X shared memory to access the clipboard, but not mark's. So I think "real uid" is a slightly better default. In any case, I think we both agree that uid != euid is a subtle security model that would require a different API. https://codereview.chromium.org/17779002/diff/39001/base/memory/shared_memory... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/17779002/diff/39001/base/memory/shared_memory... base/memory/shared_memory_unittest.cc:468: // Neither the group, nor others should be able to read the shared memory On 2013/07/02 21:27:59, Mark Mentovai wrote: > Then this comment should be > > “should have been able to read the shared memory file while it existed on disk” Done.
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { Julien Tinnes wrote: > But if the setuid program needs to open an existing shared memory resource the > safe default is to assume that it doesn't want to use it's additional ambiant > authority (as mark) to do so: jln is running the program, it's ok to read jln's > X shared memory to access the clipboard, but not mark's. You’re assuming that the program is supposed to append to /home/mark/list_of_reviews_to_do and not…well, open some of mark’s shared memory. Neither of these two options seems any more likely than the other. I don’t see why should the security model should be different if you created the resource as opposed to if you accessed a resource that already existed. If the resource doesn’t exist already, a setuid mark program that creates it gives mark more access than he would have had if it already existed, and forbids jln that same access. Regardless, this is all moot now, it’s fine with you guaranteeing that the euid and real UID are identical.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/17779002/46001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/17779002/diff/17001/base/memory/shared_memory... base/memory/shared_memory_posix.cc:174: (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) { On 2013/07/02 22:05:27, Mark Mentovai wrote: > Julien Tinnes wrote: > > But if the setuid program needs to open an existing shared memory resource the > > safe default is to assume that it doesn't want to use it's additional ambiant > > authority (as mark) to do so: jln is running the program, it's ok to read > jln's > > X shared memory to access the clipboard, but not mark's. > > You’re assuming that the program is supposed to append to > /home/mark/list_of_reviews_to_do and not…well, open some of mark’s shared > memory. Neither of these two options seems any more likely than the other. > > I don’t see why should the security model should be different if you created the > resource as opposed to if you accessed a resource that already existed. If the > resource doesn’t exist already, a setuid mark program that creates it gives mark > more access than he would have had if it already existed, and forbids jln that > same access. Because if creating a new resource mark is not put at any risk. The setuid program has been made available by mark to give jln a bit more privileges. It's normal that the security model is skewed in Mark's favor by default. From mark's (who made the setuid binary available) point of view: - If creating a new file: integrity is not an issue (there is nothing to trash), by using the euid when creating the file we're guaranteeing confidentiality. - If opening an existing file: a. checking euid risks violating integrity and confidentiality (jln who runs the program can end-up writing to that file or reading from that file. b. checking uid risks violating confidentiality (write private stuff to a jln-owned file) In addition, and it's perhaps a better argument, the euid is kind of a hack on top of an existing POSIX API. It's basically: let's change this internal uid that the kernel will actually use and all the existing APIs can still be used. So it's not typical to check for euid. Most checks for euid in open source software are actually wrong, they should almost always be replaced by checks for whether or not a resource is accessible (on Linux they break the new file capabilities model for instance). > Regardless, this is all moot now, it’s fine with you guaranteeing that the euid > and real UID are identical. Yes, it's much better that way. This long discussion proves it ;)
Message was sent while issue was closed.
Committed patchset #6 manually as r209814 (presubmit successful). |
