Auditing Code (security wise)

Yes, this pratice is really a dangerous pratice, but with a few precautions you can be lower the risk to use this type of code.

Below I put 2 links that proposes improvements to this problem differently. Perhaps using a mix of both can improve safety

http://anekostudios.com/2007/09/20/php-forms-security-vulnerability/

http://www.alt-php-faq.org/local/115/

I did a quick search and created a bug report for it: #1850769

Here's another good article about preventing sql injection attacks with php - http://www.imneebu.com/2007/11/20/preventing-mysql-injection-with-php/

yep, nice read that steve.. clear & precise.

i'm glad i never became under the illusion that a security audit would be quick & easy.. lol i've only done 2 very basic checks and those 2 alone took me over 12hrs constantly to get through.

but we have to continue with the audit on an ongoing basis, and we should also audit modules that are to be released alongside the core distribution. (yes more work to do but esential work non the less if we want impressCMS to be secure as we can)

but what we also will need to do is take notes of exactly what we are doing security wise, and explain why those changes are necessary and why we devs should do it that way instead of this way and so on. we need to make module devs and future core devs aware of the right way to do things as opposed to the insecure(r) ways, so that they aswell as ourselves don't reintroduce some of the insecure coding practices back into the core and/or modules. It would also make auditing a lot quicker if we can get devs to understand the basics of things. like encapsulating all query values in single quotes including integer values, and using sql_escape functions etc.

sometimes the little thingsd get overlooked and missed due to the complexity of the code we are developing at times, but it's sometimes those little things that mean a secure site as opposed to an insecure site.

I think we are all learning things here, especially me.

You're absolutely right, Vaughan. One of the articles I read made it very clear that security was not a condition, but a process.

I came across this thread at x.o and a very valid question by catz.

Quote:

i share the same opinion of catz in that integer values should only be treated as integer values. and htmlspecialchars is not correct usage there.

i think personally if integer values have to be treated as string variables then there is a more underlying issue with sanitation.

also whilst working on the core code, there are many instances where addslashes & stripslashes are used throughout xoops. I understand that addslashes/stripslashes are necessary (or are they?) if magic_quotes_gpc is enabled.

according to some security auditing texts I have read, addslashes & stripslashes should be avoided and replaced with proper sanitizing methods and make use of mysql_real_escape_string() instead.

see http://uk2.php.net/manual/en/function.mysql-real-escape-string.php

or have i misunderstood something?

I have read the same about addslashes/stripslashes and that the mysql function is better

Stickied the topic

Maybe a good place to start is a short doc on 'good practice' and a checklist for filtering data and queries? Perhaps this could be the foundation of a 'security review' process for modules?

I got to research this so I can release my first module. I'm not expert though so I would need a lot of input from others.

Vaughan,

I have come to the same conclusion. mysql_real_escape_string() should be used instead, because addslashes is not 100% secure in use with a number of character sets.

Best wishes

It would be possible soon hold a general audit in version 1.1 on any failure of security with some automatic system.

I am very concerned because the amount of new code is enormous.

[pt-br]
Seria possível em breve realizar uma auditoria geral na versão 1.1 sobre alguma falha de segurança com algum sistema automático.

Estou muito preocupado porque a quantidade de novo código é enorme.
[/pt-br]

Ok. So I am still learning PHP and need to get a handle on how to write secure code. So I spent most of today trolling through a few PHP books, the PHP manual and this thread, and here's a summary list of things I think I need to check for a module. Any other ideas?

1. Variables

• Always initialise variables, particularly if register_globals is enabled.

2. Ensure input is filtered

• All user-supplied data must be inspected to ensure that it is valid before it is accepted for use. This includes all $_GET, $_POST (including hidden fields!), cookie (and session?) variables.

• Use a 'whitelist' approach: Data is considered invalid unless it can be proven to be valid.

- Check all required variables are present.

- Check that values have been assigned.

- Check the data type is as expected. Functions include:

+ Is_string()

+ Is_numeric() // will also return true if data is a string containing a number

+ Is_float() // remember data passed in by form is typed as string

+ Is_array()

+ Is_object()

