4 ms·

Show HN: Mejiro – single-file photo hosting PHP app

29 points by dmpop9mm 11y ago

dmpop9mm 11y ago
I'm not a coder by any stretch of imagination, but I've managed to cobble together this simple PHP app for hosting my photos. Constructive feedback will be greatly appreciated.
- devwerks 11y ago
  It's great that you had a need and tackled it by writing a script. However, after a brief look at your source, you have a directory traversal vulnerability - there may be other issues too. You are taking a query parameter "d" and appending that to photo_dir which is then used in a variety of places. https://www.owasp.org/index.php/Path_Traversal https://www.owasp.org/index.php/Path_Traversal describes what a directory traversal is. Take a look at https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet or http://www.phptherightway.com/ http://www.phptherightway.com/ for some primers.
  - dmpop9mm 11y ago
    Thank you very much for your feedback and the pointers!
Antwan 11y ago
How can "single dirty file mixing PHP HTML CSS" can be showcased as a feature in 2016...
- sdoering 11y ago
  Does this trolling add anything to the learning experience of the OA? Esp. after he clearly told that he would really like constructive feedback and stated, that he has no real coding experience. Why would you play elite and kick someone lower in the ranks his way? It only shows your true colors. And imho they are not really nice in any way.
- danielhunt 11y ago
  This script is probably step number 5 in the 'how do I start out on this web-programming lark?' journey. There is absolutely nothing wrong with being proud of it, regardless of the language or inevitable vulnerabilities that come from being inexperienced with web development. It's a very small leap to go from this, to understanding separation of concerns. Go easy on newcomers. We're not that elite.
snowpanda 11y ago
I like your coding style, it's very organized.
- madaxe_again 11y ago
  What, JS, PHP, and HTML all in the same procedural script? I get that the idea is that it's single-file, but it doesn't make for pretty (or terribly maintainable) code. It's a neat little script, but I see at least one severe exploit in there ($filename - leave the rest to you).
  - dmpop9mm 11y ago
    Thank you for pointing out the exploit! I'll see what I can do.
NamPNQ 11y ago
Look like pasteboard?
tyingq 11y ago
There's a bit of code to try and stop path traversal, but it doesn't account for passing d=.. as a url param. A shortened test case using the same code as the app to try and thwart traversal: <?php $_GET['d']='..'; $photo_dir='photos/'; $sub_photo_dir = basename($_GET['d']).'/'; $photo_dir = str_replace("//", "/",$photo_dir.$sub_photo_dir); print "photo_dir: [$photo_dir]"; ?> That would allow you to upload a file one directory above the intended $photo_dir.
- dmpop9mm 11y ago
  Good point. I'll look into it. Thanks!
  - tyingq 11y ago
    The typical approach is to use realpath(). That would expand out any relative portions of the path, which you could then compare directly with the expected path and know for sure it's in a subdirectory of your intended photo directory. If you go that way, though, you'll also want to stop using the literal '/' and use the DIRECTORY_SEPARATOR constant so that your script runs on windows. realpath(), aside from expanding relative paths, would convert / to \ on a windows platform.
    - dmpop9mm 11y ago
      Thank you very much for the useful pointers!
nacs 11y ago
Looks like a good start and seems to do its job. A suggestion -- you mention "Automatic thumbnail generation" in the feature list but the thumbnails in the demo have massive file sizes (~80-100KB each at 800px width). Thumbnails should generally be <20KB and <320px width. You could re-use the `createTim()` function you have to generate an additional, proper thumbnail instead of the 800px preview image squashed down via HTML as you do now.
- dmpop9mm 11y ago
  Thank you! I'll definitely consider this.
devwerks 11y ago
I would also suggest to add some validation to your file upload. Currently you are allowing arbitrary files to be uploaded. Even though it is password protected, it is still trivial to brute force even with your sleep(3). The danger with allowing arbitrary file upload is someone can upload a script (PHP file for example) and run it basically allowing arbitrary code execution. This can lead to a whole slew of other issues. To get started, I would suggest: 1. Generating your own file names w/extension instead of relying on $_FILES['filetoupload']['name'] 2. After move_uploaded_file(), change file permissions to 644 to help mitigate possibility of file execution 3. Use getimagesize() to determine if file is indeed an image. It is still possible to embed code into a validate image to bypass getimagesize(), but #1 will help prevent Apache/etc from interpreting the file as PHP. 4. Ideally you would also strip metadata from the image file and only keep resized images and delete the originals. Also would suggest that you do not use the same password on your demo site as you have posted on Github.
- dmpop9mm 11y ago
  Thank you so much for your reply! I've decided to remove the upload form altogether, but I'm sure I'll make good use of your tips in the future.
replax 11y ago
looks pretty cool! have you thought about integrating the php app into other projects? I know for example that ownCloud has great problems to efficiently display images and something like what you created looks a lot better than what they have so far. Keep it up :)!

