Putting QtWebProcess into a chrooted sandbox
Created attachment 149593 [details] Completely incomplete, just to get feedback :) This is a preliminary patch for sandboxing the QtWebProcess. In this approach WebProcess is started in a separated chroot. The chroot environment is generated in runtime (into /mnt/wk-sandbox). Currently most of the needed directories are simply bound (mount --rbind) into the appropriate place in the sandbox. These directories are enumerated in a constant list. Later, this will be replaced with a dynamic generation process what will adapt the libraries to an arbitrary target. Furthermore, instead of binding, we will be able to use hard links (this way the clean-up of the chroot env will also be easy). I already have a function that carries out an ldd-like functionality and also follows symbolic links, but this is an incomplete solution yet so I don't upload it this time. This approach uses the standard chroot functionality of linux what needs superuser rights. To avoid running MiniBrowser with sudo we can modify the installation process by changing the owner of MiniBrowser to root and setting the effective userid to the owner. This way MiniBrowser will run with superuser rights until the creation of WebProcess. At this point we perform the chrooting and afterwards we don't need special rigths anymore so we change back to the real user (who is actually logged in) or to the "nobody" user (this needs further investigation, since nobody doesn't have a home directory but some of the resources (e.g. caches) are often stored there. Any comments or feedback are much appreciated! :)
(In reply to comment #1) > This approach uses the standard chroot functionality of linux what needs superuser rights. To avoid running MiniBrowser with sudo we can modify the installation process by changing the owner of MiniBrowser to root and setting the effective userid to the owner. This way MiniBrowser will run with superuser rights until the creation of WebProcess. At this point we perform the chrooting and afterwards we don't need special rigths anymore so we change back to the real user (who is actually logged in) or to the "nobody" user (this needs further investigation, since nobody doesn't have a home directory but some of the resources (e.g. caches) are often stored there. I think the traditional approach to dealing with superuser rights in this case would be to make itself QtWebProcess suid root, make it set up the chroot as the first thing and then drop down to nobody. However I'm not sure how well that approach plays together with the pile of C++ code we have that might end up executing global constructors before we reach the part in main() where we can chroot and drop privileges.
Just a quick comment. I think using chroot sandbox is a kind of deprecated pattern to sandbox processes although it is easy to implement. It also adds a new security concern for distributions since it adds a SUID binary. Seccomp filters is the approach to which rest of the world is moving into.
Is anyone familiar with how Chromium implements sandboxing on Unix? With my FreeBSD packager hat on, I'd really appreciate it if the actual implementation was not heavily tied to Linux stuff (the chroot approach probably works on other Unix flavors as well, for example). I also remember Google funded/funds research on Capsicum for this kind of sandboxing capabilities, but I dunno how far things are on Linux.
Hi Raphael, Linux sandbox in Chrome uses two sandboxing methods in Linux, which can be also stacked: - Chroot sandbox that also uses Linux namespaces so that renderer process cannot espace from the chroot cage. - Seccomp sandbox that prevents malicious system calls to be called from the renderer process. If I was implementing webkit 2 Linux sandbox, I would aim for solution that would be only based seccomp filters in order to create simple and maintainable solution. Chroot sandbox in Chrome is ugly and complex beast that requires SUID enabled wrapper program. At this point, at least in the mainline kernel, there's no Capsicum for Linux.
(In reply to comment #5) > Hi Raphael, > > Linux sandbox in Chrome uses two sandboxing methods in Linux, which can be also stacked: > > - Chroot sandbox that also uses Linux namespaces so that renderer process cannot espace from the chroot cage. > > - Seccomp sandbox that prevents malicious system calls to be called from the renderer process. > > If I was implementing webkit 2 Linux sandbox, I would aim for solution that would be only based seccomp filters in order to create simple and maintainable solution. Chroot sandbox in Chrome is ugly and complex beast that requires SUID enabled wrapper program. > > At this point, at least in the mainline kernel, there's no Capsicum for Linux. seccomp also disallows you to open new files so how are they handling fonts? I know they have a separate "fontconfig" process as their chroot'ed environment cannot load the fonts which are outside of the chroot, and because they cannot block the UI process (font-config only supports running in one thread and it already running in the main ui process).
(In reply to comment #4) > With my FreeBSD packager hat on, I'd really appreciate it if the actual implementation was not heavily tied to Linux stuff (the chroot approach probably works on other Unix flavors as well, for example). Yeah, we were talking about it on IRC too and it seems there is no obstacle that stand in the way in keeping both solutions in the trunk. The approach with chroot will work on any linux platform and if somebody has the claim to use seccomp filter he or she can choose it via a feature flag. > Chroot sandbox in Chrome is ugly and complex beast that requires SUID enabled wrapper program. Right, it really needs SUID flag but the chrooting, what requires this, is the very first thing in the life of webprocess and after it every permissions are taken away and the user runs with "nobody" rights.
(In reply to comment #6) > (In reply to comment #5) > > Hi Raphael, > > > > Linux sandbox in Chrome uses two sandboxing methods in Linux, which can be also stacked: > > > > - Chroot sandbox that also uses Linux namespaces so that renderer process cannot espace from the chroot cage. > > > > - Seccomp sandbox that prevents malicious system calls to be called from the renderer process. > > > > If I was implementing webkit 2 Linux sandbox, I would aim for solution that would be only based seccomp filters in order to create simple and maintainable solution. Chroot sandbox in Chrome is ugly and complex beast that requires SUID enabled wrapper program. > > > > At this point, at least in the mainline kernel, there's no Capsicum for Linux. > > seccomp also disallows you to open new files so how are they handling fonts? I know they have a separate "fontconfig" process as their chroot'ed environment cannot load the fonts which are outside of the chroot, and because they cannot block the UI process (font-config only supports running in one thread and it already running in the main ui process). They use a much stricter seccomp sandbox than what we are proposing. At the time they created their sandbox, seccomp filters were not available. Chromium has to proxy everything through the UIProcess, since they can only do read and write. Things get even more complicated when you think about how a proccess like this can allocate memory? On Chromium this is delegated to a separated thread. Fontconfig is handled in a separate process (used to be on the UI though): http://code.google.com/p/chromium/wiki/LinuxSandboxIPC Another point here is, like Chromium provides both seccomp and chroot, I don't see a problem in offering both for WK2 Linux. But I guess that for both implementations, would be much better to delegate the file opening to the UIProcess. This has to be done for the file picker. Chromium assumes that the UIProcess is trusted. Honestly, I only see this happening if we do the same.
Very interesting article about Chromium's use of seccomp and the use of a separate _thread_ in the renderer process given that seccomp's afinity is per thread : http://www.imperialviolet.org/2009/08/26/seccomp.html
(In reply to comment #9) > Very interesting article about Chromium's use of seccomp and the use of a separate _thread_ in the renderer process given that seccomp's afinity is per thread : http://www.imperialviolet.org/2009/08/26/seccomp.html The thread is used for memory allocation, as I said before. I think this is the one of the few things that another process cannot do in your behalf. But remember that this article is pretty old. Chromium guys were behind seccomp filters for a reason. If they haven't changed to filters yet, they will do soon and this separated thread for memory allocation is one the things that will be deprecated, since you can let the renderer perform brk() and mmap(MAP_ANONYMOUS). Many overcomplex design decisions were made at that time because seccomp was way too restrict.
(In reply to comment #7) > (In reply to comment #4) > > With my FreeBSD packager hat on, I'd really appreciate it if the actual implementation was not heavily tied to Linux stuff (the chroot approach probably works on other Unix flavors as well, for example). > > Yeah, we were talking about it on IRC too and it seems there is no obstacle that stand in the way in keeping both solutions in the trunk. The approach with chroot will work on any linux platform and if somebody has the claim to use seccomp filter he or she can choose it via a feature flag. There is no obstacle to do it but it does not make sense to make complex solution either. Seccomp filters are mainline kernel feature in 3.5. > > > Chroot sandbox in Chrome is ugly and complex beast that requires SUID enabled wrapper program. > > Right, it really needs SUID flag but the chrooting, what requires this, is the very first thing in the life of webprocess and after it every permissions are taken away and the user runs with "nobody" rights. In any case this increases attack vector. IMHO, no matter how sandbox is implemented, it should only decrease attack vector. It is kind of point why sandboxing is done in the first place.
(In reply to comment #9) > Very interesting article about Chromium's use of seccomp and the use of a separate _thread_ in the renderer process given that seccomp's afinity is per thread : http://www.imperialviolet.org/2009/08/26/seccomp.html Before seccomp filters were accepted to the mainline kernel Chrome has implemented seccomp filters in user space using two threads where privileged thread does the system call whitelisting for the unprivileged thread. Starting from Linux 3.5 mainline kernel has seccomp filters implemented in kernel where you can whitelist system calls with a packet filter.
(In reply to comment #12) > (In reply to comment #9) > > Very interesting article about Chromium's use of seccomp and the use of a separate _thread_ in the renderer process given that seccomp's afinity is per thread : http://www.imperialviolet.org/2009/08/26/seccomp.html > > Before seccomp filters were accepted to the mainline kernel Chrome has implemented seccomp filters in user space using two threads where privileged thread does the system call whitelisting for the unprivileged thread. Starting from Linux 3.5 mainline kernel has seccomp filters implemented in kernel where you can whitelist system calls with a packet filter. Yes, you are right. The 3.5 kernel has Seccomp filter support. However this kernel is just 2 weeks old. And I'm still sure that the user base of chroot is much bigger.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #9) > > > Very interesting article about Chromium's use of seccomp and the use of a separate _thread_ in the renderer process given that seccomp's afinity is per thread : http://www.imperialviolet.org/2009/08/26/seccomp.html > > > > Before seccomp filters were accepted to the mainline kernel Chrome has implemented seccomp filters in user space using two threads where privileged thread does the system call whitelisting for the unprivileged thread. Starting from Linux 3.5 mainline kernel has seccomp filters implemented in kernel where you can whitelist system calls with a packet filter. > > Yes, you are right. The 3.5 kernel has Seccomp filter support. However this kernel is just 2 weeks old. And I'm still sure that the user base of chroot is much bigger. Having a trivial chroot as a sandbox is almost the same as not having sandbox at all. It is a known fact that trivial chroot jail is a weak protection. This kind of method would require similar measures as are taken in Chromium SUID sandbox.
> > > Before seccomp filters were accepted to the mainline kernel Chrome has implemented seccomp filters in user space using two threads where privileged thread does the system call whitelisting for the unprivileged thread. Starting from Linux 3.5 mainline kernel has seccomp filters implemented in kernel where you can whitelist system calls with a packet filter. > > > > Yes, you are right. The 3.5 kernel has Seccomp filter support. However this kernel is just 2 weeks old. And I'm still sure that the user base of chroot is much bigger. > > Having a trivial chroot as a sandbox is almost the same as not having sandbox at all. It is a known fact that trivial chroot jail is a weak protection. This kind of method would require similar measures as are taken in Chromium SUID sandbox. But having a solution, what works just on a few targets, isn't better. :)
Created attachment 150823 [details] Completely incomplete, just to get feedback :) This is still a draft patch. From this version only QtWebProcess needs the suid flag. Its first tasks are: chrooting itself and dropping away all the suid rights. We are going to fall back to the "nobdoy" user (btw this is the only user defined inside the sandbox environment). Furthermore the patch contains a new feature flag: QtWebProcess will run in sandbox only if we build it with --suid-linux-sandbox flag. (All layout tests are passing with this option too.) Task in progress: I still have problem with the dynamic generation of the dependencies because some environment variable (e.g.: LD_LIBRARY_PATH) - what are needed by ldd - are cleared through the suid flag.
FYI: two weeks ago chromium has released a new stable version with security updates. Few words about this: "So why have just one Flash sandbox if you can have two? A bit of double-bagging if you like. Assuming you're running 64-bit Ubuntu 12.04 and Chrome 20 or newer, you'll also have a seccomp filter policy slapped on Flash -- in additional to the chroot() and PID namespace. This may impede attackers trying to perform a local privilege escalation, who can no longer call crazy brand-new syscalls or use socket() to load crazy protocol modules, etc. No sandbox or combination of sandboxes will ever be perfect, but "some" is better than "none"." I cited from here: http://scarybeastsecurity.blogspot.hu/2012/07/chrome-20-on-linux-and-flash-sandboxing.html
Without discussing the merits of whether chroot is the best approach, wouldn't it make sense to make it easier to have multiple backends here (with my packager hat on again)? This way one could have chroot, seccomp, capsicum etc backends to use without having to clutter WebProcessMainQt with a lot of ifdef'ed code and it'd make the whole implementation less tied to Linux.
Created attachment 153232 [details] Still not complete, but makes more sense :) The patch contains the following updates: * The number of mounted directories are strongly reduced. * An ldd funcionality is included. It gathers the shared libraries what are needed by QtWebProcess. ** It is placed into UIProcess because glibc prevent us from calling ldd from suid binary (they have security reasons to do it). ** After gathering this library list we pass it to QtWebProcess what hard links them into the sandbox environment. To do this the command line of QtWebProcess has got a third parameter with a pointer to the library list. * Unfortunatelly ldd doesn't detect all the needed libaries because some of them are dynamically loaded. So we should determine them manually. For this I'm stepping through the alexa top100 list and checking what are missing (this list is incomplete yet). Furthermore, since the different linux distrubions and versions could have these libaries with different versions and paths, we have to obtain the actually installed versions. One possible way is the `ldconfig -p` command. I implemented this one and also linked the result into the sandbox. Further plans, questions and problems: * To decrease the attack surface, what was raised by putting the suid flag to QtWebProcess, I intend to outsource the code of sandboxing into a sepatated binary. This way only this binary would have suid flag and it would be an intermediate step between UIProcess and QtWebProcess (if --suid-sandbox-linux flag is enabled ofc). Any objections? Opinions? * Using hard links raises a limitation: since hard links only works if the link and the target are on the same partition so the installed webkit should be on the OS partition. Any ideas how to workaround it are appreciated! ** Although copying the files instead hard linking them would solve this problem I'm not sure whether it worths the overhead (currently it would effect ~100 files and ~30Mb data). * An other plan is to remove all mounting step and using copy, link, etc instead. This way the cleanup also will be easier, since we don't have to wait until all the mounted resource will be free and ready to unmount. Btw what do you think, what would be the better way: building up and destroying the sandbox environment every time the WebProcess born and die or only once and using it later too? The second one is quicker but has security concerns if someone compromise the sandbox directory.
(In reply to comment #19) > Created an attachment (id=153232) [details] > Still not complete, but makes more sense :) > > The patch contains the following updates: > * The number of mounted directories are strongly reduced. > * An ldd funcionality is included. It gathers the shared libraries what are needed by QtWebProcess. > ** It is placed into UIProcess because glibc prevent us from calling ldd from suid binary (they have security reasons to do it). > ** After gathering this library list we pass it to QtWebProcess what hard links them into the sandbox environment. To do this the command line of QtWebProcess has got a third parameter with a pointer to the library list. > * Unfortunatelly ldd doesn't detect all the needed libaries because some of them are dynamically loaded. So we should determine them manually. For this I'm stepping through the alexa top100 list and checking what are missing (this list is incomplete yet). > Furthermore, since the different linux distrubions and versions could have these libaries with different versions and paths, we have to obtain the actually installed versions. One possible way is the `ldconfig -p` command. I implemented this one and also linked the result into the sandbox. > > Further plans, questions and problems: > * To decrease the attack surface, what was raised by putting the suid flag to QtWebProcess, I intend to outsource the code of sandboxing into a sepatated binary. This way only this binary would have suid flag and it would be an intermediate step between UIProcess and QtWebProcess (if --suid-sandbox-linux flag is enabled ofc). Any objections? Opinions? I don't understand how this makes any attack harder. Can you explain a bit? > * Using hard links raises a limitation: since hard links only works if the link and the target are on the same partition so the installed webkit should be on the OS partition. Any ideas how to workaround it are appreciated! > ** Although copying the files instead hard linking them would solve this problem I'm not sure whether it worths the overhead (currently it would effect ~100 files and ~30Mb data). > * An other plan is to remove all mounting step and using copy, link, etc instead. This way the cleanup also will be easier, since we don't have to wait until all the mounted resource will be free and ready to unmount. Btw what do you think, what would be the better way: building up and destroying the sandbox environment every time the WebProcess born and die or only once and using it later too? The second one is quicker but has security concerns if someone compromise the sandbox directory. The mount man page claims that you can bind-mount also files, but I haven't managed to get that working. But this particular issue of file visibility goes away if QtWebProcess itself is suid or if alternatively you do the chroot _after_ loading the depending shared libraries. Plugins are indeed trickier, but I don't think those would be so big that copying would be an issue. What plugins could there be?
(In reply to comment #20) > > Further plans, questions and problems: > > * To decrease the attack surface, what was raised by putting the suid flag to QtWebProcess, I intend to outsource the code of sandboxing into a sepatated binary. This way only this binary would have suid flag and it would be an intermediate step between UIProcess and QtWebProcess (if --suid-sandbox-linux flag is enabled ofc). Any objections? Opinions? > > I don't understand how this makes any attack harder. Can you explain a bit? The shorter time the attacker has suid rights, the less possibility to do something really bad. (At least IMHO.) And for sandboxing we only need this right by setting up the environment. > The mount man page claims that you can bind-mount also files, but I haven't managed to get that working. But this particular issue of file visibility goes away if QtWebProcess itself is suid or if alternatively you do the chroot _after_ loading the depending shared libraries. Actually I do chrooting after the dependency load right now. My problem with binding is not binding direcorties or files, but the binding itself. Let's say I bind /proc. Then I cannot umount it after WebProcess ended because it's still busy (lazy unmount could be a solution, but the containing directory won't be deleted that way). Copying can work except persistent storages like caches, where the user desires to keep the data. > Plugins are indeed trickier, but I don't think those would be so big that copying would be an issue. What plugins could there be? Adding all possible plugins and its dependencies to the sandbox seems an operable solution. We can get their paths with PluginDatabase::defaultPluginDirectories().
(In reply to comment #21) > (In reply to comment #20) > > > > Further plans, questions and problems: > > > * To decrease the attack surface, what was raised by putting the suid flag to QtWebProcess, I intend to outsource the code of sandboxing into a sepatated binary. This way only this binary would have suid flag and it would be an intermediate step between UIProcess and QtWebProcess (if --suid-sandbox-linux flag is enabled ofc). Any objections? Opinions? > > > > I don't understand how this makes any attack harder. Can you explain a bit? > The shorter time the attacker has suid rights, the less possibility to do something really bad. (At least IMHO.) And for sandboxing we only need this right by setting up the environment. So, what do you think? Do we want to have such separation or not? :)
I'm back from holiday. Let's continue the work! Comments are still welcomed ;)
Created attachment 160200 [details] Proposed patch Introducing the new SUIDSandboxHelper process.
Basically the patch contains the following changes: * It moves the suid sandbox functionality into a new binary, named SUIDSandboxHelper. This process contains all operations that need sudoer rights. This way only this short code snippet will have suid flag and WebProcess runs with nobody rights already. So a malicious attacker has less opportunity to gain root rights. * Due to this modification, the sandbox environment also had to be changed. * Only special directories (file systems): /proc and /run/shm are "mount --bind"-ed right now, the others are simply hard linked. (Binding them still raises security problems. The next step is to solve them.) These are the substantive changes. Details are in the comments.
Attachment 160200 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/Configurations/FeatureDefin..." exit_code: 1 Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:22: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:24: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 160200 [details] Proposed patch Attachment 160200 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13571747
(In reply to comment #27) > (From update of attachment 160200 [details]) > Attachment 160200 [details] did not pass qt-wk2-ews (qt): > Output: http://queues.webkit.org/results/13571747 The reason of this fail (besides I missed to write back the ENABLE(SUID_SANDBOX_LINUX) macros into the new files too) is that the glibc version on qt-wk2-ews is 2.11.3-3 however the definition of MS_MOVE and MS_REC are only supported from 2.12. Besides it's quite hard to test the patch with ews because it has only effect if the build was called with --suid-linux-sandbox flag.
Comment on attachment 160200 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=160200&action=review Just a quick look though.. so no comments on the actual implementation. I don't understand why this is so tied to Qt. It doesn't really need to. > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:43 > +SandboxEnvironmentHandlerQt::SandboxEnvironmentHandlerQt(QString chrootDir) Why not SandboxEnvironementLinux or so > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:75 > + mknod((sandboxRoot + devices.at(i)).toLocal8Bit().data(), > + S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH, makedev(major(dev), minor(dev))); > + } Maybe some description what this means? or what you are attempting > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:96 > +void SandboxEnvironmentHandlerQt::addNobodyUser() addNobodyUserToPasswd? > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:106 > + struct passwd* nobodyUser = getpwnam("nobody"); > + QString passwordContent = QLatin1String(nobodyUser->pw_name) + QLatin1Char(':') + > + QLatin1String(nobodyUser->pw_passwd) + QLatin1Char(':') + > + QString::number(nobodyUser->pw_uid) + QLatin1Char(':') + > + QString::number(nobodyUser->pw_gid) + QLatin1Char(':') + > + QLatin1String(nobodyUser->pw_gecos) + QLatin1Char(':') + Why making all this so Qt dependent for no apparent reason? The WebKit string methods are probably a lot faster and cross platform (http://trac.webkit.org/wiki/EfficientStrings) and maybe GTK and EFL would be interested in the same functionality at some point > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:114 > +void SandboxEnvironmentHandlerQt::createLink(QString src, QString target) sourcePath, targetPath? or linkPath, sourcePath. Why not call it linkFile when you already have a linkDirectory. Currently it is inconsistent > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.h:32 > + QStringList directoriesToBeBinded; Binded? Isn't it called bound (http://en.wiktionary.org/wiki/bind) > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.h:42 > + void addNobodyUser(); Add it where? > Source/WebKit2/SandboxProcess/SandboxProcess.pro:23 > + > + > + why all these newlines > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:112 > +static void collectDepencednies(QString &dependenyList, QString process, char params[]) Dependencies* > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:144 > + dependenyList += QLatin1String(pathOfTheLibrary); dependencyList* I think some of this code should be split out into methods, that would also make it a lot easier to understand > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:278 > - delete webProcess; > + delete webProcessOrSUIDHelper; Could this be an OwnPtr?
Created attachment 161242 [details] Proposed patch
(In reply to comment #29) > (From update of attachment 160200 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160200&action=review > > Just a quick look though.. so no comments on the actual implementation. First of all, thank you for the quick review. :) > I don't understand why this is so tied to Qt. It doesn't really need to. Indeed. It was easier to use the tools what Qt supplied (I think here of the file operation functions and not strings). Now they are rewritten: mkpath, mkdir, linkFileRecursively, linkDirectory, etc... They are part of the sandboxhandler now. Is it right? > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:43 > > +SandboxEnvironmentHandlerQt::SandboxEnvironmentHandlerQt(QString chrootDir) > > Why not SandboxEnvironementLinux or so Right. Renamed. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:75 > > + mknod((sandboxRoot + devices.at(i)).toLocal8Bit().data(), > > + S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH, makedev(major(dev), minor(dev))); > > + } > > Maybe some description what this means? or what you are attempting The device files, what we have to use (random and urandom), are character devices and their permissions should be: rw-rw-rw. The formula above describes this. Explanation comment is added to the source. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:96 > > +void SandboxEnvironmentHandlerQt::addNobodyUser() > > addNobodyUserToPasswd? Okay, changed. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:106 > > + struct passwd* nobodyUser = getpwnam("nobody"); > > + QString passwordContent = QLatin1String(nobodyUser->pw_name) + QLatin1Char(':') + > > + QLatin1String(nobodyUser->pw_passwd) + QLatin1Char(':') + > > + QString::number(nobodyUser->pw_uid) + QLatin1Char(':') + > > + QString::number(nobodyUser->pw_gid) + QLatin1Char(':') + > > + QLatin1String(nobodyUser->pw_gecos) + QLatin1Char(':') + > > Why making all this so Qt dependent for no apparent reason? The WebKit string methods are probably a lot faster and cross platform (http://trac.webkit.org/wiki/EfficientStrings) and maybe GTK and EFL would be interested in the same functionality at some point Right. QStrings are changed to WTFStrings. The only Qt-specific thing is the QProcess (QtWebProcess) now. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.cpp:114 > > +void SandboxEnvironmentHandlerQt::createLink(QString src, QString target) > > sourcePath, targetPath? or linkPath, sourcePath. > Why not call it linkFile when you already have a linkDirectory. Currently it is inconsistent Done. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.h:32 > > + QStringList directoriesToBeBinded; > > Binded? Isn't it called bound (http://en.wiktionary.org/wiki/bind) OK. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentHandlerQt.h:42 > > + void addNobodyUser(); > > Add it where? Renamed. > > Source/WebKit2/SandboxProcess/SandboxProcess.pro:23 > > + > > + > > + > > why all these newlines Removed. > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:112 > > +static void collectDepencednies(QString &dependenyList, QString process, char params[]) > > Dependencies* OK. > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:144 > > + dependenyList += QLatin1String(pathOfTheLibrary); > > dependencyList* OK. > I think some of this code should be split out into methods, that would also make it a lot easier to understand Done. > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:278 > > - delete webProcess; > > + delete webProcessOrSUIDHelper; > > Could this be an OwnPtr? I didn't want to change the original implementation at all (this is just a renaming too). But if you think I can change it ofc.
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:57 > + m_devices.append("/dev/urandom"); appendLiteral("/dev/urandom"); is more effective > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:69 > + m_directoriesToBeLinkedFromHome.append("/.local/share/Nokia/"); > + m_directoriesToBeLinkedFromHome.append("/.cache/Nokia/"); if PLATFORM(QT) ? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:75 > + m_directoriesToBeLinked.append(ASCIILiteral(getenv("QTDIR")) + "/plugins"); appendLiteral(getenv("QTDIR")) + "/plugins") <- but there is a method for adding dirs String pathByAppendingComponent(const String& path, const String& component) defined in FileSystem.h > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:79 > +void SandboxEnvironmentLinux::mkDir(const char *path, mode_t mode) I wonder whether it is better to call this createDirectory or so? Where are you using mode? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:92 > + char* clonePath = (char*)malloc(sizeof(char*) * strlen(path)); All buffers containing strings input by the user should be explicitly terminated with \0 before a call to strlen() Why not just use mkPath(const String &) That is safer > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:93 > + strcpy(clonePath, path); strcpy can be a security issue. https://buildsecurityin.us-cert.gov/bsi-rules/home/g1/848-BSI.html strlcpy/strncpy can be a replacement > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:107 > + startOfSubPath = endOfSubPath; > + while ((endOfSubPath = strchr(startOfSubPath, '/'))) { > + if (endOfSubPath != startOfSubPath) { > + *endOfSubPath = '\0'; > + mkDir(clonePath, mode); > + *endOfSubPath = '/'; > + } > + startOfSubPath = endOfSubPath + 1; > + } > + free(endOfSubPath); > + free(clonePath); > + // Create the last directory. > + mkDir(path, mode); Why not something like if (!path || !*path)) return false; bool ok = true; char* ptr = path; while (ptr = strchr(ptr + 1, '/')) { *ptr = '\0'; ok = mkdir(path, mode) != -1; *ptr = '/'; if (!ok) break; } return ok; } It modified in the link and avoids malloc > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:113 > + mkPath(devDirectory.utf8().data(), 0777); 0777 could use a comment > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:169 > + if (link(sourceFile.latin1().data(), targetFile.latin1().data()) == -1 && errno != EEXIST) why latin1? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:254 > + String xauthorityOfRealUser = String::fromUTF8(getenv("HOME")) + "/.Xauthority"; > + String xauthorityInSandbox = m_sandboxRoot + m_homeDirectory + "/.Xauthority"; What if not running under X ? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:278 > + const String sandboxDirectory = String::fromUTF8(getpwuid(getpwuid(getuid())->pw_uid)->pw_dir) + String::fromUTF8("/.wk-sandbox102"); the latter seems like a literal.. .why from UTF8? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:36 > + Vector<String> m_filesToBeLinked; I dont like this tobe much... why not just m_linkedFiles, m_linkedDirectories, m_linkedHomeDirectories > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:40 > + String m_webkitDirectory; Why not binaryDirectory? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:42 > + SandboxEnvironmentLinux(String, String); const STring &? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:44 > + void addDependencies(String); why nost const String& ? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:114 > +static void filterDependenciesFromLddOutput(char* buffer, String& dependencyList) LDD would be the correct style. libraryDependencies > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:118 > + // The output of ldd can have two formats: > + // 1) \tname_of_the_library => path_of_library (address_of_the_libary) > + // 2) path_of_the_library (address_of_the_libary) What if ldd gets compromised? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:126 > + if (numberOfmatchingCharacters > 0) { > + dependencyList.append(pathOfTheLibrary); > + dependencyList.append(';'); > + } Why not use a vector > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:131 > + String library(buffer); What is this is really long? not a security issue? :-), why no limit > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:133 > + // The list of emirically gathered dependency libraries. emirically? empirically? "library dependencies" maybe
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:105 > + free(endOfSubPath); > + free(clonePath); Do you really need to do all this manual memory management? :(
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:69 >> + m_directoriesToBeLinkedFromHome.append("/.local/share/Nokia/"); >> + m_directoriesToBeLinkedFromHome.append("/.cache/Nokia/"); > > if PLATFORM(QT) ? How about ./cache/org.webkit ?
(In reply to comment #32) > (From update of attachment 161242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:57 > > + m_devices.append("/dev/urandom"); > > appendLiteral("/dev/urandom"); is more effective m_devices and the others are WTFVectors right now and appendLiteral is for StringBuilders. Should I change the type of these members? > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:79 > > +void SandboxEnvironmentLinux::mkDir(const char *path, mode_t mode) > > I wonder whether it is better to call this createDirectory or so? Where are you using mode? Actually nowhere right now. I created all directories with 0777 for simplicity, but I'm thinking of the right solution. The permissions of the most system directories are 0755 and their owner is root ofc. In the other hand, directories in home directory have different permissions (and owner). So what should I do? Ask the permissions of the original directories and set the same to the "mirror" directories in the sandbox? Or since we only have a nobody user should I care only with this and set system dirs to 0050 and nobodys home to 0700? (I have to change the owner of the nobodys home folder too ofc.) > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:254 > > + String xauthorityOfRealUser = String::fromUTF8(getenv("HOME")) + "/.Xauthority"; > > + String xauthorityInSandbox = m_sandboxRoot + m_homeDirectory + "/.Xauthority"; > > What if not running under X ? I guess you think of using webkit on embedded systems. But unfortunatelly I'm not really sure what should I take care of. Any hints would be appreciated. > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:118 > > + // The output of ldd can have two formats: > > + // 1) \tname_of_the_library => path_of_library (address_of_the_libary) > > + // 2) path_of_the_library (address_of_the_libary) > > What if ldd gets compromised? If ldd is compromised that means the enemy is "in the house" already. Should/can we stop him? > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:126 > > + if (numberOfmatchingCharacters > 0) { > > + dependencyList.append(pathOfTheLibrary); > > + dependencyList.append(';'); > > + } > > Why not use a vector Because I should send this list to the sandboxProcess and the commandline (QString::arg()) only operates with strings.
(In reply to comment #34) > (From update of attachment 161242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:69 > >> + m_directoriesToBeLinkedFromHome.append("/.local/share/Nokia/"); > >> + m_directoriesToBeLinkedFromHome.append("/.cache/Nokia/"); > > > > if PLATFORM(QT) ? > > How about ./cache/org.webkit ? Actually I don't have cache directory with this name in my home :$. Should I?
(In reply to comment #36) > (In reply to comment #34) > > (From update of attachment 161242 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > > > >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:69 > > >> + m_directoriesToBeLinkedFromHome.append("/.local/share/Nokia/"); > > >> + m_directoriesToBeLinkedFromHome.append("/.cache/Nokia/"); > > > > > > if PLATFORM(QT) ? > > > > How about ./cache/org.webkit ? > Actually I don't have cache directory with this name in my home :$. Should I? I think he is suggesting that you change Qt, EFL, GTK etc to use that name instead
(In reply to comment #35) > (In reply to comment #32) > > (From update of attachment 161242 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > > > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:57 > > > + m_devices.append("/dev/urandom"); > > > > appendLiteral("/dev/urandom"); is more effective > m_devices and the others are WTFVectors right now and appendLiteral is for StringBuilders. Should I change the type of these members? Then you can use ASCIILiteral
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:32 > +#include <QCoreApplication> > +#include <QDebug> > +#include <QDir> > +#include <QFile> > +#include <QFileInfo> > +#include <QProcess> I would not call it SandboxEnvironment_Linux_.cpp with these Qt dependencies. It is misleading and some might think it will work on GTK or EFL (which would be nice). >>>>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:69 >>>>> + m_directoriesToBeLinkedFromHome.append("/.cache/Nokia/"); >>>> >>>> if PLATFORM(QT) ? >>> >>> How about ./cache/org.webkit ? >> >> Actually I don't have cache directory with this name in my home :$. Should I? > > I think he is suggesting that you change Qt, EFL, GTK etc to use that name instead Why not use the ::platformInitialize() pattern here?
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:32 >> +#include <QProcess> > > I would not call it SandboxEnvironment_Linux_.cpp with these Qt dependencies. It is misleading and some might think it will work on GTK or EFL (which would be nice). Yeah... I forgot to remove them. Only QProcess is what we need.
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:75 >> + m_directoriesToBeLinked.append(ASCIILiteral(getenv("QTDIR")) + "/plugins"); > > appendLiteral(getenv("QTDIR")) + "/plugins") <- but there is a method for adding dirs String pathByAppendingComponent(const String& path, const String& component) defined in FileSystem.h Yes. I checked it. There are few useful methods what I could use (makeAllDirectories, fileExist, directoryName, etc...). But w/o linking the whole libWebCore to my sandbox it won't work. However if I do this then the sandbox conception would lose its original purpose. We discussed about it with bbandix on IRC and it's possible to link just a few source from webcore to the sandbox. Unfortunatelly it would need a lot of hack and MACROs and I guess it doesn't worth for just few functions. What do you think?
(In reply to comment #41) > (From update of attachment 161242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:75 > >> + m_directoriesToBeLinked.append(ASCIILiteral(getenv("QTDIR")) + "/plugins"); > > > > appendLiteral(getenv("QTDIR")) + "/plugins") <- but there is a method for adding dirs String pathByAppendingComponent(const String& path, const String& component) defined in FileSystem.h > > Yes. I checked it. There are few useful methods what I could use (makeAllDirectories, fileExist, directoryName, etc...). But w/o linking the whole libWebCore to my sandbox it won't work. However if I do this then the sandbox conception would lose its original purpose. We discussed about it with bbandix on IRC and it's possible to link just a few source from webcore to the sandbox. Unfortunatelly it would need a lot of hack and MACROs and I guess it doesn't worth for just few functions. What do you think? That is fine reasoning. Still reusing the method names makes sense
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review >>>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:75 >>>> + m_directoriesToBeLinked.append(ASCIILiteral(getenv("QTDIR")) + "/plugins"); >>> >>> appendLiteral(getenv("QTDIR")) + "/plugins") <- but there is a method for adding dirs String pathByAppendingComponent(const String& path, const String& component) defined in FileSystem.h >> >> Yes. I checked it. There are few useful methods what I could use (makeAllDirectories, fileExist, directoryName, etc...). But w/o linking the whole libWebCore to my sandbox it won't work. However if I do this then the sandbox conception would lose its original purpose. We discussed about it with bbandix on IRC and it's possible to link just a few source from webcore to the sandbox. Unfortunatelly it would need a lot of hack and MACROs and I guess it doesn't worth for just few functions. What do you think? > > That is fine reasoning. Still reusing the method names makes sense Well, the other option is to not have a separate binary for the sandbox. What does it _really_ buy us?
(In reply to comment #43) > (From update of attachment 161242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > >>>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:75 > >>>> + m_directoriesToBeLinked.append(ASCIILiteral(getenv("QTDIR")) + "/plugins"); > >>> > >>> appendLiteral(getenv("QTDIR")) + "/plugins") <- but there is a method for adding dirs String pathByAppendingComponent(const String& path, const String& component) defined in FileSystem.h > >> > >> Yes. I checked it. There are few useful methods what I could use (makeAllDirectories, fileExist, directoryName, etc...). But w/o linking the whole libWebCore to my sandbox it won't work. However if I do this then the sandbox conception would lose its original purpose. We discussed about it with bbandix on IRC and it's possible to link just a few source from webcore to the sandbox. Unfortunatelly it would need a lot of hack and MACROs and I guess it doesn't worth for just few functions. What do you think? > > > > That is fine reasoning. Still reusing the method names makes sense > > Well, the other option is to not have a separate binary for the sandbox. What does it _really_ buy us? The basic problem with chrooted sandbox is that the process that performs the chroot() call needs superuser permissions. For this, the process should be run with sudo or it should have its SUID flag set. That's, if WebProcess calls the chroot() then it will live with suid rights from the beginnig until its end, even if we change back its effective user to nobody. Then again, if we push all the problematic tasks into a separated binary and give rights only to this binary, then we have to take care of this little codebase only security-wise, not of the whole WebProcess.
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review >> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:114 >> +static void filterDependenciesFromLddOutput(char* buffer, String& dependencyList) > > LDD would be the correct style. > > libraryDependencies Did you think of "filterLibraryDependenciesFromLDDOutput" or "String& libraryDependencies" by your second naming suggestion?
Created attachment 161645 [details] Proposed patch
Comment on attachment 161242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review >>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:32 >>> +#include <QProcess> >> >> I would not call it SandboxEnvironment_Linux_.cpp with these Qt dependencies. It is misleading and some might think it will work on GTK or EFL (which would be nice). > > Yeah... I forgot to remove them. Only QProcess is what we need. Removed. >>>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:57 >>>> + m_devices.append("/dev/urandom"); >>> >>> appendLiteral("/dev/urandom"); is more effective >> >> m_devices and the others are WTFVectors right now and appendLiteral is for StringBuilders. Should I change the type of these members? > > Then you can use ASCIILiteral OK. >>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:79 >>> +void SandboxEnvironmentLinux::mkDir(const char *path, mode_t mode) >> >> I wonder whether it is better to call this createDirectory or so? Where are you using mode? > > Actually nowhere right now. I created all directories with 0777 for simplicity, but I'm thinking of the right solution. The permissions of the most system directories are 0755 and their owner is root ofc. In the other hand, directories in home directory have different permissions (and owner). So what should I do? Ask the permissions of the original directories and set the same to the "mirror" directories in the sandbox? Or since we only have a nobody user should I care only with this and set system dirs to 0050 and nobodys home to 0700? (I have to change the owner of the nobodys home folder too ofc.) I've added the permissions() function what obtain the protection information of the original directory and set the same in the sandbox too. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:92 >> + char* clonePath = (char*)malloc(sizeof(char*) * strlen(path)); > > All buffers containing strings input by the user should be explicitly terminated with \0 before a call to strlen() > > Why not just use mkPath(const String &) That is safer This function is rewritten in the new version according to your suggestions. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:93 >> + strcpy(clonePath, path); > > strcpy can be a security issue. https://buildsecurityin.us-cert.gov/bsi-rules/home/g1/848-BSI.html > > strlcpy/strncpy can be a replacement I don't need such functionality anymore. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:105 >> + free(clonePath); > > Do you really need to do all this manual memory management? :( Not anymore. :) >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:169 >> + if (link(sourceFile.latin1().data(), targetFile.latin1().data()) == -1 && errno != EEXIST) > > why latin1? Mistake. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:278 >> + const String sandboxDirectory = String::fromUTF8(getpwuid(getpwuid(getuid())->pw_uid)->pw_dir) + String::fromUTF8("/.wk-sandbox102"); > > the latter seems like a literal.. .why from UTF8? Yeah, it's a literal. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:36 >> + Vector<String> m_filesToBeLinked; > > I dont like this tobe much... why not just m_linkedFiles, m_linkedDirectories, m_linkedHomeDirectories Renamed. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:40 >> + String m_webkitDirectory; > > Why not binaryDirectory? Renamed. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:42 >> + SandboxEnvironmentLinux(String, String); > > const STring &? Ok. >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:44 >> + void addDependencies(String); > > why nost const String& ? Ok. >>> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:114 >>> +static void filterDependenciesFromLddOutput(char* buffer, String& dependencyList) >> >> LDD would be the correct style. >> >> libraryDependencies > > Did you think of "filterLibraryDependenciesFromLDDOutput" or "String& libraryDependencies" by your second naming suggestion? I've changed both of them :P >> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:131 >> + String library(buffer); > > What is this is really long? not a security issue? :-), why no limit Limited. >> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:133 >> + // The list of emirically gathered dependency libraries. > > emirically? empirically? "library dependencies" maybe Ok.
Comment on attachment 161645 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161645&action=review > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:77 > +{ > + char* clone = const_cast<char*>(path.utf8().data()); I think this would be nicer (and more safe; modifying a local copy): String copy = path; char* ptr = copy.utf8().data(); > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:92 > + ok = mkdir(path.utf8().data(), mode) || errno == EEXIST; > + if (ok) > + return true; > + return false; why not return ok? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:95 > +static mode_t permissions(String directory) const String& ? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:222 > + source.insert("/", 0); isn't there an insert('/', 0)? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:274 > +int main(int argc, char *argv[]) > +{ > + const String sandboxDirectory = ASCIILiteral(getpwuid(getpwuid(getuid())->pw_uid)->pw_dir) + "/.wk-sandbox"; > + SandboxEnvironmentLinux* sandboxHandler = new SandboxEnvironmentLinux(sandboxDirectory, ASCIILiteral(argv[0])); > + sandboxHandler->initializeSandbox(ASCIILiteral(argv[3])); Don't you want to varify the args ? > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:305 > + // Calling WebProcess with nobody rights. > + QString commandLine = QLatin1String("%1 %2"); > + commandLine = commandLine.arg(QLatin1String(argv[1])); > + commandLine = commandLine.arg(QLatin1String(argv[2])); > + > + QProcess* webProcess = new QProcess(); > + webProcess->setProcessChannelMode(QProcess::ForwardedChannels); > + webProcess->start(commandLine); > + This should be in some platform method. Or the whole main should be in a different file per platform. > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:126 > + if (!numberOfmatchingCharacters) > + numberOfmatchingCharacters = sscanf(buffer, "\t/%s %*s\n", pathOfTheLibrary); > + if (numberOfmatchingCharacters > 0) { > + libraryDependencies.append(pathOfTheLibrary); > + libraryDependencies.append(';'); > + } weird indentation > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:137 > + dynamicDependencies.append("libcrypto.so"); > + dynamicDependencies.append("libexpat.so"); > + dynamicDependencies.append("libfontconfig.so"); > + dynamicDependencies.append("libfreetype.so"); Can't these be gotten from the build system deps? or like pkgconfig ?
Comment on attachment 161645 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161645&action=review >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:305 >> + > > This should be in some platform method. Or the whole main should be in a different file per platform. Yeah, it would make sense to take apart the platform specific codes (as Thiago mentioned it in #39 too). The question is what we want to have in those platform files? Do we really need to move out the whole main? It seems to me that the first part is common in all platforms and just the calling of WebPrcoess differs. Furthermore we could add an addPlaformSpecificDependencies() virtual method. This way SandboxEnvironmentLinux won't contain any Qt or platformdependent code.
(In reply to comment #49) > (From update of attachment 161645 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161645&action=review > > >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:305 > >> + > > > > This should be in some platform method. Or the whole main should be in a different file per platform. > > Yeah, it would make sense to take apart the platform specific codes (as Thiago mentioned it in #39 too). The question is what we want to have in those platform files? Do we really need to move out the whole main? It seems to me that the first part is common in all platforms and just the calling of WebPrcoess differs. Furthermore we could add an addPlaformSpecificDependencies() virtual method. This way SandboxEnvironmentLinux won't contain any Qt or platformdependent code. Platform methods in webkit usually start with platform. like say there was a scrollBy() there might be a platformScrollby() as well called from it. Check WebKit for examples
(In reply to comment #50) > (In reply to comment #49) > > (From update of attachment 161645 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161645&action=review > > > > >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:305 > > >> + > > > > > > This should be in some platform method. Or the whole main should be in a different file per platform. > > > > Yeah, it would make sense to take apart the platform specific codes (as Thiago mentioned it in #39 too). The question is what we want to have in those platform files? Do we really need to move out the whole main? It seems to me that the first part is common in all platforms and just the calling of WebPrcoess differs. Furthermore we could add an addPlaformSpecificDependencies() virtual method. This way SandboxEnvironmentLinux won't contain any Qt or platformdependent code. > > Platform methods in webkit usually start with platform. like say there was a scrollBy() there might be a platformScrollby() as well called from it. Check WebKit for examples Okay. The mentioned name is just an example, we can change it ofc. The point is what we have in its body.
Hmm... I was thinking about where is the best place for the new SandboxProcess directory and I guess I should have placed it into WebKit2/Shared instead of the root of WebKit2. What do you think?
(In reply to comment #34) > (From update of attachment 161242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > >> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:69 > >> + m_directoriesToBeLinkedFromHome.append("/.local/share/Nokia/"); > >> + m_directoriesToBeLinkedFromHome.append("/.cache/Nokia/"); > > > > if PLATFORM(QT) ? > > How about ./cache/org.webkit ? I've checked where this cache path comes from (at least in Qt): there is a dataLocation variable in QtDefaultDataLocation.cpp what requests the following: QStandardPaths::writableLocation(QStandardPaths::DataLocation); According to the specification it is equialent with the concatenation of QCoreApplication::organizationName() and QCoreApplication::applicationName(). These variables are set in the constructor of MiniBrowserApplication. At first sight, gtk and efl work similar. The question is: do we still want to use a common cache path? In this case all the three platform should agree on this ofc.
(In reply to comment #48) > (From update of attachment 161645 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161645&action=review > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:222 > > + source.insert("/", 0); > > isn't there an insert('/', 0)? WTFString only supports the inserting of const String& and const UChar*, not single chars. > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:274 > > +int main(int argc, char *argv[]) > > +{ > > + const String sandboxDirectory = ASCIILiteral(getpwuid(getpwuid(getuid())->pw_uid)->pw_dir) + "/.wk-sandbox"; > > + SandboxEnvironmentLinux* sandboxHandler = new SandboxEnvironmentLinux(sandboxDirectory, ASCIILiteral(argv[0])); > > + sandboxHandler->initializeSandbox(ASCIILiteral(argv[3])); > > Don't you want to varify the args ? Hmm... What do you think of here exactly? Is it enough to check the number of the parameters? Or the types? Or what else? > > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:305 > > + // Calling WebProcess with nobody rights. > > + QString commandLine = QLatin1String("%1 %2"); > > + commandLine = commandLine.arg(QLatin1String(argv[1])); > > + commandLine = commandLine.arg(QLatin1String(argv[2])); > > + > > + QProcess* webProcess = new QProcess(); > > + webProcess->setProcessChannelMode(QProcess::ForwardedChannels); > > + webProcess->start(commandLine); > > + > > This should be in some platform method. Or the whole main should be in a different file per platform. Okay. I separated the platformdependent parts into two functions: platformInitialize() and platformRunWebProcess(). Is it all rigth? > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:137 > > + dynamicDependencies.append("libcrypto.so"); > > + dynamicDependencies.append("libexpat.so"); > > + dynamicDependencies.append("libfontconfig.so"); > > + dynamicDependencies.append("libfreetype.so"); > > Can't these be gotten from the build system deps? or like pkgconfig ? Hm... I see that the build system uses pkgconfig to determine the paths of some libraries. But I cannot see clearly how could I use this in my source :(
Created attachment 163116 [details] Proposed patch
In this patch the platform dependent parts are refactored into separated files. Besides Qt, I've added the skeleton of such codes for efl and gtk. Furthermore the permissions of all created directory are checked and set properly according to the original one. If the original owner was a simple user, then its owner inside the sandbox will be the nobody user. Finally it's rebased to the current repository and to the latest qt. At some points you can also see more refactored parts but they don't have functional effect.
Created attachment 163118 [details] Proposed patch
Comment on attachment 163118 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=163118&action=review > Source/WebCore/storage/StorageTracker.cpp:143 > + I will remove this. > Source/WebKit2/GNUmakefile.list.am:281 > + Source/WebKit2/SandboxProcess/SandboxEnvironmentLinuxGtk.cpp \ Oops. I've used tabs instead of spaces.
Comment on attachment 163118 [details] Proposed patch Attachment 163118 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13796896
Comment on attachment 163118 [details] Proposed patch Attachment 163118 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13811185
(In reply to comment #44) > (In reply to comment #43) > > (From update of attachment 161242 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161242&action=review > > > > >>>> Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:75 > > >>>> + m_directoriesToBeLinked.append(ASCIILiteral(getenv("QTDIR")) + "/plugins"); > > >>> > > >>> appendLiteral(getenv("QTDIR")) + "/plugins") <- but there is a method for adding dirs String pathByAppendingComponent(const String& path, const String& component) defined in FileSystem.h > > >> > > >> Yes. I checked it. There are few useful methods what I could use (makeAllDirectories, fileExist, directoryName, etc...). But w/o linking the whole libWebCore to my sandbox it won't work. However if I do this then the sandbox conception would lose its original purpose. We discussed about it with bbandix on IRC and it's possible to link just a few source from webcore to the sandbox. Unfortunatelly it would need a lot of hack and MACROs and I guess it doesn't worth for just few functions. What do you think? > > > > > > That is fine reasoning. Still reusing the method names makes sense > > > > Well, the other option is to not have a separate binary for the sandbox. What does it _really_ buy us? > The basic problem with chrooted sandbox is that the process that performs the chroot() call needs superuser permissions. For this, the process should be run with sudo or it should have its SUID flag set. That's, if WebProcess calls the chroot() then it will live with suid rights from the beginnig until its end, even if we change back its effective user to nobody. Then again, if we push all the problematic tasks into a separated binary and give rights only to this binary, then we have to take care of this little codebase only security-wise, not of the whole WebProcess. You're right. We don't want the WebProcess to be SUID, also considering that it links against the rest of WebKit, which has global object constructors that may be executed before main() enters. That code will be executed with full super user rights.
Comment on attachment 163118 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=163118&action=review > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:281 > + String libraryDependencies; > + // There are two way how we gather the dependencies: > + // 1) Using ldd to obtain the static dependencies of QtWebProcess. > + // 2) Using a develeper defined list of dynamic dependencies (it's gathered empirical way). > + collectLibraryDependencies(libraryDependencies, "/lib64/ld-linux-x86-64.so.2", (char*)(executablePathOfWebProcess().utf8().data())); > + collectLibraryDependencies(libraryDependencies, "/sbin/ldconfig", (char*)("-p")); This part bugs me a little, because I think it's going to be rather error prone due to many hard-coded paths and assumptions. Did you see the approach that Google took? It seems rather elegant: (1) Your suid startup process fires off a second process before exec'ing into the WebProcess. That secondary process was cloned with CLONE_FS, to share the file system namespace. It first sits and waits there on a pipe. (2) Then the suid startup process drops privileges altogether and exec()s into the WebProcess. (3) The web process is loaded by the dynamic linker and the necessary libraries are loaded automatically. The process is not chroot'ed yet, but it's also not running with root rights. (4) Directly after entering main(), the WebProcess writes into a pipe to notify the chroot-me process earlier to _now_ change the root of the filesystem, which it can promptly do and exit.
(In reply to comment #61) > You're right. We don't want the WebProcess to be SUID, also considering that it links against the rest of WebKit, which has global object constructors that may be executed before main() enters. That code will be executed with full super user rights. On second thought, I think the code for the suid sandbox helper should ideally be written in plain C and not link against any big libraries (not even WTF). It should be a goal to minimize the amount of code executed before dropping uid zero, and in a nutshell all that code needs to do is to do a few system calls with a few string operations, nothing that fundamentally requires C++ libraries (or requires even calling another process such as LDD to be also executed with root rights!)
(In reply to comment #63) > (In reply to comment #61) > > You're right. We don't want the WebProcess to be SUID, also considering that it links against the rest of WebKit, which has global object constructors that may be executed before main() enters. That code will be executed with full super user rights. > > On second thought, I think the code for the suid sandbox helper should ideally be written in plain C and not link against any big libraries (not even WTF). It should be a goal to minimize the amount of code executed before dropping uid zero, and in a nutshell all that code needs to do is to do a few system calls with a few string operations, nothing that fundamentally requires C++ libraries (or requires even calling another process such as LDD to be also executed with root rights!) Also, note that the system used by google to do that is available as a separate reusable project: http://code.google.com/p/setuid-sandbox/ How about reusing that code?
Okay, I feel like it's time to discuss the next step. I agree with Simon on that using ldd, wtf and constatly burned dependencies are not the best solutions. Moreover, it's useful taking over ideas from google too. But how should we carry out this? If I'm not wrong, what Guillaume alluded to is just a script what you can chroot a process with, after you have installed the sandboxme tool on your system: "Example : $ /usr/sbin/sandboxme -- /bin/sh" But how do we want to use this? The script itself? Or its source? Or ideas from its source? Or what the community want to have in WK2 as chroot sandbox? I'm waiting for everybody's opinion, especially for the security experts.
(In reply to comment #65) > Okay, I feel like it's time to discuss the next step. Agreed :). New interesting things have come up. > I agree with Simon on that using ldd, wtf and constatly burned dependencies are not the best solutions. Moreover, it's useful taking over ideas from google too. But how should we carry out this? > If I'm not wrong, what Guillaume alluded to is just a script what you can chroot a process with, after you have installed the sandboxme tool on your system: > > "Example : > $ /usr/sbin/sandboxme -- /bin/sh" > > But how do we want to use this? The script itself? Or its source? Or ideas from its source? Or what the community want to have in WK2 as chroot sandbox? I'm waiting for everybody's opinion, especially for the security experts. I think what Guillaume might be referring to is to use the sandboxme binary directly to start the QtWebProcess and then inside QtWebProcess use libsandboxme to complete the chroot'ing.
(In reply to comment #66) > (In reply to comment #65) > > Okay, I feel like it's time to discuss the next step. > > Agreed :). New interesting things have come up. > > > I agree with Simon on that using ldd, wtf and constatly burned dependencies are not the best solutions. Moreover, it's useful taking over ideas from google too. But how should we carry out this? > > If I'm not wrong, what Guillaume alluded to is just a script what you can chroot a process with, after you have installed the sandboxme tool on your system: > > > > "Example : > > $ /usr/sbin/sandboxme -- /bin/sh" > > > > But how do we want to use this? The script itself? Or its source? Or ideas from its source? Or what the community want to have in WK2 as chroot sandbox? I'm waiting for everybody's opinion, especially for the security experts. > > I think what Guillaume might be referring to is to use the sandboxme binary directly to start the QtWebProcess and then inside QtWebProcess use libsandboxme to complete the chroot'ing. Yes, that's what I meant. I don't really have an opinion (yet?) on whether to have setuid-sandbox as a dependency or to copy it in the tree.
> > I think what Guillaume might be referring to is to use the sandboxme binary directly to start the QtWebProcess and then inside QtWebProcess use libsandboxme to complete the chroot'ing. > > Yes, that's what I meant. I don't really have an opinion (yet?) on whether to have setuid-sandbox as a dependency or to copy it in the tree. I've checked this project and tried the example attached to it. My experiences are the followings: * The last commit in this git repo is one year old and it contains a lot of FIXMEs. It seems to me that it was taken out from the implementation of chromium and while the original solution is maintained and improved, this version didn't change. * However it's a very good base and it would be useful to take over parts from it. E.g.: according to this we could avoid the ldd call. Let's have a closer look to the project: * The package contains the setup of the setuid environment and an example for the target executable. * The main steps of the environment creation are: (1) sanity checks whether we are suid or not? (2) set the capabilities of the process to SETUID, SETGID, SYS_ADMIN, SYS_CHROOT (3) create new PID namespace with sys_clone system call (with CLONE_NEWPID and SIGCHLD flags). We need the previously set SYS_ADMIN capability to be able to do this. From this point every single process inherits from this will live in this new namespace. (4) do the chroot: * do a sys_clone() again with filesystem-sharing (CLONE_FS flag) * restrict the limit of resources to (0,0) in the _untrusted process_ * wait for the chrootme msg (actually a 'C' character) from the child process (in our case this would be the WebProcess) via a socketpair. * if the msg has arrived we chroot ourself into our proc-directory (/proc/self/fd) and send an acknowledgement. * in the trused process we save the pid of the untrusted process and the socket into environment variables. (5) changing uid/gid in the sandbox: there are 4 possible ways to do this: (a) using the uid/gid of the real user (that's no change) (b) using the gid of the SANDBOXUSER (this should be added by the administrator of the OS) (c) using only the gid of ths SANDBOXUSER (d) using a uniqe uid/gid pair but it's unsupported right now (commented out) (6) execute the target process (e.g: WebProcess) The task of the target process is to call the chrootme() function, what simple reads the previously set environment variables and send the 'C' msg to the sandbox process. The requirements of this solutions are: * libcap_dev package should be installed * a new SANDBOXUSER should be added to the system (except if we don't want to change uid/gid) Finally, here is what I suggest: according to this project I can alter my solution for SandboxProcess. It will remain a separated suid binary, just like the sandboxme binary. I would take over the PID NS and the file system NS restrictons (and the other missing security settings too). However I still prefer "nobody" as the user of the sandbox environment. But anyway, we have to agree on only one uid/gid option and don't need the possibilty of choice. Furthermore, WebProcess would call the chrootme() function to notify the SandboxProcess to perform the chroot. Hopefully this way the only modification in the current source will be the branch of calling Sandbox or WebProcess (so no ldd, no ldconfig, etc). At the same time, I have a hunch that we will need a special handling for the runtime dependencies... What do you think? :)
(In reply to comment #68) > > > I think what Guillaume might be referring to is to use the sandboxme binary directly to start the QtWebProcess and then inside QtWebProcess use libsandboxme to complete the chroot'ing. > > > > Yes, that's what I meant. I don't really have an opinion (yet?) on whether to have setuid-sandbox as a dependency or to copy it in the tree. > > I've checked this project and tried the example attached to it. My experiences are the followings: > * The last commit in this git repo is one year old and it contains a lot of FIXMEs. It seems to me that it was taken out from the implementation of chromium and while the original solution is maintained and improved, this version didn't change. Are there any differences between the code in Chromium and the separate project? Perhaps the Chrome folks would be up for sharing this code specifically with us, i.e. allowing for maintenance out of the Chromium tree? > * However it's a very good base and it would be useful to take over parts from it. E.g.: according to this we could avoid the ldd call. [...] > The requirements of this solutions are: > * libcap_dev package should be installed > * a new SANDBOXUSER should be added to the system (except if we don't want to change uid/gid) Do you know if we can also use nobody for this if there is no dedicated user defined? Also it seems Chrome is using something else, because I don't have a special user in my /etc/passwd after installing Chrome. > Finally, here is what I suggest: according to this project I can alter my solution for SandboxProcess. It will remain a separated suid binary, just like the sandboxme binary. I would take over the PID NS and the file system NS restrictons (and the other missing security settings too). > However I still prefer "nobody" as the user of the sandbox environment. But anyway, we have to agree on only one uid/gid option and don't need the possibilty of choice. > Furthermore, WebProcess would call the chrootme() function to notify the SandboxProcess to perform the chroot. Hopefully this way the only modification in the current source will be the branch of calling Sandbox or WebProcess (so no ldd, no ldconfig, etc). At the same time, I have a hunch that we will need a special handling for the runtime dependencies... > > What do you think? :) Sounds good. I think overall it would be great if we can really share code with the Chromium folks. The less duplicated security sensitive code the more eyes auditing it and the better (safer) the outcome :)
(In reply to comment #69) > (In reply to comment #68) > > > > I think what Guillaume might be referring to is to use the sandboxme binary directly to start the QtWebProcess and then inside QtWebProcess use libsandboxme to complete the chroot'ing. > > > > > > Yes, that's what I meant. I don't really have an opinion (yet?) on whether to have setuid-sandbox as a dependency or to copy it in the tree. > > > > I've checked this project and tried the example attached to it. My experiences are the followings: > > * The last commit in this git repo is one year old and it contains a lot of FIXMEs. It seems to me that it was taken out from the implementation of chromium and while the original solution is maintained and improved, this version didn't change. > > Are there any differences between the code in Chromium and the separate project? Perhaps the Chrome folks would be up for sharing this code specifically with us, i.e. allowing for maintenance out of the Chromium tree? What Chromium does is more sophisitcated but based on this solution. As I mentioned before this is an one year old project (what means that it hasn't changed in the last year). So while the equivalent code was improved inside the trunk of chromium, this version remained the same. Although I can update it to the current version, but it's clear that we cannot use the same version like Chromium does, because the structure of the projects are different. They have one broker process and more renderer processes. These renderers are sandboxed separatelly. But since the design of chromium project aims to be secure from the beginning, these renderers don't need to access any resources in the filesystem. In spite of this, WebProcess uses font files, cache directories, etc. And this is where I stand right now. Sandboxme is built into WebKit2, but due to some missing runtime dependencies (and missing resource directories) it crashes. So I think the best thing would be merging somehow the two solutions. And I'm working on it now. > > * However it's a very good base and it would be useful to take over parts from it. E.g.: according to this we could avoid the ldd call. > [...] > > > The requirements of this solutions are: > > * libcap_dev package should be installed > > * a new SANDBOXUSER should be added to the system (except if we don't want to change uid/gid) > > Do you know if we can also use nobody for this if there is no dedicated user defined? I cannot see any objections why we couldn't do this. > Also it seems Chrome is using something else, because I don't have a special user in my /etc/passwd after installing Chrome. Yes, this project contains a lot of options how to drop the privileges. However chromium simply falls back to the original uid/gid pair.
(In reply to comment #70) > And this is where I stand right now. Sandboxme is built into WebKit2, but due to some missing runtime dependencies (and missing resource directories) it crashes. So I think the best thing would be merging somehow the two solutions. And I'm working on it now. Well... I'm a bit confused right now. I've found the reason of the crash but I don't know how to solve it. The problem is the following: First we cloned the suidhelper process with CLONE_NEWPID flag. This way the current process got the PID 1 in a new namespace and its every children live in this namespace. OK. This way we eliminated our processes from the others. In the second turn we cloned ourself again, with CLONE_FS flag, what resulted that both the calling process and its children will share the same filesystem, regarding the results of several FS operations called either from parent or from a child process (such functions are chroot, chdir, umask). This way if we call chroot() in suidhelper, webprocess also will be sandboxed. Furthermore we have also done a resource restricion with setrlimit (with RLIMIT_NOFILE flag), this way the child (webprocess) cannot access more than 0 filedescriptor (it cannot read anyhing by mistake). To summarize, we created a pure PID NS, "merged" the FS of suidhelper and webprocess, started the webprocess (it loaded its static dependencies), after starting it sends immadiately the "chrootme" signal to suidhelper and gets sandboxed. However, we don't have our runtime depdendencies, the cache directories and the plugins. How could we solve this? Ideas? I tried the follwoing: I took a proper sandbox environment from my previous solution without /proc and /run/shm directories and tried to chroot ourself here instead of /proc/self/fdinfo. However I still had the crash. My next idea was to create a new /proc inside this chroot and bind just the proc directory of WebProcess. The problem with this is, when we start the webprocess we are in a new pid namespace. And if I get the pid of webprocess here and mount it into my /proc then I'd do a mistake because they are in different namespaces. Contrarly, if i mount the whole original /proc directory into the sandbox environment, then it works. :O Well... I hope that I was clear enough. Does anybody have any bright idea? :)
(In reply to comment #71) > (In reply to comment #70) > > And this is where I stand right now. Sandboxme is built into WebKit2, but due to some missing runtime dependencies (and missing resource directories) it crashes. So I think the best thing would be merging somehow the two solutions. And I'm working on it now. > Well... I'm a bit confused right now. I've found the reason of the crash but I don't know how to solve it. The problem is the following: > First we cloned the suidhelper process with CLONE_NEWPID flag. This way the current process got the PID 1 in a new namespace and its every children live in this namespace. OK. This way we eliminated our processes from the others. In the second turn we cloned ourself again, with CLONE_FS flag, what resulted that both the calling process and its children will share the same filesystem, regarding the results of several FS operations called either from parent or from a child process (such functions are chroot, chdir, umask). This way if we call chroot() in suidhelper, webprocess also will be sandboxed. Furthermore we have also done a resource restricion with setrlimit (with RLIMIT_NOFILE flag), this way the child (webprocess) cannot access more than 0 filedescriptor (it cannot read anyhing by mistake). > To summarize, we created a pure PID NS, "merged" the FS of suidhelper and webprocess, started the webprocess (it loaded its static dependencies), after starting it sends immadiately the "chrootme" signal to suidhelper and gets sandboxed. However, we don't have our runtime depdendencies, the cache directories and the plugins. How could we solve this? Ideas? > I tried the follwoing: I took a proper sandbox environment from my previous solution without /proc and /run/shm directories and tried to chroot ourself here instead of /proc/self/fdinfo. However I still had the crash. > My next idea was to create a new /proc inside this chroot and bind just the proc directory of WebProcess. The problem with this is, when we start the webprocess we are in a new pid namespace. And if I get the pid of webprocess here and mount it into my /proc then I'd do a mistake because they are in different namespaces. > Contrarly, if i mount the whole original /proc directory into the sandbox environment, then it works. :O > Well... I hope that I was clear enough. Does anybody have any bright idea? :) Let's see if I get this right. This approach could work, but it has two problems: (1) We're missing the plugins and their dependencies (2) We won't share the disk cache directories (as well as icondb, appcache, etc.) Regarding the plugins I suggest we ignore them for the moment. They're fortunately becoming less and less relevant. Regarding the disk cache directories we do have a problem, but in a way that's generic to the web process trying to access any file, isn't it? So do we need a way of allowing fine-grained file access from this sand-box (perhaps using a request pattern where the uiprocess returns file handles), or could this be also done by bind-mounting the cache directory into the chroot?
(In reply to comment #72) > (In reply to comment #71) > > (In reply to comment #70) > > > And this is where I stand right now. Sandboxme is built into WebKit2, but due to some missing runtime dependencies (and missing resource directories) it crashes. So I think the best thing would be merging somehow the two solutions. And I'm working on it now. > > Well... I'm a bit confused right now. I've found the reason of the crash but I don't know how to solve it. The problem is the following: > > First we cloned the suidhelper process with CLONE_NEWPID flag. This way the current process got the PID 1 in a new namespace and its every children live in this namespace. OK. This way we eliminated our processes from the others. In the second turn we cloned ourself again, with CLONE_FS flag, what resulted that both the calling process and its children will share the same filesystem, regarding the results of several FS operations called either from parent or from a child process (such functions are chroot, chdir, umask). This way if we call chroot() in suidhelper, webprocess also will be sandboxed. Furthermore we have also done a resource restricion with setrlimit (with RLIMIT_NOFILE flag), this way the child (webprocess) cannot access more than 0 filedescriptor (it cannot read anyhing by mistake). > > To summarize, we created a pure PID NS, "merged" the FS of suidhelper and webprocess, started the webprocess (it loaded its static dependencies), after starting it sends immadiately the "chrootme" signal to suidhelper and gets sandboxed. However, we don't have our runtime depdendencies, the cache directories and the plugins. How could we solve this? Ideas? > > I tried the follwoing: I took a proper sandbox environment from my previous solution without /proc and /run/shm directories and tried to chroot ourself here instead of /proc/self/fdinfo. However I still had the crash. > > My next idea was to create a new /proc inside this chroot and bind just the proc directory of WebProcess. The problem with this is, when we start the webprocess we are in a new pid namespace. And if I get the pid of webprocess here and mount it into my /proc then I'd do a mistake because they are in different namespaces. > > Contrarly, if i mount the whole original /proc directory into the sandbox environment, then it works. :O > > Well... I hope that I was clear enough. Does anybody have any bright idea? :) > > Let's see if I get this right. This approach could work, but it has two problems: > > (1) We're missing the plugins and their dependencies Not just the dependencies of plugins but such libaries like libresolv, libdbus, etc (up to ~11 libraries)... actually what were added as constants in my previous solution (you can see the list in the last uploaded patch). > (2) We won't share the disk cache directories (as well as icondb, appcache, etc.) Right. > Regarding the plugins I suggest we ignore them for the moment. They're fortunately becoming less and less relevant. Ok. > Regarding the disk cache directories we do have a problem, but in a way that's generic to the web process trying to access any file, isn't it? Yes. I would describe the problem like: if we chroot ourself into /proc/self then we miss runtime dependencies. Contrarly, if we are chrooted into a generated environment, then we miss our needed /proc/self entries (because of the different namespaces). At least I just think that this it the problem, because I don't have a working solution yet. > So do we need a way of allowing fine-grained file access from this sand-box (perhaps using a request pattern where the uiprocess returns file handles), or could this be also done by bind-mounting the cache directory into the chroot? Maybe. Altogheter, we need the ~11 libraries mentioned above and 2 libraries from the users home (caches). Theoretically...
(In reply to comment #73) > I would describe the problem like: if we chroot ourself into /proc/self then we miss runtime dependencies. Contrarly, if we are chrooted into a generated environment, then we miss our needed /proc/self entries (because of the different namespaces). At least I just think that this it the problem, because I don't have a working solution yet. > > So do we need a way of allowing fine-grained file access from this sand-box (perhaps using a request pattern where the uiprocess returns file handles), or could this be also done by bind-mounting the cache directory into the chroot? > Maybe. Altogheter, we need the ~11 libraries mentioned above and 2 libraries from the users home (caches). Theoretically... Well, I debugged a bit and I have more than a theory already. So after "strace"-ing QtWebProcess we can see that it operates on the followings: * /run/shm (shared memyory): maybe we could mount a new empty tmpfs here? (with a limited memorysize) * /proc/self (in our case this is a symlink to the proc entry of QtWebprocess) * /proc/meminfo (file, not directory): no idea how to recreate/copy/whatever it into the sandbox environment without mount --bind the whole /proc * /proc/net (symlink to /proc/self/net) * $HOME/.local (I can hard link them into the sandbox) * $HOME/.cache (ditto) * /dev/random and urandom (i can recreate them easily. simple, quick, secure, no problem) * font directories in /etc and /usr/share (hard link them too) * a few runtime libraries: libnss, libresolv, libssl (link them too). To locate them i can still use ldconfig (or maybe pkgconfig?) So my only real problem is the binding of /proc FS. But since WebProcess runs in a separated PID namespace maybe we can live with this until we don't have a better idea. (Or does anybody have it already?) Anyway... I'll upload the current version with the built-in sandboxme soon, but 'till then I'm open for any good suggestions.
Created attachment 168044 [details] Proposed patch
Last night I was too tired to write a comment to the patch. So I do it now: Basically it contains the modification we talked about earlier: * The creation of the sandbox environment is similar to the one in previous versions, but we don't need parsing ldd anymore (however ldconfig is still needed to detect the path of 4 runtime libraries). * After this we restrict the process capabilities to a minmal needed set (CAP_SETUID, CAP_SETGID, CAP_SYS_ADMIN, CAP_SYS_CHROOT). * Next, we clone ourself twice what will bring us into a new PID NS and will share the FS with the sandbox with restrictions. This last step causes that if we call the chroot() in the sandbox helper process, then its child (that's the WebProcess) also will be chrooted. * Drop the privileges (we fallback to the original user uid/gid right now). * Start the WebProcess. * In the WebProcess we call the chrootMe() function what sends a message to the helper, who call the chroot() in turn. And tadaaaam! We are sandboxed :)
Some further notes: * Since we only used plain C functions, this patch is totally platform independent. Even the WebProcess itself is called via execl(). * The patch contains a lot of string manipulation operation. To avoid the using of iostream, WTFString or QString, etc I had to wrote a few string manipulator functions. * Many ideas are taken from sandboxMe project but its strongly refactored, adapted to our specific needs and style.
Comment on attachment 168044 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=168044&action=review > Tools/qmake/mkspecs/features/default_post.prf:290 > +QMAKE_LIBS_OPENGL += -lcap I almost forgot... I know this is totally not the right place to add a linker flag to the SandboxProcess, but temporarly it worked and I'm sure that you can quickly tell me how to do it properly.
Comment on attachment 168044 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=168044&action=review >> Tools/qmake/mkspecs/features/default_post.prf:290 >> +QMAKE_LIBS_OPENGL += -lcap > > I almost forgot... I know this is totally not the right place to add a linker flag to the SandboxProcess, but temporarly it worked and I'm sure that you can quickly tell me how to do it properly. You're already doing it in SandboxProcess.pro, shouldn't be needed here.
Here are some comments: * SandboxProcess seems very Unix/Linux specific, so It shouldn't go into its own toplevel directory. Mac will never use anything from SandboxProcess for example. * Don't use "using namespace std". STL functions should be prefixed by std:: * SandboxEnvironmentLinux.cpp looks like it's reinventing a bunch of string functions. Is there a reason why WTF::CString can't be used? (Or at least std::string?) * Comments like // Get a new PID namespace. static bool moveToNewPIDNamespace() aren't useful. * The code in general lacks comments and is very hard to follow for someone not familiar with how Linux sandboxing works.
(In reply to comment #79) > (From update of attachment 168044 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168044&action=review > > >> Tools/qmake/mkspecs/features/default_post.prf:290 > >> +QMAKE_LIBS_OPENGL += -lcap > > > > I almost forgot... I know this is totally not the right place to add a linker flag to the SandboxProcess, but temporarly it worked and I'm sure that you can quickly tell me how to do it properly. > > You're already doing it in SandboxProcess.pro, shouldn't be needed here. Yes, the plan was simply adding LIBS += -lcap to SandboxProcess.pro but this way the linker failed because it couldn't find the libcap library.
(In reply to comment #80) > Here are some comments: > > * SandboxProcess seems very Unix/Linux specific, so It shouldn't go into its own toplevel directory. Mac will never use anything from SandboxProcess for example. Yeah, I thought it's not the best place for this. What about WebKit2/Shared/linux? > * Don't use "using namespace std". STL functions should be prefixed by std:: Auch. In a previous local version I used std::string-s, but they are completly abolished already, so this line is superfluous. > * SandboxEnvironmentLinux.cpp looks like it's reinventing a bunch of string functions. Is there a reason why WTF::CString can't be used? (Or at least std::string?) We've talked about it earlier. The conclusion from #63 is: "the code for the suid sandbox helper should ideally be written in plain C and not link against any big libraries (not even WTF)." > * Comments like > > // Get a new PID namespace. > static bool moveToNewPIDNamespace() > > aren't useful. All right, I'll remove them. > * The code in general lacks comments and is very hard to follow for someone not familiar with how Linux sandboxing works. Ofc, this is just a "hey guys, here is what I have" patch. I'll add comments later. :)
Comment on attachment 168044 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=168044&action=review Some quick notes from scrolling through the patch. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:126 > + if (!stat(SAFEDIR, &safedirInfo) && S_ISDIR(safedirInfo.st_mode)) You probably want to use lstat() instead of stat() everywhere in this code to protect against symlink attacks. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:146 > + // Child. I think this code would be a bit clearer if you move the logic that executes in the child process to a separate functions. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:301 > + if (mkdir(pathToCreateInSandbox, 0777) == -1 && errno != EEXIST) { 777 seems like a poor choice of permissions. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:474 > +static bool filterAndLinkLibraryDependenciesFromLdconfigOutput(char* buffer, int bufferLength) I want to believe that there's a better way of doing this than parsing output from a shell command. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:564 > + if (mkdir(SAFEDIR, 0777) == -1 && errno != EEXIST) 777 seems like a poor choice of permissions. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:573 > + const char* home = getenv("HOME"); I would look up the home directory via getpw() instead of reading environment variable. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:656 > + stringConcat(safedir, getenv("HOME"), "/.wk2-sandbox", PATHSIZE); I would look up the home directory via getpw() instead of reading environment variable. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.cpp:707 > + if (!geteuid() || !getegid() || !setuid(0) || !setgid(0)) { > + fprintf(stderr, "My euid or egid is 0! Something went really wrong\n"); This logic could be more readable. > Source/WebKit2/SandboxProcess/SandboxEnvironmentLinux.h:46 > +class SandboxEnvironmentLinux { > +public: > + static int launchChrootHelper(void); > + static int dropPrivileges(uid_t, gid_t); > + static int moveToNewPIDNamespace(void); > + static int setDumpable(void); > + static int setCapabilities(cap_value_t capablityList[], int ncap); > +}; None of this class is actually implemented in the .cpp file.
Created attachment 168993 [details] Proposed patch
Comment on attachment 168993 [details] Proposed patch Attachment 168993 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14384298
Stupid ews... she knew the -lcap flag in the previous version. In spite of this I 'd say a few words about this patch because it contains two important new parts. First, the whole sandboxprocess is moved into WebKit2/Shared/linux/ directory. Secondly, calling the ldconfig is dropped out and we use a dlopen magic instead of it now. This dlopen opens the given runtime library and got as a result a handle. With this dlinfo is able to obtain the path of the library and we can link it into the sandbox environment. The further changes tends to fulfill the previous requests except the comments. I will add more.
Created attachment 169192 [details] Proposed patch Fixed the previous build issue.
Created attachment 169652 [details] Proposed patch In this patch WebProcess runs as "nobody" inside the jail. Furthermore it contains a bigger refactor: stringoperations went into separated files and the code was formed to webkit-style.
Created attachment 170358 [details] Proposed patch
The new patch aims to avoid the ews failure (the actual revision was broken and probably it was the problem). Furthermore, I've removed the workaround from the build environment. The problem was that the new source files were added to Target.pri, but since SandboxProcess isn' t part of WK2, it was wrong. And this is why I had to add the linker flags globally. So it's fixed now.
Comment on attachment 170358 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=170358&action=review > Source/WebKit2/Shared/linux/SandboxProcess/StringOperations.h:31 > +// Secure string operations. > +bool stringCopy(char*, const char*, int); > +bool stringCopy(char*, const char*, const int, int); > +bool stringConcat(char*, const char*, const char*, int); > +bool stringConcat(char*, const char*, const char*, const char*, int); > +bool stringAppend(char*, const char*, int); I don't see what's "secure" about these functions. Please use the standard C library string functions instead. You're already using memcpy() from string.h to implement them. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:72 > +#define SBX_D "SBX_D" > +#define SBX_HELPER_PID "SBX_HELPER_PID" > + > +#define MSG_CHROOTME 'C' > +#define MSG_CHROOTED 'O' You probably want to put these in a common header file instead of copy-pasting them across the codebase.
(In reply to comment #91) > (From update of attachment 170358 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170358&action=review > > > Source/WebKit2/Shared/linux/SandboxProcess/StringOperations.h:31 > > +// Secure string operations. > > +bool stringCopy(char*, const char*, int); > > +bool stringCopy(char*, const char*, const int, int); > > +bool stringConcat(char*, const char*, const char*, int); > > +bool stringConcat(char*, const char*, const char*, const char*, int); > > +bool stringAppend(char*, const char*, int); > > I don't see what's "secure" about these functions. > Please use the standard C library string functions instead. > You're already using memcpy() from string.h to implement them. The reason why I reimplemented these concat functions: Let's take a look at strncat(dest, src, n) in string.h. What if the number of characters to append is more that n? The reference says: "If the length of the C string in source is less than num, only the content up to the terminating null-character is copied." But it doesn't say a word about the case if the length of src > n. So the result is implementation dependent and not deterministic. What's more you don't get any error messages. And since our primary goal is security we cannot rely on this.
(In reply to comment #92) > The reason why I reimplemented these concat functions: Let's take a look at strncat(dest, src, n) in string.h. What if the number of characters to append is more that n? The reference says: "If the length of the C string in source is less than num, only the content up to the terminating null-character is copied." But it doesn't say a word about the case if the length of src > n. So the result is implementation dependent and not deterministic. What's more you don't get any error messages. And since our primary goal is security we cannot rely on this. I don't understand what you're talking about here. Could you give a concrete example in code?
I only read the first 20 or so comments, so sorry if there's a repeat, but the Chrome suid sandbox (the non-seccomp one) relies on a helper binary to reduce complexity in the high-privilege process. You can see the code here: http://code.google.com/searchframe#OAMlx_jo-ck/src/sandbox/linux/suid/sandbox.c&exact_package=chromium&q=sandbox.c and the other files in that directory. You can read more about it here: http://code.google.com/p/chromium/wiki/LinuxSUIDSandbox There's a bunch of tricky details in there, worth taking a look.
> I don't understand what you're talking about here. Could you give a concrete example in code? I understand her, and it seems a smart approach. What is happening, if you do strncat(a, b, 5), where a = "abc" b = "cde"? The answer is: implementation dependent. The behaviour of her code is defined: it displays an error message, and returns with an error code. So there is no random behaviour accross platforms, and it does not process a string further, if there is not enough space in the buffer (like creating wrong files, when you concat a path and a filename).
> I only read the first 20 or so comments, so sorry if there's a repeat, but the Chrome suid sandbox (the non-seccomp one) relies on a helper binary to reduce complexity in the high-privilege process. This patch also creates a helper binary. But its purpose is not "reducing complexity" (???) it creates a separate pid namespace and drop privileges.
(In reply to comment #94) > I only read the first 20 or so comments, so sorry if there's a repeat, but the Chrome suid sandbox (the non-seccomp one) relies on a helper binary to reduce complexity in the high-privilege process. > > You can see the code here: > http://code.google.com/searchframe#OAMlx_jo-ck/src/sandbox/linux/suid/sandbox.c&exact_package=chromium&q=sandbox.c > and the other files in that directory. > > You can read more about it here: > http://code.google.com/p/chromium/wiki/LinuxSUIDSandbox > > There's a bunch of tricky details in there, worth taking a look. Thanks, I've checked it already and I also took a few ideas from there. Unfortunatelly we couldn't use it in its current form because the architecture of webkit is different from chrome. AFAIK chrome has a broker process and sandboxed render processes what don't need file system access at all. However webprocess, what should be sandboxed here, opens fonts, caches, loads runtime dependencies (eg. with dlopen) and these resources have to be present in the sandbox environment.
Created attachment 172529 [details] Proposed patch
Attachment 172529 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/QtWebKit.pro', u'Source/WebKit2/Con..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebKit2/Shared/linux/SandboxProcess/StringOperations.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 172532 [details] Proposed patch
(In reply to comment #91) > (From update of attachment 170358 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170358&action=review > I don't see what's "secure" about these functions. > Please use the standard C library string functions instead. > You're already using memcpy() from string.h to implement them. Since we don't get any objections after the explanation of string operations, I've left them in the code. > > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:72 > > +#define SBX_D "SBX_D" > > +#define SBX_HELPER_PID "SBX_HELPER_PID" > > + > > +#define MSG_CHROOTME 'C' > > +#define MSG_CHROOTED 'O' > > You probably want to put these in a common header file instead of copy-pasting them across the codebase. Right, done. Both WebProcess and SandboxProcess use a common header right now.
Style checker will fail, because config.h isn't included. The reason of this is that Sandboxprocess shouldn't use anything from webkit at all, neither classes, functions nor macros. We'd only need config.h to be able to use the ENABLE macro (from platform.h), however in QtWebKit.pro we already ensured that sandboxprocess will be only built if sandbox is enabled, so we don't need to do it again in the source too. Ofc I could create a dummy config.h to make stylechecker happy, but I don't see much sense in that. I runned layout tests inside sandbox. The only failing tests are gstreamer-related (e.g. tests with video content). The problem is caused by few missing libraries from the sandbox. Although the "main" gstreamer libraries are loaded before WebProcess get sandboxed, it seems that gstreamer also has a plugin system. It means that it opens further shared libraries with dlopen() what also have dependencies. This way the library list could be long, so it doesn't worth to add them one-by-one to the sandbox. But since it only affects ~10 tests, I don't intend to solve it in this first patch. Furthermore, I've measured the pageloads with methanol (with and without sandbox). For this I used a collection of real world websites from alexa top50. The results are shown below. You can see that the difference is ~0.05% (every tests were runned 20 times and the variance was up to 4%). TEST W/O SB W/ SB DIFF sample/baidu/baidu.html 7,35 7,3 0,05 sample/bing/bing.html 12,45 12,4 0,05 sample/blogger/blogger.html 19,9 19,7 0,2 sample/conduit/conduit.html 88,25 86,65 1,6 sample/ebay/ebay.html 51,7 51,95 -0,25 sample/facebook/facebook.html 37,35 37,45 -0,1 sample/fc2/fc2.html 87,65 88,45 -0,8 sample/google/google.html 10,3 10,3 0 sample/linkedin/linkedin.html 56,45 53,65 2,8 sample/live/live.html 12,15 12,05 0,1 sample/mail/mail.html 65,35 68,25 -2,9 sample/microsoft/microsoft.html 79,3 80,05 -0,75 sample/msn/msn.html 153,55 152,8 0,75 sample/qq/qq.html 88,35 89,65 -1,3 sample/sina/sina.html 275,6 275,6 0 sample/taobao/taobao.html 502 503,3 -1,3 sample/twitter/twitter.html 70,2 71,05 -0,85 sample/wikipedia/Wikipedia.html 36,25 36,5 -0,25 sample/wordpress/wordpress.html 79,05 85,55 -6,5 sample/yahoo/yahoo.html 63,05 63,35 -0,3 sample/yandex/yandex.html 101,6 102,1 -0,5 sample/youtube/youtube.html 105,1 106,45 -1,35 ------------------------------------------------------ SUMMARY: 2002,95 2014,55 -11,6
Attachment 172532 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/QtWebKit.pro', u'Source/WebKit2/Con..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebKit2/Shared/linux/SandboxProcess/StringOperations.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #95) > > I don't understand what you're talking about here. Could you give a concrete example in code? > > I understand her, and it seems a smart approach. What is happening, if you do strncat(a, b, 5), where a = "abc" b = "cde"? The answer is: implementation dependent. The behaviour of her code is defined: it displays an error message, and returns with an error code. So there is no random behaviour accross platforms, and it does not process a string further, if there is not enough space in the buffer (like creating wrong files, when you concat a path and a filename). So you're writing _new_ code to do raw string manipulation (one of the most error prone operations when it comes to buffer overflow), to get around the fact that there may be _platform differences_ on code that will only run on Linux? Even using the STL would be better than that. I'ms till not convinced.
(In reply to comment #104) > So you're writing _new_ code to do raw string manipulation (one of the most error prone operations when it comes to buffer overflow)... For example, this code stores the return value from strlen() in int variables. strlen() returns size_t. What happens if a string is longer than std::numeric_limits<int>::max()?
> For example, this code stores the return value from strlen() in int variables. strlen() returns size_t. What happens if a string is longer than std::numeric_limits<int>::max()? So you suggest to replace strlen with our own code? That is a good idea! It could have a max value, and returns with that if the string is longer than that.
(In reply to comment #105) > (In reply to comment #104) > > So you're writing _new_ code to do raw string manipulation (one of the most error prone operations when it comes to buffer overflow)... > > For example, this code stores the return value from strlen() in int variables. strlen() returns size_t. What happens if a string is longer than std::numeric_limits<int>::max()? Maybe we should use the size_t strnlen(const char *s, size_t maxlen) function. (?)
> size_t strnlen(const char *s, size_t maxlen) This function is even more strict than what we need. It is a good idea to use it.
Checking the comments it seems the only remaining issue is string manipulation functions. The problem with strncat, it does not report an error, if the concatenation is unsuccessful. Consider the following example: sandbox creates files using a directory, which was passed as an argument. You concatenate the dir and the name of the file: strncat("dir_path", "/file", maxlen - strlen(dest) - 1). However, if n = 0, the result will be the dir_path, which can be a tricky name of a file, e.g: path/././././i_want_to_create_this_file And nothing is reported by strncat! Or even worse, n can be negative, which will be converted as a huge positive number, since n is unsigned. We want to avoid these, we want to know if a concatenation is not successful! I think Reni did a great job here, so if nothing else comes up, I think this work is ready to land. We can still fix things later, e.g if we invent a better idea for strings.
I broke up the first paragraph in multiple lines so I could more easy answer everything: (In reply to comment #109) > Checking the comments it seems the only remaining issue is string manipulation functions. This is not true. I haven't looked closely at the rest of the patch. I focused on the string manipulation functions because I think it's a mistake to reimplement string functions that already exist in WTF, STL and Libc. > The problem with strncat, it does not report an error, if the concatenation is unsuccessful. Consider the following example: sandbox creates files using a directory, which was passed as an argument. You concatenate the dir and the name of the file: strncat("dir_path", "/file", maxlen - strlen(dest) - 1). However, if n = 0, the result will be the dir_path, which can be a tricky name of a file, e.g: path/././././i_want_to_create_this_file And nothing is reported by strncat! Thanks for the explanation. I still don't see why either std::string or even WTF::CString can be used for this. Reading the bug, it seems that Reni used std::string in a previous version of this patch. > I think Reni did a great job here, so if nothing else comes up, I think this work is ready to land. I agree that the patch is a good start, but it has not yet been fully reviewed and neither mine nor Andreas's comments have been fully addressed so I don't think this patch is ready to land just yet. > We can still fix things later, e.g if we invent a better idea for strings. Landing patches where multiple reviewers have addressed concerns with the promise of "we'll fix things later!" is not something we aim to do in the WebKit project because most likely things will not be fixed later.
> Thanks for the explanation. I still don't see why either std::string or even WTF::CString can be used for this. Reading the bug, it seems that Reni used std::string in a previous version of this patch. I am worried about out-of-memory conditions. It would be better if the process with root privileges would never crash. > I agree that the patch is a good start, but it has not yet been fully reviewed and neither mine nor Andreas's comments have been fully addressed so I don't think this patch is ready to land just yet. Fair enough. Please help us and fully review the patch. > Landing patches where multiple reviewers have addressed concerns with the promise of "we'll fix things later!" is not something we aim to do in the WebKit project because most likely things will not be fixed later. Sorry, I used a wrong word. Bugs needs fixing. This is not a bug. It is a completely safe approach. We can improve it further in the future, if we find a better solution.
Any further objections against string operations?
(In reply to comment #112) > Any further objections against string operations? And any other comments, idea, etc. are appreciated!
Created attachment 174148 [details] Proposed patch Since I haven't got further objections after Zoltan's explanation, I dare to upload the patch with StringOperations again. I only made a fix according to Andreas observation, so we don't use the risky strlen() function anymore (instead strnlen() is used).
Attachment 174148 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/QtWebKit.pro', u'Source/WebKit2/Con..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebKit2/Shared/linux/SandboxProcess/StringOperations.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 174148 [details] Proposed patch Hi Reni, I think the patch is good in overall, but I have some comments: View in context: https://bugs.webkit.org/attachment.cgi?id=174148&action=review > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:55 > +enum { PathSize = 512 }; > +char sandboxDirectory[PathSize]; > +uid_t sandboxUserUid; > +uid_t sandboxUserGid; These variables could be static. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:57 > +static void launchChrootHelper(int socketPair[]) A little comment is needed here that this function does not return on success. And you have a lot of variable names with chroot, I would prefer ChangeRoot. Short names are not preferred in WebKit. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:181 > +// Set capabilities in all three sets. Please a little more explanation about these 3 sets. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:304 > + char dev[] = "/dev/"; Why this variable is not const as the other below it. And why it is char[] instead of char*? And some newlines could help in this function the make the code readable. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:332 > + char proc[] = "/proc/"; Ditto. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:404 > + // Making difference between relative and absolute paths. Little more explanation please. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:570 > + if (mkdir(sandboxDirectory, 0100)) { Isn't there a named constant for this 0100? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:673 > + return false; Same "never reached" thing here as above. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.h:27 > +#ifndef CLONE_NEWPID > +#define CLONE_NEWPID 0x20000000 > +#endif A littel explanation why this is needed, and why this is safe to define it if it is not defined (this is a system call argument after all). > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:154 > + long int fd = -1; Full names please. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:158 > + ssize_t cnt; Ditto. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:222 > #endif > - > // Create the connection. Do not remove this line.
Created attachment 174415 [details] Proposed patch Patch according to Zoltan's requests.
Attachment 174415 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebKit2/Shared/linux/SandboxProcess/StringOperations.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 174415 [details] Proposed patch Thanks for fixing everything. Since there was no other comments, r=me But before land, please fix the style issue.
Comment on attachment 174415 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=174415&action=review I'm still of the opinion that rolling your own string functions for code whose sole purpose is increasing security is insane. This version is *still* storing the size_t result of str(n)len in int variables, leaving the code open to arithmetic overflow bugs. And this is just one problem, I'm sure there are more of them hiding in the code. Also, why do you need to send messages back and forth to communicate with the clone() child? Can't you just waitpid() for it and check the exit code? Seems like that would decrease the attack surface of this helper considerably. > Source/WebKit2/Configurations/FeatureDefines.xcconfig:136 > +ENABLE_SUID_SANDBOX_LINUX = ; Why add this to the XCode build? It's not ENABLE_SUID_SANDBOX_LINUX will ever be used on Mac.
(In reply to comment #120) > (From update of attachment 174415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174415&action=review > > I'm still of the opinion that rolling your own string functions for code whose sole purpose is increasing security is insane. Okay-okay... I'm going to remove them... > This version is *still* storing the size_t result of str(n)len in int variables, leaving the code open to arithmetic overflow bugs. And this is just one problem, I'm sure there are more of them hiding in the code. Well, I'm here and I gladly fix anything. Just review it pls :P > Also, why do you need to send messages back and forth to communicate with the clone() child? Can't you just waitpid() for it and check the exit code? Seems like that would decrease the attack surface of this helper considerably. The reason of this communication is to notify SandboxProcess that WebProcess is ready to be sandboxed. If I'd use waitpid(1) ('1' will be the PID of WebProcess inside the new pid namespace) then WebProcess would be jailed before its static libraries are loaded (I'm speaking out of experience: I tried it out and it crashes). > > Source/WebKit2/Configurations/FeatureDefines.xcconfig:136 > > +ENABLE_SUID_SANDBOX_LINUX = ; > > Why add this to the XCode build? It's not ENABLE_SUID_SANDBOX_LINUX will ever be used on Mac. Right. Removed.
> This version is *still* storing the size_t result of str(n)len in int variables, leaving the code open to arithmetic overflow bugs. And this is just one problem, I'm sure there are more of them hiding in the code. What is the purpose of this game? You are smart enough to know that overflow is not possible. Strnlen returns a value between 0 and n (512). Even 2 * n (512) is way less than 2G. Even if we increase it to 10000, we will have no problem. The root process is intended to run without many dependencies. It does not use WK or STL or other utilities, only system calls. You disappeared from IRC, you did not say anything here. I suspect there is something you don't want to tell us. At least tell me in privately.
(In reply to comment #122) > > This version is *still* storing the size_t result of str(n)len in int variables, leaving the code open to arithmetic overflow bugs. And this is just one problem, I'm sure there are more of them hiding in the code. > > What is the purpose of this game? You are smart enough to know that overflow is not possible. Strnlen returns a value between 0 and n (512). Even 2 * n (512) is way less than 2G. Even if we increase it to 10000, we will have no problem. The root process is intended to run without many dependencies. It does not use WK or STL or other utilities, only system calls. This is not about strnlen() specifically. This is about having a sane approach to writing secure software. You should keep the amount of code that runs in a privileged state to an absolute minimum. You should re-use well tested library functionality instead of writing your own things. You should handle any and all errors. (There's tons of missing error handling: fclose(), fwrite(), stat(), lstat(), execl(), ...) As for the "out of memory conditions" argument - this code is for Linux, where malloc() never returns NULL. In the absolute worst-case scenario the OOM killer will just kill your process. > You disappeared from IRC, you did not say anything here. I suspect there is something you don't want to tell us. wat
> As for the "out of memory conditions" argument - this code is for Linux, where malloc() never returns NULL. In the absolute worst-case scenario the OOM killer will just kill your process. Source: http://www.kernel.org/doc/man-pages/online/pages/man3/malloc.3.html RETURN VALUE For calloc() and malloc(), return a pointer to the allocated memory, which is suitably aligned for any kind of variable. On error, these functions return NULL. NULL may also be returned by a successful call to malloc() with a size of zero, or by a successful call to calloc() with nmemb or size equal to zero. NOTES By default, Linux follows an optimistic memory allocation strategy. This means that when malloc() returns non-NULL there is no guarantee that the memory really is available. In case it turns out that the system is out of memory, one or more processes will be killed by the OOM killer. For more information, see the description of /proc/sys/vm/overcommit_memory and /proc/sys/vm/oom_adj in proc(5), and the kernel source file Documentation/vm/overcommit-accounting. So malloc can return with NULL. OOM only handles those cases, where malloc returns non-NULL, and still the memory is not available.
> You should handle any and all errors. (There's tons of missing error handling: fclose(), fwrite(), stat(), lstat(), execl(), ...) This is a valid point. However: http://linux.die.net/man/3/execl The exec() functions only return if an error has have occurred. The return value is -1, and errno is set to indicate the error. So we just return with fail if the function returns with anything. Also checking the error code of fclose() and other free like functions can throw warnings, but these are not exactly errors, since the process can continue.
(In reply to comment #124) > > So malloc can return with NULL. OOM only handles those cases, where malloc returns non-NULL, and still the memory is not available. In that case, if you use the WTF::CString class and we can't allocate memory we'll crash. If you use std::string, it'll throw an std::bad_alloc exception and terminate (through a call to std::terminate). I remain unconvinced that you need custom string functions for security.
> In that case, if you use the WTF::CString class and we can't allocate memory we'll crash. If you use std::string, it'll throw an std::bad_alloc exception and terminate (through a call to std::terminate). I remain unconvinced that you need custom string functions for security. For simplicity the process with root priveliges does not use anything from WebKit, STL or Qt at the moment. For example, if you add Qt libs, the fork with a new process namespace does not work (don't ask me why, probably Qt initializes something, which conflicts with the fork). I am not sure you can add anything from WebKit without Qt (due to the config.h/Platform.h dependency). STL is huge as well, and you need to use exception handling, which is uncommon in WebKit. Anyway, I don't care about string manipulations anymore. Probably std::string is the only viable way then.
Created attachment 174722 [details] Proposed patch
Here is a cool new version of suid sandbox! Without my little string operations, only with cstrings! All functions return values are double checked! Only here, only now, only for you! ;) Love and kisses! ;)
Attachment 174722 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
@kling, @andersca: did you think of something similar?
As mentioned above, all mod requests have been addressed. Could those, who made those requests, please, review the latest version of the patch? Or should we review it in-house?
(In reply to comment #132) > As mentioned above, all mod requests have been addressed. Could those, who made those requests, please, review the latest version of the patch? Or should we review it in-house? Everyone at Apple has the thanksgiving week off so we're not really around. I'll try to review it anyway later this week though. Thanks for addressing our comments!
(In reply to comment #133) > (In reply to comment #132) > > As mentioned above, all mod requests have been addressed. Could those, who made those requests, please, review the latest version of the patch? Or should we review it in-house? > > Everyone at Apple has the thanksgiving week off so we're not really around. I'll try to review it anyway later this week though. Thanks for addressing our comments! Ooh, I didn't know about that. Thank you and happy thanksgiving!
(In reply to comment #133) > (In reply to comment #132) > > As mentioned above, all mod requests have been addressed. Could those, who made those requests, please, review the latest version of the patch? Or should we review it in-house? > > Everyone at Apple has the thanksgiving week off so we're not really around. I'll try to review it anyway later this week though. Thanks for addressing our comments! If you have a little time, don't forget about me pls :)
Comment on attachment 174722 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=174722&action=review > ChangeLog:6 > + Reviewed by Zoltan Herczeg. And me! > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:52 > +static char sandboxDirectory[PathSize]; Doesn't Linux have a MAX_PATH #define? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:54 > +static uid_t sandboxUserUid; > +static uid_t sandboxUserGid; Please make UID and GID all caps. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:65 > +#define strlcpy(destination, source, maxLength) \ > + do { \ > + (destination)[0] = '\0'; \ > + strncat((destination), (source), (maxLength) - 1); \ > + } while (0) > + > +#define strlcat(destination, source, maxLength) \ > + do { \ > + strncat((destination), (source), (maxLength) - 1 - strnlen((destination), (maxLength) - 1)); \ > + } while (0) Why are these macros and not inline functions? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:70 > +static void launchChangeRootHelper(int socketPair[]) I think it'd be more clear for this function to take two parameters instead of a pointer. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:73 > + // a file by mistake. However, CAP_SYS_RESSOURCE capability should be dropped Spelling error: CAP_SYS_RESOURCE. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:76 > + if (setrlimit(RLIMIT_NOFILE, &restrictedResource)) { Please explicitly check for -1 here instead of just any nonzero value. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:77 > + fprintf(stderr, "Helper couldn't set the resourcelimit: %s.\n", strerror(errno)); resourcelimit -> resource limit. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:81 > + if (close(socketPair[1])) { Please explicitly check for -1 here instead of just any nonzero value. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:82 > + fprintf(stderr, "Couldn't close socket %d: %s\n", socketPair[1], strerror(errno)); Missing period before the newline in the format string. Maybe "Failed to close socket" is a better message? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:86 > + // We expect a 'C' (ChrootMe) message from the WebProcess. I think this comment is better suited for the if statement below this one. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:89 > + fprintf(stderr, "Couldn't read the proper chrootme msg: %s\n", strerror(errno)); I suggest making this error message more generic, something like "Failed to read message from the web process." > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:94 > + fprintf(stderr, "Wrong message recieved.\n"); Maybe print out the message as a hex character here? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:99 > + if (lstat(sandboxDirectory, &sandboxDirectoryInfo) && S_ISDIR(sandboxDirectoryInfo.st_mode)) { I think you want to check whether lstat returns -1 here. Also, if lstat fails, then i don't think you can count on sandboxDirectoryInfo.st_mode being initialized. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:104 > + if (chroot(sandboxDirectory)) { You should check for a -1 return value here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:112 > + if (chdir("/")) { Please explicitly check for -1 here instead of just any nonzero value. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:126 > +static bool setEnvironmentVariablesForChangeRootHelper(pid_t pid, int socketPair[]) Same comment about the socketPair array. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:130 > + char sandboxHelperPid[descriptorSize]; The correct WebKit style guidelines name for this would be sandboxHelperPID. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:134 > + fprintf(stderr, "Converting the pid to string is failed: %s\n", strerror(errno)); snprintf doesn't sett errno. Maybe just print out "Failed to convert the sandbox helper PID to a string.\n". > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:138 > + if (setenv(SANDBOX_HELPER_PID, sandboxHelperPid, 1)) { Please check for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:139 > + fprintf(stderr, "Couldn't set the SBX_HELPER_PID env variable: %s\n", strerror(errno)); env -> environment. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:145 > + fprintf(stderr, "Converting the file descriptor to string is failed: %s.\n", strerror(errno)); Same comment about snprintf not setting errno. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:149 > + if (setenv(SANDBOX_DESCRIPTOR, socketDescriptor, 1)) { Please check for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:150 > + fprintf(stderr, "Saving the helpers filedescriptor into an env variable failed: %s\n", strerror(errno)); "Failed to store the helper's file descriptor into an environment variable: %s.\n" > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:154 > + if (close(socketPair[0])) { Please check for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:155 > + fprintf(stderr, "Closing of %d failed: %s\n", socketPair[0], strerror(errno)); Missing period. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:164 > + pid_t pid; You can just declare and initialize pid on the same row. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:166 > + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socketPair)) { Please check for -1 here. Also, do you really need to create a socket pair? Wouldn't pipe be OK here? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:171 > + pid = syscall(SYS_clone, CLONE_FS | SIGCHLD, 0, 0, 0); Why are you calling syscall here instead of clone directly? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:173 > + switch (pid) { I think just using two if statements instead of this switch statement would be clearer. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:182 > + // We shouldn't reach this part, because launchChrootHelper() should exit in every cases. I don't think this comment is correct. launchChrootHelper only exists in the success case. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:191 > + // We should never reach here. Missing newline. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:199 > +static bool setCapabilities(cap_value_t* capabilityList, int length) I think this should take a Vector<cap_value_t> instead of a pointer + length. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:201 > + cap_t capabilities; Please put the declaration on the same line as the initialization. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:206 > + fprintf(stderr, "Process capabilities init failed: %s\n", strerror(errno)); "Failed to initialize process capabilities" > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:210 > + if (cap_clear(capabilities)) { Please check for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:218 > + if (cap_set_flag(capabilities, CAP_EFFECTIVE, length, capabilityList, CAP_SET) > + || cap_set_flag(capabilities, CAP_INHERITABLE, length, capabilityList, CAP_SET) > + || cap_set_flag(capabilities, CAP_PERMITTED, length, capabilityList, CAP_SET)) { Please check for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:219 > + fprintf(stderr, "Cannot set process capability flags: %s\n", strerror(errno)); "Failed to ..." > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:226 > + fprintf(stderr, "Cannot set process capabilities: %s\n", strerror(errno)); "Failed to ..." > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:231 > + if (cap_free(capabilities) == -1) { I don't think you need to check if cap_free succeeds here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:232 > + fprintf(stderr, "Liberating capabilities failed: %s\n", strerror(errno)); "Failed to ..." > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:242 > + if (prctl(PR_SET_DUMPABLE, 0, 0, 0, 0)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:243 > + fprintf(stderr, "Setting dumplable is failed: %s\n", strerror(errno)); Spelling error. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:248 > + if (setresgid(sandboxUserGid, sandboxUserGid, sandboxUserGid) > + || setresuid(sandboxUserUid, sandboxUserUid, sandboxUserUid)) { These should be two separate calls so you can better pinpoint which call failed in case of failure. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:252 > + // Drop all capabilities. Again, setuid() normally takes care of this if we had euid 0. Please add an extra newline before the comment. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:259 > + struct stat fileStat; > + return !(lstat(path, &fileStat) && errno == ENOENT); Please rewrite this as two if statements to make it more clear. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:265 > + if (lstat(directory, &fileStat)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:278 > + strlcpy(pathToCreateInSandbox, sandboxDirectory, PathSize); > + strlcat(pathToCreateInSandbox, pathToCreate, PathSize); It looks like this strlcpy/strlcat pattern is common, so I suggest you factor it out into a "appendDirectoryComponent" function that takes a path and a name and returns them concatenated. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:281 > + if (mkdir(pathToCreateInSandbox, mode)) { Please check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:289 > + if (lstat(pathToCreate, &fileInfo)) { Please check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:296 > + if (fileInfo.st_uid == getuid()) { > + if (chown(pathToCreateInSandbox, sandboxUserUid, sandboxUserGid)) > + return false; > + } This could use some error reporting/explanation. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:298 > + if (chmod(pathToCreateInSandbox, fileInfo.st_mode)) > + return false; Ditto. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:302 > +static bool createDirectoryPath(const char* path) I don't think this name fully clarifies what the function does. Maybe something to indicate that it creates all intermediate directories as well. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:306 > + char fullPathInSandbox[PathSize]; > + strlcpy(fullPathInSandbox, sandboxDirectory, PathSize); > + strlcat(fullPathInSandbox, path, PathSize); This is why I think you should have gone with C++ style strings... > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:318 > + strlcpy(nextDirectoryToCreate, startPos - 1, strnlen(startPos - 1, endPos - startPos + 1) + 1); Again, C++ style strings would have made this more clear. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:324 > + // Create the last directory of the directorypath. missing space between directory and path. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:331 > + const char* dev = "/dev/"; I don't think you need the trailing slash here. I think devDirectory or devPath would be a better variable name. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:336 > + for (int i = 0; i < 2; ++i) { Might want to use the num-elements-in-array trick here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:339 > + fprintf(stderr, "Error by obtaining information about device file (%s): %s\n", devices[i], strerror(errno)); "Failed to stat device file" > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:345 > + // their permissions should be: rw-rw-rw-. No need for this comment to be on a separate line. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:350 > + if (mknod(device, S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH, makedev(major(dev), minor(dev)))) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:362 > + const char* proc = "/proc/"; Trailing slash. procPath. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:369 > + if (mount(proc, procPathInSandbox, "proc", 0, 0)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:376 > + const char* sharedMemory = "/run/shm/"; Trailing slash. sharedMemoryPath. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:378 > + if (!createDirectoryPath(sharedMemory)) > + return false; Don't you want to print out an error message in this case? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:383 > + if (mount(sharedMemory, sharedMemoryPathInSandbox, "tmpfs", 0, 0)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:385 > + fprintf(stderr, "Error by mounting %s: %s\n", sharedMemory, strerror(errno)); "Failed to mount '%s': %s.\n" > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:392 > +static bool linkFile(char* sourceFile, char* targetFile) These should be const char*. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:395 > + bool isSymlink = true; > + while (isSymlink) { It looks like you can just do while (true) here since isSymlink is only set once inside the loop and then you immediately break out of it. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:397 > + if (lstat(sourceFile, &fileInfo)) { -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:406 > + char* endOfBaseDirectoryInSource = strrchr(sourceFile, '/'); const char. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:413 > + strlcpy(baseDirectoryOfSource, sourceFile, endOfBaseDirectoryInSource - sourceFile + 2); Why +2? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:420 > + if (link(sourceFile, targetFile)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:431 > + isSymlink = (fileInfo.st_mode & S_IFMT) == S_IFLNK; > + if (!isSymlink) > + break; Again, you can just break directly here if this is false - no need to assign it. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:438 > + char symlinkTargetInRealWorld[PathSize]; What does real world mean here? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:440 > + // Making difference between relative and absolute paths. Please add an extra newline before the comment. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:458 > + strlcat(symlinkTargetInRealWorld, symlinkTarget, PathSize); > + > + strlcat(symlinkTargetInSandbox, sandboxDirectory, PathSize); > + strlcat(symlinkTargetInSandbox, symlinkTarget, PathSize); > + } else { > + strlcat(symlinkTargetInRealWorld, baseDirectoryOfSource, PathSize); > + strlcat(symlinkTargetInRealWorld, "/", PathSize); > + strlcat(symlinkTargetInRealWorld, symlinkTarget, PathSize); > + > + strlcat(symlinkTargetInSandbox, sandboxDirectory, PathSize); > + strlcat(symlinkTargetInSandbox, "/", PathSize); > + strlcat(symlinkTargetInSandbox, symlinkTargetInRealWorld, PathSize); Again, having a helper function for concatenating paths together would simplify this a lot. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:465 > + sourceFile[0] = '\0'; > + targetFile[0] = '\0'; > + strlcat(sourceFile, symlinkTargetInRealWorld, PathSize); > + strlcat(targetFile, symlinkTargetInSandbox, PathSize); I see now why sourceFile and targetFile are not const pointers. I suggest that you don't use the parameters like this but instead use your own variables inside the function. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:470 > +static bool linkDirectory(const char* sourceDirectoryPath, const char* targetDirectoryPath) I think this function could use a comment indicating what it does or a better name. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:481 > + struct dirent *directoryInfo = 0; > + while ((directoryInfo = readdir(directory))) { You can put the declaration iside the while statement here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:482 > + char* nextFileInDirectory = directoryInfo->d_name; I don't think "next" adds anything here. Maybe just fileName? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:485 > + if (!strncmp(nextFileInDirectory, ".", strnlen(nextFileInDirectory, PathSize)) || !strncmp(nextFileInDirectory, "..", 2)) > + continue; It'd be more clear to just call strcmp directly here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:489 > + strlcpy(sourceFile, sourceDirectoryPath, PathSize); > + strlcat(sourceFile, "/", PathSize); > + strlcat(sourceFile, nextFileInDirectory, PathSize); Same comment about adding a helper function. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:496 > + bool ok = true; Give this a more descriptive name, such as "returnValue". No need to initialize it. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:497 > + if ((directoryInfo->d_type == DT_DIR)) No need for the extra parentheses. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:506 > + // it could have meaning e.g. in the hashgeneration of cache files. hash generation. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:508 > + if (lstat(sourceDirectoryPath, &fileStat)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:512 > + struct utimbuf times = { fileStat.st_atime, fileStat.st_mtime }; Please don't use aggregate initialization for this. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:513 > + if (utime(targetDirectoryPath, ×)) { Check for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:514 > + fprintf(stderr, "Couldn't set back the last modification time of %s: %s\n", targetDirectoryPath, strerror(errno)); It's always good to quote paths, so '%s'. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:532 > + while (*currentRuntimeDependency) { Instead of null terminating this I think you should use a for-loop and count up to the number of elements in the array. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:543 > + if (dlinfo(handle, RTLD_DI_LINKMAP, &linkMap)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:552 > + char pathOfTheLibrary[PathSize]; I don't think this variable is needed, you can just access linkMap->l_name directly. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:557 > + strlcpy(pathOfTheLibraryInSandbox, sandboxDirectory, PathSize); > + strlcat(pathOfTheLibraryInSandbox, pathOfTheLibrary, PathSize); Same comment about a helper function. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:560 > + if (dlclose(handle)) Check for -1 (or just remove the error handling altogether). > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:564 > + if (dlclose(handle)) { Check for -1 (or just remove the error handling altogether). > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:578 > + char buffer[BUFSIZ]; What's BUFSIZ here? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:589 > + strlcpy(xauthorityOfRealUser, realUser->pw_dir, PathSize); > + strlcat(xauthorityOfRealUser, "/.Xauthority", PathSize); Same comment about a helper function. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:592 > + strlcpy(xauthorityInSandbox, sandboxDirectory, PathSize); > + strlcat(xauthorityInSandbox, xauthorityOfRealUser, PathSize); Same comment about a helper function. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:606 > + // We copy the .Xauthority file of the real user (instead of linking) because nobody user the 'nobody' user. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:609 > + while ((size = fread(buffer, 1, BUFSIZ, source))) > + fwrite(buffer, 1, size, dest); Don't you want to check that the write was successful here? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:621 > + if (chown(xauthorityInSandbox, sandboxUserUid, sandboxUserGid)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:626 > + if (setenv("XAUTHORITY", xauthorityInSandbox, 1)) { Check for -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:633 > +static bool initSandbox() Maybe initializeSandbox? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:636 > + // Create the sandbox directory. We only need to step into it, so > + // the executable permission is needed only. "step into it" is oddly worded. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:637 > + if (mkdir(sandboxDirectory, S_IFDIR | S_IXUSR | S_IXOTH)) { -1. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:656 > + char localDirectory[PathSize]; > + strlcpy(localDirectory, home, PathSize); > + strlcat(localDirectory, "/.local/share/Nokia/", PathSize); Helper function! > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:660 > + char cacheDirectory[PathSize]; > + strlcpy(cacheDirectory, home, PathSize); > + strlcat(cacheDirectory, "/.cache/Nokia/", PathSize); Helper function!! > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:664 > + char fontDirectory[PathSize]; > + strlcpy(fontDirectory, home, PathSize); > + strlcat(fontDirectory, "/.fontconfig/", PathSize); Helper function!!! > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:678 > + const char** currentLinkedDirectory = linkedDirectories; > + while (*currentLinkedDirectory) { For loop instead of null terminating. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:681 > + strlcpy(linkedDirectoryInSandbox, sandboxDirectory, PathSize); > + strlcat(linkedDirectoryInSandbox, *currentLinkedDirectory, PathSize); Helper function!!!! > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:697 > + cap_value_t capabiltyList[4]; capabiltyList is misspelled. Please do use aggregate initialization here and use [] instead of a fixed size. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:714 > +static bool moveToNewPidNamespace() moveToNewPIDNamespace. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:717 > + // We can't share FS accross namespaces. FS -> filesystems. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:718 > + pid_t pid, expectedPid; expectedPid-> expectedPID. (And both variables should be declared where they are used). > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:720 > + pid = syscall(SYS_clone, CLONE_NEWPID | SIGCHLD, 0, 0, 0); Again, you can initialize pid directly and why is syscall used instead of just calling clone directly? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:722 > + switch (pid) { Again, two if statements would be clearer. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:738 > + fprintf(stderr, "Waitpid is failed with: %s\n", strerror(errno)); It's not waitpid that fails here, you should indicate that in your error message. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:749 > +static bool run(int argc, char *const argv[]) Why does this need to be a separate function? Why can't this code just live in main? > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:758 > + strlcpy(sandboxDirectory, userInfo->pw_dir, PathSize); > + strlcat(sandboxDirectory, "/.wk2-sandbox", PathSize); Helper function! > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:760 > + // Currently we use nobody user as the sandbox user and fallback to the real user The 'nobody' user. fallback -> fall back. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:761 > + // if it's failed. (We could extend this in the future with a specific restricted user.) if it's failed -> if we failed do get it? I think the period should go after the ). > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:763 > + struct passwd* nobodyUser = getpwnam("nobody"); > + if (nobodyUser) { Variable declaration can go inside the if statement. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:818 > + if (execl(argv[1], argv[1], argv[2], reinterpret_cast<char*>(0)) == -1) { Check for -1. Oh wait! :) > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:175 > + QProcess* webProcessOrSUIDHelper; > + webProcessOrSUIDHelper = new QtWebProcess(); Can just initialize the variable on the same line. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:159 > + long int sandboxSocketDescriptor = -1; > + char* sandboxSocketDescriptorString; > + char* helperPid; > + char sandboxMeMessage = MSG_CHROOTME; > + ssize_t numberOfCharacters; > + pid_t helper = -1; Please move these to where they're actually initialized. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:166 > + errno = 0; No need to set errno here. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:167 > + sandboxSocketDescriptor = strtol(sandboxSocketDescriptorString, (char **) 0, 10); Extra space before the ** and the 0. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:168 > + if (errno || (sandboxSocketDescriptor == -1)) Actually, it'd be better to pass a second parameter to strtol and check that it's \0 instead of checking the return value like this. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:172 > + helperPid = getenv(SANDBOX_HELPER_PID); This variable should be called helperPID, or maybe sandboxHelperPID. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:178 > + errno = 0; > + helper = strtol(helperPid, (char **) 0, 10); > + if (errno || (helper == -1)) Same comment here about passing a parameter to strtol. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:181 > + // Send the chrootMe message to the helper. Newline before the comment. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:187 > + // Read the acknowledgement message from the helper. Newline before the comment. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:189 > + if ((numberOfCharacters != 1) || (sandboxMeMessage != MSG_CHROOTED)) { No need for the extra parentheses. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:194 > + // Wait for the helper process. Newline before the comment. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:195 > + int expectedPid = waitpid(helper, 0, 0); expectedPID. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:196 > + if (expectedPid != -1 && ((helper == -1) || (expectedPid == helper))) No need for the extra parentheses. > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:207 > + pid_t helper; > + helper = chrootMe(); Put the initialization and declaration on the same line. > Tools/ChangeLog:6 > + Reviewed by Zoltan Herczeg. And me!
Comment on attachment 174722 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=174722&action=review >> ChangeLog:6 >> + Reviewed by Zoltan Herczeg. > > And me! Ofc! Sorry! This line remained from the previous, almost-committed version :) >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22 >> +#include "SandboxEnvironmentLinux.h" > > Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Should I create a dummy config.h? Because if I use the config.h of WebKit2 then it'd require further dependencies (e.g. Qt libraries) and the base concept is to keep this code as small as possible. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:52 >> +static char sandboxDirectory[PathSize]; > > Doesn't Linux have a MAX_PATH #define? limits.h contains the PATH_MAX constant, but from the linux manual: "According to POSIX.1-2001 a buffer of size PATH_MAX suffices, but PATH_MAX need not be a defined constant, and may have to be obtained using pathconf(3)." Because of this many functions based on PATH_MAX constant is pronounced as insecure, deprecated or broken (e.g.: getcwd(), realpath(), etc.). I guess we neither should have to trust in it. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:54 >> +static uid_t sandboxUserGid; > > Please make UID and GID all caps. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:65 >> + } while (0) > > Why are these macros and not inline functions? Right. Updated. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:70 >> +static void launchChangeRootHelper(int socketPair[]) > > I think it'd be more clear for this function to take two parameters instead of a pointer. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:73 >> + // a file by mistake. However, CAP_SYS_RESSOURCE capability should be dropped > > Spelling error: CAP_SYS_RESOURCE. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:76 >> + if (setrlimit(RLIMIT_NOFILE, &restrictedResource)) { > > Please explicitly check for -1 here instead of just any nonzero value. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:77 >> + fprintf(stderr, "Helper couldn't set the resourcelimit: %s.\n", strerror(errno)); > > resourcelimit -> resource limit. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:81 >> + if (close(socketPair[1])) { > > Please explicitly check for -1 here instead of just any nonzero value. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:82 >> + fprintf(stderr, "Couldn't close socket %d: %s\n", socketPair[1], strerror(errno)); > > Missing period before the newline in the format string. Maybe "Failed to close socket" is a better message? Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:86 >> + // We expect a 'C' (ChrootMe) message from the WebProcess. > > I think this comment is better suited for the if statement below this one. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:89 >> + fprintf(stderr, "Couldn't read the proper chrootme msg: %s\n", strerror(errno)); > > I suggest making this error message more generic, something like "Failed to read message from the web process." Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:94 >> + fprintf(stderr, "Wrong message recieved.\n"); > > Maybe print out the message as a hex character here? Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:99 >> + if (lstat(sandboxDirectory, &sandboxDirectoryInfo) && S_ISDIR(sandboxDirectoryInfo.st_mode)) { > > I think you want to check whether lstat returns -1 here. Also, if lstat fails, then i don't think you can count on sandboxDirectoryInfo.st_mode being initialized. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:104 >> + if (chroot(sandboxDirectory)) { > > You should check for a -1 return value here. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:112 >> + if (chdir("/")) { > > Please explicitly check for -1 here instead of just any nonzero value. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:126 >> +static bool setEnvironmentVariablesForChangeRootHelper(pid_t pid, int socketPair[]) > > Same comment about the socketPair array. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:130 >> + char sandboxHelperPid[descriptorSize]; > > The correct WebKit style guidelines name for this would be sandboxHelperPID. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:134 >> + fprintf(stderr, "Converting the pid to string is failed: %s\n", strerror(errno)); > > snprintf doesn't sett errno. Maybe just print out "Failed to convert the sandbox helper PID to a string.\n". Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:138 >> + if (setenv(SANDBOX_HELPER_PID, sandboxHelperPid, 1)) { > > Please check for -1 here. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:139 >> + fprintf(stderr, "Couldn't set the SBX_HELPER_PID env variable: %s\n", strerror(errno)); > > env -> environment. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:145 >> + fprintf(stderr, "Converting the file descriptor to string is failed: %s.\n", strerror(errno)); > > Same comment about snprintf not setting errno. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:149 >> + if (setenv(SANDBOX_DESCRIPTOR, socketDescriptor, 1)) { > > Please check for -1 here. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:150 >> + fprintf(stderr, "Saving the helpers filedescriptor into an env variable failed: %s\n", strerror(errno)); > > "Failed to store the helper's file descriptor into an environment variable: %s.\n" Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:154 >> + if (close(socketPair[0])) { > > Please check for -1 here. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:155 >> + fprintf(stderr, "Closing of %d failed: %s\n", socketPair[0], strerror(errno)); > > Missing period. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:164 >> + pid_t pid; > > You can just declare and initialize pid on the same row. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:166 >> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socketPair)) { > > Please check for -1 here. Also, do you really need to create a socket pair? Wouldn't pipe be OK here? The communication between the SandboxProcess and the WebProcess (what is bidirectional) goes through this socket pair. This way pipe, what is unidirectional, is not suitable here (IMHO). >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:171 >> + pid = syscall(SYS_clone, CLONE_FS | SIGCHLD, 0, 0, 0); > > Why are you calling syscall here instead of clone directly? I do not use clone() with syscall here, but I call sys_clone() system call. They are different: Usually clone(int (*child_main)(void *), void *child_stack, int flags, void *main_arg, ...) is used to run an approriate function in a separate thread. sys_clone(void *child_stack, int flags) corresponds more to fork(), the new task continues its execution by returning from clone() so the child_main function and its argument are not needed. And our goal with this cloning is to run our current process with additional abilities. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:173 >> + switch (pid) { > > I think just using two if statements instead of this switch statement would be clearer. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:182 >> + // We shouldn't reach this part, because launchChrootHelper() should exit in every cases. > > I don't think this comment is correct. launchChrootHelper only exists in the success case. Indeed. Comment is updated to "// We reach this part only if launchChrootHelper() failed, instead it should have exited." >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:191 >> + // We should never reach here. > > Missing newline. This part is removed when switch statement was changed to if statement. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:201 >> + cap_t capabilities; > > Please put the declaration on the same line as the initialization. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:206 >> + fprintf(stderr, "Process capabilities init failed: %s\n", strerror(errno)); > > "Failed to initialize process capabilities" Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:210 >> + if (cap_clear(capabilities)) { > > Please check for -1 here. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:218 >> + || cap_set_flag(capabilities, CAP_PERMITTED, length, capabilityList, CAP_SET)) { > > Please check for -1 here. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:219 >> + fprintf(stderr, "Cannot set process capability flags: %s\n", strerror(errno)); > > "Failed to ..." Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:226 >> + fprintf(stderr, "Cannot set process capabilities: %s\n", strerror(errno)); > > "Failed to ..." Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:231 >> + if (cap_free(capabilities) == -1) { > > I don't think you need to check if cap_free succeeds here. Ok, removed. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:232 >> + fprintf(stderr, "Liberating capabilities failed: %s\n", strerror(errno)); > > "Failed to ..." Removed too. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:242 >> + if (prctl(PR_SET_DUMPABLE, 0, 0, 0, 0)) { > > Check for -1. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:243 >> + fprintf(stderr, "Setting dumplable is failed: %s\n", strerror(errno)); > > Spelling error. Fixed. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:248 >> + || setresuid(sandboxUserUid, sandboxUserUid, sandboxUserUid)) { > > These should be two separate calls so you can better pinpoint which call failed in case of failure. Right. Separated. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:252 >> + // Drop all capabilities. Again, setuid() normally takes care of this if we had euid 0. > > Please add an extra newline before the comment. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:259 >> + return !(lstat(path, &fileStat) && errno == ENOENT); > > Please rewrite this as two if statements to make it more clear. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:265 >> + if (lstat(directory, &fileStat)) { > > Check for -1. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:278 >> + strlcat(pathToCreateInSandbox, pathToCreate, PathSize); > > It looks like this strlcpy/strlcat pattern is common, so I suggest you factor it out into a "appendDirectoryComponent" function that takes a path and a name and returns them concatenated. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:281 >> + if (mkdir(pathToCreateInSandbox, mode)) { > > Please check for -1. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:289 >> + if (lstat(pathToCreate, &fileInfo)) { > > Please check for -1. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:296 >> + } > > This could use some error reporting/explanation. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:298 >> + return false; > > Ditto. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:302 >> +static bool createDirectoryPath(const char* path) > > I don't think this name fully clarifies what the function does. Maybe something to indicate that it creates all intermediate directories as well. What about createDirecoryChain ? >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:324 >> + // Create the last directory of the directorypath. > > missing space between directory and path. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:331 >> + const char* dev = "/dev/"; > > I don't think you need the trailing slash here. I think devDirectory or devPath would be a better variable name. Indeed. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:339 >> + fprintf(stderr, "Error by obtaining information about device file (%s): %s\n", devices[i], strerror(errno)); > > "Failed to stat device file" Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:345 >> + // their permissions should be: rw-rw-rw-. > > No need for this comment to be on a separate line. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:350 >> + if (mknod(device, S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH, makedev(major(dev), minor(dev)))) { > > Check for -1. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:362 >> + const char* proc = "/proc/"; > > Trailing slash. procPath. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:369 >> + if (mount(proc, procPathInSandbox, "proc", 0, 0)) { > > Check for -1. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:376 >> + const char* sharedMemory = "/run/shm/"; > > Trailing slash. sharedMemoryPath. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:378 >> + return false; > > Don't you want to print out an error message in this case? I do :) Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:383 >> + if (mount(sharedMemory, sharedMemoryPathInSandbox, "tmpfs", 0, 0)) { > > Check for -1. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:385 >> + fprintf(stderr, "Error by mounting %s: %s\n", sharedMemory, strerror(errno)); > > "Failed to mount '%s': %s.\n" Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:392 >> +static bool linkFile(char* sourceFile, char* targetFile) > > These should be const char*. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:395 >> + while (isSymlink) { > > It looks like you can just do while (true) here since isSymlink is only set once inside the loop and then you immediately break out of it. Right. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:397 >> + if (lstat(sourceFile, &fileInfo)) { > > -1. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:406 >> + char* endOfBaseDirectoryInSource = strrchr(sourceFile, '/'); > > const char. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:413 >> + strlcpy(baseDirectoryOfSource, sourceFile, endOfBaseDirectoryInSource - sourceFile + 2); > > Why +2? +1 is for the trailing slash (we need is because file names will be concatenated later). The second +1 is because of the implementation of strlcpy: strlcpy(dest, src, maxLength) copies (maxLength - 1) characters from the src to dst to make sure not to overflow. So if we add the length of the string - without the '\0' - the last character will be cutted. In all other cases maxLength is the PathSize constant and don't require such trick. Maybe I should put a comment here to explain what happens. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:420 >> + if (link(sourceFile, targetFile)) { > > Check for -1. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:431 >> + break; > > Again, you can just break directly here if this is false - no need to assign it. Right. Fixed. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:438 >> + char symlinkTargetInRealWorld[PathSize]; > > What does real world mean here? It's the users file system, that's not the sandbox environment. Should I change it? Maybe to symlinkTarget or originalSymlinkTarget? >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:440 >> + // Making difference between relative and absolute paths. > > Please add an extra newline before the comment. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:458 >> + strlcat(symlinkTargetInSandbox, symlinkTargetInRealWorld, PathSize); > > Again, having a helper function for concatenating paths together would simplify this a lot. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:465 >> + strlcat(targetFile, symlinkTargetInSandbox, PathSize); > > I see now why sourceFile and targetFile are not const pointers. I suggest that you don't use the parameters like this but instead use your own variables inside the function. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:470 >> +static bool linkDirectory(const char* sourceDirectoryPath, const char* targetDirectoryPath) > > I think this function could use a comment indicating what it does or a better name. Comment is added. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:481 >> + while ((directoryInfo = readdir(directory))) { > > You can put the declaration iside the while statement here. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:482 >> + char* nextFileInDirectory = directoryInfo->d_name; > > I don't think "next" adds anything here. Maybe just fileName? fileName is fine. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:485 >> + continue; > > It'd be more clear to just call strcmp directly here. ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:489 >> + strlcat(sourceFile, nextFileInDirectory, PathSize); > > Same comment about adding a helper function. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:496 >> + bool ok = true; > > Give this a more descriptive name, such as "returnValue". No need to initialize it. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:497 >> + if ((directoryInfo->d_type == DT_DIR)) > > No need for the extra parentheses. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:506 >> + // it could have meaning e.g. in the hashgeneration of cache files. > > hash generation. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:508 >> + if (lstat(sourceDirectoryPath, &fileStat)) { > > Check for -1. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:512 >> + struct utimbuf times = { fileStat.st_atime, fileStat.st_mtime }; > > Please don't use aggregate initialization for this. Ok. Separated. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:513 >> + if (utime(targetDirectoryPath, ×)) { > > Check for -1 here. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:514 >> + fprintf(stderr, "Couldn't set back the last modification time of %s: %s\n", targetDirectoryPath, strerror(errno)); > > It's always good to quote paths, so '%s'. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:532 >> + while (*currentRuntimeDependency) { > > Instead of null terminating this I think you should use a for-loop and count up to the number of elements in the array. What about a for loop from 0 to (sizeof(runtimeDependencies) / sizeof(runtimeDependencies[0])) ? >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:543 >> + if (dlinfo(handle, RTLD_DI_LINKMAP, &linkMap)) { > > Check for -1. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:552 >> + char pathOfTheLibrary[PathSize]; > > I don't think this variable is needed, you can just access linkMap->l_name directly. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:557 >> + strlcat(pathOfTheLibraryInSandbox, pathOfTheLibrary, PathSize); > > Same comment about a helper function. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:560 >> + if (dlclose(handle)) > > Check for -1 (or just remove the error handling altogether). Ok, removed. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:564 >> + if (dlclose(handle)) { > > Check for -1 (or just remove the error handling altogether). Removed too. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:578 >> + char buffer[BUFSIZ]; > > What's BUFSIZ here? It's a constant from stdio.h. It contains the optimal size of buffers to make I/O operations more effective. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:589 >> + strlcat(xauthorityOfRealUser, "/.Xauthority", PathSize); > > Same comment about a helper function. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:592 >> + strlcat(xauthorityInSandbox, xauthorityOfRealUser, PathSize); > > Same comment about a helper function. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:606 >> + // We copy the .Xauthority file of the real user (instead of linking) because nobody user > > the 'nobody' user. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:609 >> + fwrite(buffer, 1, size, dest); > > Don't you want to check that the write was successful here? I do. :) Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:621 >> + if (chown(xauthorityInSandbox, sandboxUserUid, sandboxUserGid)) { > > Check for -1. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:626 >> + if (setenv("XAUTHORITY", xauthorityInSandbox, 1)) { > > Check for -1. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:633 >> +static bool initSandbox() > > Maybe initializeSandbox? OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:636 >> + // the executable permission is needed only. > > "step into it" is oddly worded. "We only need to enter it, ..." ? >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:637 >> + if (mkdir(sandboxDirectory, S_IFDIR | S_IXUSR | S_IXOTH)) { > > -1. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:656 >> + strlcat(localDirectory, "/.local/share/Nokia/", PathSize); > > Helper function! OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:660 >> + strlcat(cacheDirectory, "/.cache/Nokia/", PathSize); > > Helper function!! OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:664 >> + strlcat(fontDirectory, "/.fontconfig/", PathSize); > > Helper function!!! OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:678 >> + while (*currentLinkedDirectory) { > > For loop instead of null terminating. Ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:697 >> + cap_value_t capabiltyList[4]; > > capabiltyList is misspelled. Please do use aggregate initialization here and use [] instead of a fixed size. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:714 >> +static bool moveToNewPidNamespace() > > moveToNewPIDNamespace. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:717 >> + // We can't share FS accross namespaces. > > FS -> filesystems. ok. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:718 >> + pid_t pid, expectedPid; > > expectedPid-> expectedPID. (And both variables should be declared where they are used). OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:720 >> + pid = syscall(SYS_clone, CLONE_NEWPID | SIGCHLD, 0, 0, 0); > > Again, you can initialize pid directly and why is syscall used instead of just calling clone directly? Answer is above. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:722 >> + switch (pid) { > > Again, two if statements would be clearer. Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:738 >> + fprintf(stderr, "Waitpid is failed with: %s\n", strerror(errno)); > > It's not waitpid that fails here, you should indicate that in your error message. Right. Updated. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:749 >> +static bool run(int argc, char *const argv[]) > > Why does this need to be a separate function? Why can't this code just live in main? I've followed the template of PluginProcess main function (Qt, Gtk, etc.). Both of them have just a minimal main() function. Furhermore, this way it's easy to modify the return value of SandboxProcess. I mean if we have an error then run() returns with false and in main() we can simply change the error code from 1 to anything. But I can put this code to main ofc. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:758 >> + strlcat(sandboxDirectory, "/.wk2-sandbox", PathSize); > > Helper function! Done. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:760 >> + // Currently we use nobody user as the sandbox user and fallback to the real user > > The 'nobody' user. fallback -> fall back. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:761 >> + // if it's failed. (We could extend this in the future with a specific restricted user.) > > if it's failed -> if we failed do get it? I think the period should go after the ). OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:763 >> + if (nobodyUser) { > > Variable declaration can go inside the if statement. OK. >> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:818 >> + if (execl(argv[1], argv[1], argv[2], reinterpret_cast<char*>(0)) == -1) { > > Check for -1. Oh wait! :) :) >> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:175 >> + webProcessOrSUIDHelper = new QtWebProcess(); > > Can just initialize the variable on the same line. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:159 >> + pid_t helper = -1; > > Please move these to where they're actually initialized. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:166 >> + errno = 0; > > No need to set errno here. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:167 >> + sandboxSocketDescriptor = strtol(sandboxSocketDescriptorString, (char **) 0, 10); > > Extra space before the ** and the 0. ok. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:168 >> + if (errno || (sandboxSocketDescriptor == -1)) > > Actually, it'd be better to pass a second parameter to strtol and check that it's \0 instead of checking the return value like this. Ok. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:172 >> + helperPid = getenv(SANDBOX_HELPER_PID); > > This variable should be called helperPID, or maybe sandboxHelperPID. Ok. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:178 >> + if (errno || (helper == -1)) > > Same comment here about passing a parameter to strtol. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:181 >> + // Send the chrootMe message to the helper. > > Newline before the comment. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:187 >> + // Read the acknowledgement message from the helper. > > Newline before the comment. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:189 >> + if ((numberOfCharacters != 1) || (sandboxMeMessage != MSG_CHROOTED)) { > > No need for the extra parentheses. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:194 >> + // Wait for the helper process. > > Newline before the comment. OK. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:195 >> + int expectedPid = waitpid(helper, 0, 0); > > expectedPID. Ok. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:196 >> + if (expectedPid != -1 && ((helper == -1) || (expectedPid == helper))) > > No need for the extra parentheses. Ok. >> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:207 >> + helper = chrootMe(); > > Put the initialization and declaration on the same line. OK. >> Tools/ChangeLog:6 >> + Reviewed by Zoltan Herczeg. > > And me! Added! :)
Created attachment 177000 [details] Proposed patch
Attachment 177000 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
@andersca: And thank you very, very much for the detailed review!
@andersca: What do you think? Are there any further suggestion or objection?
(In reply to comment #141) > @andersca: What do you think? Are there any further suggestion or objection? Sorry for not responding earlier, I haven't forgotten about you! :) I'll try to do another review pass during the weekend.
(In reply to comment #142) > (In reply to comment #141) > > @andersca: What do you think? Are there any further suggestion or objection? > > Sorry for not responding earlier, I haven't forgotten about you! :) I'll try to do another review pass during the weekend. Thanks. I'm waiting :)
It seems the end of the world hasn't happened yet. So we could probably use this little sandbox in webkit. I just need somebody to review it...
Comment on attachment 177000 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=177000&action=review Looks much better, only a couple of minor nits left to fix! ;) > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:52 > +enum { PathSize = 512 }; It's a little weird to use an enum here, can't you just use a const unsigned? Also, globals should still start with a lowercase letter. I think a better name would be maximumPathLength. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:114 > + if (lstat(sandboxDirectory, &sandboxDirectoryInfo) != -1) { > + if (!S_ISDIR(sandboxDirectoryInfo.st_mode)) { > + fprintf(stderr, "%s is not a directory!\n", sandboxDirectory); > + return; > + } > + } else { > + fprintf(stderr, "Sandbox directory (%s) is not available: %s.\n", sandboxDirectory, strerror(errno)); > + return; > + } I think this would look better as checking if lstat returns -1 and returning early, and then checking the ISDIR flag. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:150 > + if (setenv(SANDBOX_HELPER_PID, sandboxHelperPID , 1) == -1) { Extra space before the comma. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:170 > + return true; I think there should be an extra newline before this return statement. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:221 > + if (cap_set_flag(capabilities, CAP_EFFECTIVE, length, capabilityList, CAP_SET) > + || cap_set_flag(capabilities, CAP_INHERITABLE, length, capabilityList, CAP_SET) > + || cap_set_flag(capabilities, CAP_PERMITTED, length, capabilityList, CAP_SET) == -1) { I think you need to check each cap_set_flag value for -1 here. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:313 > +static bool createDirectoryChain(const char* path) I think this could use a comment detailing what it does. > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:655 > + appendDirectoryComponent(localDirectory, home, "/.local/share/Nokia/"); > + appendDirectoryComponent(cacheDirectory, home, "/.cache/Nokia/"); Nokia?
Created attachment 183918 [details] Proposed patch
(In reply to comment #145) > (From update of attachment 177000 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177000&action=review > > Looks much better, only a couple of minor nits left to fix! ;) Thank you for the review :) > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:52 > > +enum { PathSize = 512 }; > > It's a little weird to use an enum here, can't you just use a const unsigned? Also, globals should still start with a lowercase letter. I think a better name would be maximumPathLength. OK. > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:114 > > + if (lstat(sandboxDirectory, &sandboxDirectoryInfo) != -1) { > > + if (!S_ISDIR(sandboxDirectoryInfo.st_mode)) { > > + fprintf(stderr, "%s is not a directory!\n", sandboxDirectory); > > + return; > > + } > > + } else { > > + fprintf(stderr, "Sandbox directory (%s) is not available: %s.\n", sandboxDirectory, strerror(errno)); > > + return; > > + } > > I think this would look better as checking if lstat returns -1 and returning early, and then checking the ISDIR flag. OK. > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:150 > > + if (setenv(SANDBOX_HELPER_PID, sandboxHelperPID , 1) == -1) { > > Extra space before the comma. OK. UIProcess/Launcher/qt/ProcessLauncherQt.cpp > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:170 > > + return true; > > I think there should be an extra newline before this return statement. > > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:221 > > + if (cap_set_flag(capabilities, CAP_EFFECTIVE, length, capabilityList, CAP_SET) > > + || cap_set_flag(capabilities, CAP_INHERITABLE, length, capabilityList, CAP_SET) > > + || cap_set_flag(capabilities, CAP_PERMITTED, length, capabilityList, CAP_SET) == -1) { > > I think you need to check each cap_set_flag value for -1 here. OK. > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:313 > > +static bool createDirectoryChain(const char* path) > > I think this could use a comment detailing what it does. > > > Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:655 > > + appendDirectoryComponent(localDirectory, home, "/.local/share/Nokia/"); > > + appendDirectoryComponent(cacheDirectory, home, "/.cache/Nokia/"); > > Nokia? This is a built-in directory of Qt where WebKit can store its local resources. This path is different on every linux-based ports. We plan to organize them to use a common directory later. But you were right, it's weird right now, so I've removed the last Nokia directory, this way it will work on every linux platform.
Attachment 183918 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1 Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:22: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 183918 [details] Proposed patch Looks good!
Comment on attachment 183918 [details] Proposed patch Clearing flags on attachment: 183918 Committed r140957: <http://trac.webkit.org/changeset/140957>
All reviewed patches have been landed. Closing bug.