+ Ctype functions permit more specific screening of input (see PHP manual).

- Check the value is within acceptable ranges. For example:

+ A select box should only return one of the listed values.

+ Names should only include alphanumeric characters.

- Reject data that does not conform to expectations.

3. Eliminating SQL injection

• An SQL injection vulnerability exists whenever you used un-escaped data in an SQL query.

• Use msql_real_escape_string() to escape output.



• All values within a query should be contained within quotes regardless of data type.

• Queries that alter the database should be within a $_POST request rather than $_GET.

4. Cross-site scripting (maybe should be called 'html injection')

• A vulnerability exists whenever un-escaped data is output, eg:



• Escape the data with htmlentities:



• URLs containing variables that could potentially have been manipulated by users need to be escaped twice:



5. Session fixation

• May be conducted as a prelude to session hijacking. To prevent, regenerate the session identifier with session_regenerate_id() whenever there is an escalation of privilege (eg. logging in).

6. Filenames

• Be wary of opening files with user-supplied 'filename data'. Users may pass something nasty instead, like a relative path to a sensitive local file or a URL to open a remote file with exploit code. Don't forget that fopen(), include() and require() all accept URLs as arguments. To prevent problems:

- Disable remote file access (allow_url_fopen).

- Use the open_basedir option in PHP to restrict filesystem access.

- Check filenames with realpath() and basename().

• If you have to allow users to specify a file name, use a combination of realpath() and basename() to verify the relative path to the file on your site:

- realpath() returns the full path to a file, resolving any special characters like directory traversals.

- Basename() just returns the file name from the path.

- Combine them to verify the file name by resolving the full path to the file then extracting the file name and comparing it with the user-supplied one:



7. File uploads

• Don't use user-supplied filenames sent by a browser. Create your own temporary file name to handle it.

• Set a maximum uploadable file size with post_max_size in php.ini to avoid DOS attempts to jam your server.

• Cookies can be used to overwrite global variables in GET and POST. Check that the uploaded file really *has* been uploaded using is_uploaded_file()

• Use PHP's move_uploaded_file() which moves a file only if it was uploaded. Use this in preference to copy() or system level functions.

8. File access

• Restrict filesystem acess to a specific directory using the open_basedir in php.ini.

• Store sensitive data in the database in preference to files [perhaps trust_path is a better option in Impress?].

• If you are on a shared server ask the hosting company to store your session files in your own session directory, because otherwise you are probably sharing a session directory with all the other people on the server and everyone's scripts can access everyone else's session files. This involves setting a session path in the VirtualHost block in Apache's httpd.conf.

- php_value session.save_path /myown/path

• If you have .htaccess rights and Apache is configured to let you over-ride options you may be able to make this change yourself.

9. PHP code

• Disable dangerous function calls like system() and eval() by setting disable_functions in php.ini.

- disable_functions system, eval

10. Environment

• Disable the following in php.ini:

- register_globals
- magic_quotes_gpc
- allow_url_fopen

I'd also add that if you're using a system like ours - perhaps making usage of other security methods, purifyier, trustpath etc is a must...

Exelent Zaphod


Added in Wiki:Auditing Code

Good thinking Giba - and if anyone has any additional advice to add to this, please do so!

A very nice summary, Zaphod!

Quote:

Properly casting the variables is important for many reasons, just be aware of your options and the impact on performance.

Similarly, I feel if the data and variables are handled properly, the numeric values in sql strings don't need quotes, which also improves performance of the database. By proper handling I mean -
* The variable has been declared and initialized
* The input has been sanitized
* The input has been encoded, if appropriate
* The input has been validated, and
* The input has been cast as the expected type (int, float, string, array)

In fact, using string values in the query when the field type is numeric can lead to different results - http://dev.mysql.com/doc/refman/5.1/en/type-conversion.html

PHP 5.2+ has filters for sanitizing and validating many types of input - we should be taking a close look at these, since we have gone php5 with 1.1.

Here's a slideshow from the PHP web site about input filters

General security talks are here - http://talks.php.net/index.php/Security

Another good one is by Rasmus Lerdorf - http://talks.php.net/show/ygatech3/0