The main code is insecure :
* no real check for succes on operations,
* SQL injection opportunities /!\
* module does not respect coding conventions

Comments

silviogutierrez’s picture

silviogutierrez commented

Where are the injection opportunities? Please manifest some examples before making these claims.

Silvio

vm’s picture

vm commented

This should be discussed in email I think. If there are vulnerabilities you don't want them to be public before they are fixed, if there arises a need for them to be fixed.

silviogutierrez’s picture

silviogutierrez commented

Very good point. I'm just wondering how there are vulnerabilities if all user input was done through db_query() calls using placeholders.

Silvio

vm’s picture

vm commented

the user with the report has only been a user for about 6 hours as of the time of this post.

klando’s picture

klando commented

db_query does not prevent string injection, I hope some db_text_escape have to be used. Currently you directly input the exim or iptc content in the db_query query.

I am new user to drupal yes.

silviogutierrez’s picture

silviogutierrez commented

db_text_escape is not a currently available function in the Drupal database API. If you're referring to db_escape_string, then you should know this function only works in PosgreSQL and not MySQL, so using it is pointless.
Using db_query() with placeholders for the arguments, as I used throughout the entire module, escapes each argument and makes it safe against an injection.

Silvio

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: insecure code » Coding style issues

silviogutierrez is right. Everything that goes through db_query() is automatically escaped. Please inform yourself better before making claims of insecurity.
Renaming this issue to remove the FUD.

klando’s picture

klando commented

Silvio, I am really sorry about that misread.

For info, it seems db_escape_strings is use in both mysql and postgresql.

Well htis ticket was not only about sql injections, but also for strange tests:

$success = false;

$path = file_directory_path() . '/gallerix';
$success = mkdir($path);
$success = chmod($path, 0777);
$success = mkdir($path . '/trash');
$success = chmod($path . '/trash', 0777);
$success = mkdir($path . '/repository');
$success = chmod($path . '/repository', 0777);
$success = mkdir($path . '/albums');
$success = chmod($path . '/albums', 0777);

if (!$success) {

Usefull ? Also db_query sucess are not checked, I know that because install failled silently on my tests.

silviogutierrez’s picture

silviogutierrez commented

Hey Klando,

No problem.

Those tests just make sure that all the album folders were created successfully. They're not really necessary so they may be removed in the next version.

I will take a look at the install script to make sure PostgreSQL is working.

Thanks for your input,

Silvio

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Closed (won't fix)

The Drupal 5 version of this module is no longer maintained.