4 ms·

Please investigate 'safe' php file upload handling, specifically: the `is_uploaded_file` and `move_uploaded_file` functions. Also perhaps consider the `mkdir`

Please investigate 'safe' php file upload handling, specifically: the `is_uploaded_file` and `move_uploaded_file` functions. Also perhaps consider the `mkdir` function, and perhaps 'file_put_contents'. Your reliance on shell functions via `exec` is quite concerning, given what you're doing. Lastly, I'd suggest either `random_int` or `random_bytes` (combined with e.g. bin2hex, or a hashing function to get an ascii string) instead of uniqid. I'm not quite sure why there's a check on user agent, to carry out the same logic regardless?

eps 7y ago
I think OP's code is actually markedly better than your first two suggestions exactly because it doesn't use any of the advanced PHP APIs. It's all there, bare and easy to check, with no gotchas, ini-dependent caveats and what not. I agree that uniqid(), as used, is not the best choice, but that's only because it may produce dupes if the machine's clock is rolled back (ie by ntpd). Regardless of the prng choice, the code could use a check for whether selected folder already exists.
- stephenr 7y ago
  What "gotchas" or caveats are you expecting from `is_uploaded_file` or `move_uploaded_file`? Any use of `exec` should be considered dangerous, when there's native language/stdlib support for achieving the same thing. Edit: corrected function name.
  - huhtenberg 7y ago
    `is_uploaded_file` and `move_uploaded_file` are the band-aid API meant for broken code that may be mistakenly working with some random local files instead of uploaded ones. This is akin to using "if (3 == x)" notation in C in a fear of accidentally writing "if (x = 3)". For this particular case the use of both `rename` and `exec` would've been perfectly fine. > Any use of `exec` should be considered dangerous By whom? It may be a red flag if used by an inexperienced programmer or if it appears in an otherwise messy code base. But if I know what I'm doing, then it's just another API that's perfectly OK when used with due care.
    - meritt 7y ago
      > `is_uploaded_file` and `move_uploaded_file` are the band-aid It's not a band-aid. When a file is uploaded (see RFC 1867) via HTTP POST, PHP internally allocates a hash table that tracks which files on the filesystem have been uploaded. When the process terminates, it uses that hashtable to remove any remaining uploaded files in the tmp space. move_uploaded_file() confirms that the filename was indeed created as a result of an upload, and additionally removes that entry from the hash table.
      - huhtenberg 7y ago
        > move_uploaded_file() confirms that the filename was indeed created as a result of an upload, and additionally removes that entry from the hash table. In other words, move_uploaded_file() protects against moving a non-uploaded file or moving one more than once. So it's for an upload processing code that, at some point, is becoming unsure of what the heck it is doing. It is a band-aid.
        meritt 7y ago
        Edit: You know what? Fuck you. I'm done trying to help explain why you're incorrect.
        huhtenberg 7y ago
        > You're creating a memory leak A resource leak. That's assuming that you are referring to temp files not getting removed once the POST handler exits... which is not how it works. https://www.php.net/manual/en/features.file-upload.post-method.php https://www.php.net/manual/en/features.file-upload.post-meth... The file will be deleted from the temporary directory at the end of the request if it has not been moved away or renamed. > the internal rfc1867 uploaded files hash table. This RFC does not talk about life cycle management of uploaded files at all, leave alone such specifics as hash tables.
        meritt 7y ago
- deleted 7y ago
  [deleted]
nikosch86 7y ago
good point with the mkdir, mv and touch, I just pushed an update to use php functions for that instead of shell. No reason to be concerned though, the user controlled input is sanitized and no shell command is/was executed based on unsanitized user input. the user agent check was a relic from when it behaved differently, browser vs curl, it is not needed anymore and has been removed. Pull Request are always welcome edit: I also just pushed an update to use bin2hex(random_bytes(16)) instead of uniquid to make sure we never have collisions, for arguments sake (even though I thought that risk was rather low to begin with)

eps 7y ago

I think OP's code is actually markedly better than your first two suggestions exactly because it doesn't use any of the advanced PHP APIs. It's all there, bare and easy to check, with no gotchas, ini-dependent caveats and what not. I agree that uniqid(), as used, is not the best choice, but that's only because it may produce dupes if the machine's clock is rolled back (ie by ntpd). Regardless of the prng choice, the code could use a check for whether selected folder already exists.

stephenr 7y ago
What "gotchas" or caveats are you expecting from `is_uploaded_file` or `move_uploaded_file`? Any use of `exec` should be considered dangerous, when there's native language/stdlib support for achieving the same thing. Edit: corrected function name.
- huhtenberg 7y ago
  `is_uploaded_file` and `move_uploaded_file` are the band-aid API meant for broken code that may be mistakenly working with some random local files instead of uploaded ones. This is akin to using "if (3 == x)" notation in C in a fear of accidentally writing "if (x = 3)". For this particular case the use of both `rename` and `exec` would've been perfectly fine. > Any use of `exec` should be considered dangerous By whom? It may be a red flag if used by an inexperienced programmer or if it appears in an otherwise messy code base. But if I know what I'm doing, then it's just another API that's perfectly OK when used with due care.
  - meritt 7y ago
    > `is_uploaded_file` and `move_uploaded_file` are the band-aid It's not a band-aid. When a file is uploaded (see RFC 1867) via HTTP POST, PHP internally allocates a hash table that tracks which files on the filesystem have been uploaded. When the process terminates, it uses that hashtable to remove any remaining uploaded files in the tmp space. move_uploaded_file() confirms that the filename was indeed created as a result of an upload, and additionally removes that entry from the hash table.
    - huhtenberg 7y ago
      > move_uploaded_file() confirms that the filename was indeed created as a result of an upload, and additionally removes that entry from the hash table. In other words, move_uploaded_file() protects against moving a non-uploaded file or moving one more than once. So it's for an upload processing code that, at some point, is becoming unsure of what the heck it is doing. It is a band-aid.
      - meritt 7y ago
        Edit: You know what? Fuck you. I'm done trying to help explain why you're incorrect.
        huhtenberg 7y ago
        > You're creating a memory leak A resource leak. That's assuming that you are referring to temp files not getting removed once the POST handler exits... which is not how it works. https://www.php.net/manual/en/features.file-upload.post-method.php https://www.php.net/manual/en/features.file-upload.post-meth... The file will be deleted from the temporary directory at the end of the request if it has not been moved away or renamed. > the internal rfc1867 uploaded files hash table. This RFC does not talk about life cycle management of uploaded files at all, leave alone such specifics as hash tables.
        meritt 7y ago
deleted 7y ago
[deleted]

nikosch86 7y ago

good point with the mkdir, mv and touch, I just pushed an update to use php functions for that instead of shell. No reason to be concerned though, the user controlled input is sanitized and no shell command is/was executed based on unsanitized user input. the user agent check was a relic from when it behaved differently, browser vs curl, it is not needed anymore and has been removed. Pull Request are always welcome edit: I also just pushed an update to use bin2hex(random_bytes(16)) instead of uniquid to make sure we never have collisions, for arguments sake (even though I thought that risk was rather low to begin with)