Hacker News new | past | comments | ask | show | jobs | submit
I prefer to post issues to the public because that benefits more people in general. Not always true for security issues, but somehow miraculously, I didn't spot any severe issue to warrant that yet. So the following is a bit more detailed explanation for each item.

"No error handling." You need to check absolutely any unexpected situation in general. For example, I can easily trigger a PHP warning with a null character (https://old.net.eu.org?p=asdf%00asdf). These file names directly go into the content file path, only going through some "sanitization" steps that remove /, \ and ".txt". Don't do that, instead only accept a class of characters that you are absolutely sure to be safe.

"No XSS protection." Cross-site scripting (XSS) allows attackers to inject some unrestricted contents into the output, and the systematic way is to escape virtually everything with htmlspecialchars() and so on. Many modern web frameworks even do escaping by default unless explicitly instructed to, but PHP is too old to benefit from that. Be absolutely pedantic about escaping; there are several places with missing escapes in admin.php, while index.php doesn't have one by itself at least for now. (XSS in the title is probably from a custom modification.)

"No CSRF protection." Cross-site request forgery (CSRF) is possible when destructive commands can be sent outside of the origin website. Specifically, if the session is already enabled and some external website has a form that links to admin.php with a destructive command, that command will be immediately executed regardless of its origin due to the HTML form's legacy requirement. There are many mitigations, but the easiest applicable way would be to add random bytes to the session on login and require the form to resend that bytes on command. Of course details matter a lot, like how those random bytes are generated.

"No atomic file writing." Different storage methods have different guarantees for the stored data and applicable operations. Filesystems typically have a strong guarantee that no operation or (temporary) disruption renders the whole filesystem unusable, while the data itself is subject to much more relaxed guarantees unless specifically requested. In your case, any file update may be interrupted in the middle and that partial write will be present after the interruption. (And of course multiple updates to the same file in parallel will result in an interleaved file.) I think there is no built-in PHP function for atomic file writing (and no, file_put_contents can't do that), so you need to make one.

"No correct permission in the data directory." It is, to my knowledge, still common that a webserver runs as its own user distinct from everyone else. So any filesystem access will be done as that user, and especially file writes won't be possible unless the data directory is set to allow that. Many webserver-writable directories had too many permissions enabled for anyone (e.g. chmod 777) as a result. You should have some solution to guide users to set the correct and only necessary permission on such cases, and that requires some understanding about UNIX permission system.

"No real way to change password which is built into admin.php." This is probably what most users would expect to be able to do. Also password is written in a plain text, which is yet another potential issue but probably less important in this particular setting as long as HTTPS is used. (You may want to always enforce HTTPS for that reason.)

"No constant-time comparison on login." Any credential comparison might be vulnerable to the timing attack, which uses the fact that the time to do a short-circuited comparison reveals how long prefix does match. PHP does have a dedicated `hash_equals` function that compares both strings all the way, though it (and any other such functions) does necessarily reveal the length of password, which is why we have password hashing functions in general.

"No correct template handling." Any pattern of [[foo.txt]] will be replaced. But the actual replacement is done with str_replace, which means that if foo.txt had [[bar.txt]] then it might or might not be replaced depending on whether [[bar.txt]] was also in the original file. Use `preg_replace_callback` instead, possibly with a cache to read any included file at most once. You might also implement recursive templating with some more logics.

"Unlimited writes to the session even when credentials do not match." The admin.php file doesn't actually have no login action, instead it overwrites $_POST[...] to $_SESSION[...] whenever they present before the actual checking. So an attacker might not be able to log on, but might be able to use a large amount of session storage which is only bounded by the maximum memory limit. The checking should be done before writing anything into the session storage instead.

"No protection against any attempt to use fopen wrappers after login." PHP (still) has a questionable feature to accept URLs in place of file paths, which can be used to send an attacker-controlled request to other servers for example. Index.php always constructs any path with a fixed prefix so this attack is impossible, however only accidentally. Admin.php doesn't fully protect the attack though.

Finally, file uploading would be quite difficult to implement safely. This is because PHP used to work "transparently" from views of web servers, so if you expose the uploaded file directory in public, then that directory is no different from the directory that hosts index.php and admin.php. That directory should be ideally hidden from the public view for this reason, and some dedicated script should be instead used for serving any uploaded file. Do not use an allowed file extension list alone to solve this, there exist many workarounds for that.

did you test the version on the site, what about the version on github? the difference between them is half a year, and several releases. the site has the very first version of the script, naturally there were errors then, but I thought you were looking at the latest
I only looked at Github and didn't realize the website used an older version until you said so. (I wondered why admin.php didn't seem to exist.) These issues are so well-known enough that I didn't even have to run the script at all.
the admin panel can be renamed. this does not affect the functionality. they are not linked. it may not exist at all

another addition - the system is single-user, it was created for a business card site, not for a blog or a corporate site, there can't be more than one user here. that's the idea, it's made for small net, it's not another wordpress with frameworks, it's created for the simplest tasks

Yes that was what I assumed, but it ideally should be safe without the renaming. Security by obscurity only works to the limited extent.
and by the way - thank you very much for your comments, you really help, I will try to take into account all these points, it is impossible to take everything into account at once, this is a human factor, in general I thought to expand the functionality a little, but you showed me the problems that need to be (and can be) solved. nevertheless, if you have a solution to the problems, you know my email, I will be very grateful. although you have already saved me a lot of time without this. thank you