29 points by dmpop9mm 11y ago

I'm not a coder by any stretch of imagination, but I've managed to cobble together this simple PHP app for hosting my photos. Constructive feedback will be greatly appreciated.

devwerks 11y ago
It's great that you had a need and tackled it by writing a script. However, after a brief look at your source, you have a directory traversal vulnerability - there may be other issues too. You are taking a query parameter "d" and appending that to photo_dir which is then used in a variety of places. https://www.owasp.org/index.php/Path_Traversal https://www.owasp.org/index.php/Path_Traversal describes what a directory traversal is. Take a look at https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet or http://www.phptherightway.com/ http://www.phptherightway.com/ for some primers.
- dmpop9mm 11y ago
  Thank you very much for your feedback and the pointers!

Antwan 11y ago

How can "single dirty file mixing PHP HTML CSS" can be showcased as a feature in 2016...

sdoering 11y ago
Does this trolling add anything to the learning experience of the OA? Esp. after he clearly told that he would really like constructive feedback and stated, that he has no real coding experience. Why would you play elite and kick someone lower in the ranks his way? It only shows your true colors. And imho they are not really nice in any way.
danielhunt 11y ago
This script is probably step number 5 in the 'how do I start out on this web-programming lark?' journey. There is absolutely nothing wrong with being proud of it, regardless of the language or inevitable vulnerabilities that come from being inexperienced with web development. It's a very small leap to go from this, to understanding separation of concerns. Go easy on newcomers. We're not that elite.

snowpanda 11y ago

I like your coding style, it's very organized.

madaxe_again 11y ago
What, JS, PHP, and HTML all in the same procedural script? I get that the idea is that it's single-file, but it doesn't make for pretty (or terribly maintainable) code. It's a neat little script, but I see at least one severe exploit in there ($filename - leave the rest to you).
- dmpop9mm 11y ago
  Thank you for pointing out the exploit! I'll see what I can do.

NamPNQ 11y ago

Look like pasteboard?

tyingq 11y ago

There's a bit of code to try and stop path traversal, but it doesn't account for passing d=.. as a url param. A shortened test case using the same code as the app to try and thwart traversal: <?php $_GET['d']='..'; $photo_dir='photos/'; $sub_photo_dir = basename($_GET['d']).'/'; $photo_dir = str_replace("//", "/",$photo_dir.$sub_photo_dir); print "photo_dir: [$photo_dir]"; ?> That would allow you to upload a file one directory above the intended $photo_dir.

dmpop9mm 11y ago
Good point. I'll look into it. Thanks!
- tyingq 11y ago
  The typical approach is to use realpath(). That would expand out any relative portions of the path, which you could then compare directly with the expected path and know for sure it's in a subdirectory of your intended photo directory. If you go that way, though, you'll also want to stop using the literal '/' and use the DIRECTORY_SEPARATOR constant so that your script runs on windows. realpath(), aside from expanding relative paths, would convert / to \ on a windows platform.
  - dmpop9mm 11y ago
    Thank you very much for the useful pointers!

nacs 11y ago

Looks like a good start and seems to do its job. A suggestion -- you mention "Automatic thumbnail generation" in the feature list but the thumbnails in the demo have massive file sizes (~80-100KB each at 800px width). Thumbnails should generally be <20KB and <320px width. You could re-use the `createTim()` function you have to generate an additional, proper thumbnail instead of the 800px preview image squashed down via HTML as you do now.

dmpop9mm 11y ago
Thank you! I'll definitely consider this.

devwerks 11y ago

I would also suggest to add some validation to your file upload. Currently you are allowing arbitrary files to be uploaded. Even though it is password protected, it is still trivial to brute force even with your sleep(3). The danger with allowing arbitrary file upload is someone can upload a script (PHP file for example) and run it basically allowing arbitrary code execution. This can lead to a whole slew of other issues. To get started, I would suggest: 1. Generating your own file names w/extension instead of relying on $_FILES['filetoupload']['name'] 2. After move_uploaded_file(), change file permissions to 644 to help mitigate possibility of file execution 3. Use getimagesize() to determine if file is indeed an image. It is still possible to embed code into a validate image to bypass getimagesize(), but #1 will help prevent Apache/etc from interpreting the file as PHP. 4. Ideally you would also strip metadata from the image file and only keep resized images and delete the originals. Also would suggest that you do not use the same password on your demo site as you have posted on Github.

dmpop9mm 11y ago
Thank you so much for your reply! I've decided to remove the upload form altogether, but I'm sure I'll make good use of your tips in the future.

replax 11y ago

looks pretty cool! have you thought about integrating the php app into other projects? I know for example that ownCloud has great problems to efficiently display images and something like what you created looks a lot better than what they have so far. Keep it up :)!