On Mittwoch, 17. August 2016 13:37:33 CEST Ander Juaristi wrote: > I was thinking we could rename php extensions to phps, but it's all the > same thing in the end, and even better, since the former applies to any > kind of file and I've seen some broken servers that actually run phps files. > > So, this is what I would do: > > 1. Write temporary files with 600 perms, and make sure they're owned > by the running user and group. qmail goes even further [1] by not > letting root run, but I would not do that here. > 2. Use mkostemp() to generate a unique filename and give it a > harmless extension (like Mozilla's .part). We already have unique_name() > in utils.c, altough it returns the file name untouched if it does not > exist. We should do some research on whether we could reuse parts of it.
Giuseppe and I have a working patch that is basically like this. We are still discussing the details (mkstemp extension, fixed extension, both or maybe a mkdtemp directory where we put all the temp files). As soon as we agree, we'll post the patch here for further discussion/review. > 3. Place them in /tmp, or even better, in ~/.wget-tempfiles, or > something like that. /tmp often is on a separate filesystem (e.g. RAM disk) with limited space. This could open another (DOS) attack vector. You do not always have a home directory when running Wget. > There's a patch by Tim somewhere in this list that already does 1 (but > please, remove the braces ;D). > > It also comes to my mind, instead of writing each temp file to its own > file, we could put them all in the same file (with O_APPEND). But a) we > need a way to tell them apart later, and b) it may cause problems in > NFS, according to open(2). > > [1] http://cr.yp.to/qmail/guarantee.html > > On 15/08/16 18:31, Tim Rühsen wrote: > > On Montag, 15. August 2016 10:02:55 CEST moparisthebest wrote: > >> Hello, > >> > >> I find it extremely hard to call this a wget vulnerability when SO many > >> other things are wrong with that 'vulnerable code' implementation it > >> isn't even funny: > >> > >> 1. The image_importer.php script takes a single argument, why would it > >> download with the recursive switch turned on? Isn't that clearly a bug > >> in the php script? Has a php script like this that downloads all files > >> from a website of a particular extension ever been observed in the wild? > >> > >> 2. A *well* configured server would have a whitelist of .php files it > >> will execute, making it immune to this. A *decently* configured server > >> would always at a minimum make sure they don't execute code in > >> directories with user provided uploads in them. So it's additionally a > >> bug in the server configuration. (incidentally every php package I've > >> downloaded has at minimum a .htaccess in upload directories to prevent > >> this kind of thing with apache) > >> > >> It seems to me like there has always been plenty of ways to shoot > >> yourself in the foot with PHP, and this is just another iteration on a > >> theme. > > > > Hi, > > > > this is absolutely true and your points were the first things that came to > > my mind when reading the original post. > > > > But there is also non-obvious wget behavior in creating those (temp) files > > in the filesystem. And there is also a long history of attack vectors > > introduced by temp files as well. > > > > Today the maintainers discussed a few possible fixes, all with pros and > > cons. I would like to list them here, in case someone likes to comment: > > > > 1. Rewrite code to keep temp files in memory. > > Too complex, needs a redesign of wget. And has been done for wget2... > > > > 2. Add a harmless extension to the file names. > > Possible name collision with wanted files. > > Possible name length issues, have to be worked around. > > > > 3. Using file mode 0 (no flags at all). > > Short vulnerability when changing modes to write/read the data. > > > > 4. Using O_TMPFILE for open(). > > Just for Linux, not for every filesystem available. > > > > 5. Using mkostemp(). > > Possible name collision with wanted files (which would be unexpectedly > > named as *.1 in case of a collision). At least the chance for a collision > > seems very low. > > > > Any thoughts or other ideas ? > > > > Regards, Tim
signature.asc
Description: This is a digitally signed message part.
