Skip to main content
added 3 characters in body
Source Link
Kate
  • 8.8k
  • 9
  • 22

To determine that the user is logged in you rely on this:

It's not knowknown how and when this variable is set, and whether it will always be in sync. I would rather expect a reference to a session object here.

To determine that the user is logged you rely on this:

It's not know how and when this variable is set, and whether it will always be in sync. I would rather expect a reference to a session object here.

To determine that the user is logged in you rely on this:

It's not known how and when this variable is set, and whether it will always be in sync. I would rather expect a reference to a session object here.

Source Link
Kate
  • 8.8k
  • 9
  • 22

Security

If you are worried about security (and you should), then you should not connect to the database with the root user. The Mysql root user has extended rights like reading other databases, reading arbitrary files on the file system, and even writing to some designated directories (or worse if you have explicitly tweaked configuration).

In case of vulnerability in your code (more specifically a SQL injection) this could escalate to takeover of your server, in addition to exfiltration of your database.

At first glance, I think this is the biggest oversight. A hacker will be going for the low hanging fruit, and an attack will not necessarily take place where you expect.

Create a user that has access to that database only, and do not grant more rights than necessary for your purpose. Usually you just need to read from and write to tables, you don't need to change schema or FILE privilege.

Speaking of SQL injection, you've done what has to be done.

Unchecked checks

Some stuff is redundant eg:

// Check if upload is not empty
if(empty($_FILES['upload'])){
    die('{"error":"Faça upload da imagem"}');
}

// Check if upload is not empty
if (empty($_FILES['upload']['tmp_name'])) {
    die('{"error":"Faça upload da imagem"}');
}

It should be sufficient to use the is_uploaded_file function and then there is no conceivable reason why the tmp_name attribute would be missing. Even if that happened, an exception would be raised and the code would simply stop.

And you also do this further down:

// Check if tmp file exists
if (!file_exists($_FILES['upload']['tmp_name'])){
    die('{"error":"Imagem não encontrada no servidor."}');
}

Which is another unneeded check. The file must exist if the previous checks have gone through.

Make sure you have read the PHP docs on file uploads, which would be a good starting point to understand the possible pitfalls.

This at line 93:

// Verify File Mime with GD
if (!in_array($verifyimg2['mime'], $mimePermitidos)) {
    die('{"error":"Tipo de arquivo não permitido."}');
}

is a repetition of lines 58-60:

if (!in_array($tipoArquivo, $mimePermitidos)) {
    die('{"error":"Tipo de arquivo não permitido."}');
}

As far as I can tell, you are still processing the same image. It seems to me that you've added so many checks that you are losing track of the process.

This code could be simplified a bit.

It would surely be useful to split your code in small functions. For example, write a dedicated function to check images. Then the redundant code would stand out better.

File uploads are dangerous if not done right. Probably the most important is to restrict the file extension: if an attacker can upload a .php file (a webshell) to your server then you have a problem. Some webserver configuration can still mitigate this problem, for example I believe Nginx won't run files it does not own. Other pitfalls to avoid include directory traversal attacks, which can happen if rely on file name without sufficient sanitization.

I believe the code is safe enough at first glance. I haven't run it though, and this is nothing more than visual analysis.

File size and disposal

You're restricting the file size to 2 Mb which is low by today's standards. Pictures from digital cameras or smartphones typically weight a couple Mbs. Many of your users will have to tweak with settings or edit the picture on their own before they can even upload it to your site. Consider raising the limit a bit. Since you are not using the move_uploaded_file function but building a new image, I suggest you erase the temporary file when you're done with it. Although the temp directory should get flushed at the next reboot, some servers do not reboot frequently. It's conceivable that the temp could fill up under heavy usage.

Misc

To determine that the user is logged you rely on this:

$ta_logado == 0

It's not know how and when this variable is set, and whether it will always be in sync. I would rather expect a reference to a session object here.