Announcement

  •  » Engine
  •  » Does piwigo use prepared queries?

#1 2020-05-12 00:33:12

VictoriqueMoe
Member
UK
2020-05-12
3

Does piwigo use prepared queries?

Hello/Hi/Greetings,
THIS could be me not understanding the codebase well, so it is just a question.

I use cloudflare for my CDN, and because of this, the IP address captured in the history is wrong, and i need to change the header that obtains it. sure no problem.

in functions.inc.php i find the SQL statement to insert this info. but to my surprise, the string is built from the variables by concatenation instead of prepared statements:
e.g "INSERT INTO foo (firstname, lastname, email) VALUES (?, ?, ?)" and then the pwg_query function, i expected to take an array of values to bind.

even if the pwg_query was a generic query/insert function, it could use an abstract DAO class or even an optimal param for inserts.

Now, i can see that pwg_query is called from a lot of places, and I admit i have not inspected more obvious places for SQL injection like the register page.


Although i was surprised of the lack of an obvious architecture of the code, classes, abstraction, interfaces, etc... or even MVC. but putting that aside, this software works VERY well and is really awesome, I love it

I am just worried about security with SQL injection here.

I tried looking on your forum for a question like this but could only find an unrelated question about moving to PDO, of that, I do not care.

Piwigo version: 2.10.2
PHP version:  7.3.7
MySQL version: MariaDB 10.4
Piwigo URL: https://album.victorique.moe

Last edited by VictoriqueMoe (2020-05-12 00:37:04)

Offline

 

#2 2020-05-13 02:21:20

erAck
Only trying to help
2015-09-06
1998

Re: Does piwigo use prepared queries?

I see two INSERT INTO statements in include/functions.inc.php but none that takes your example of (firstname, lastname, email) or other user input.

User input is processed with pwg_db_real_escape_string() before being used in queries, see include/functions_user.inc.php for examples, which in turn uses the mysqli::real_escape_string() function.


Running Piwigo at https://erack.net/gallery/

Offline

 

#3 2020-05-13 14:14:30

VictoriqueMoe
Member
UK
2020-05-12
3

Re: Does piwigo use prepared queries?

erAck wrote:

I see two INSERT INTO statements in include/functions.inc.php but none that takes your example of (firstname, lastname, email) or other user input.

User input is processed with pwg_db_real_escape_string() before being used in queries, see include/functions_user.inc.php for examples, which in turn uses the mysqli::real_escape_string() function.

Thank you for your reply!

Yes, the SQL statement i posted was just an example i made up, a real example would be:

$query = '
INSERT INTO '.HISTORY_TABLE.'
  (
    date,
    time,
    user_id,
    IP,
    section,
    category_id,
    image_id,
    image_type,
    format_id,
    auth_key_id,
    tag_ids
  )
  VALUES
  (
    CURRENT_DATE,
    CURRENT_TIME,
    '.$user['id'].',
    \''.$ip.'\',
    '.(isset($section) ? "'".$section."'" : 'NULL').',
    '.(isset($page['category']['id']) ? $page['category']['id'] : 'NULL').',
    '.(isset($image_id) ? $image_id : 'NULL').',
    '.(isset($image_type) ? "'".$image_type."'" : 'NULL').',
    '.(isset($format_id) ? $format_id : 'NULL').',
    '.(isset($page['auth_key_id']) ? $page['auth_key_id'] : 'NULL').',
    '.(isset($tags_string) ? "'".$tags_string."'" : 'NULL').'
  )
;';

this is line 482 in functions.inc.php

then that string is passed to pwg_query

then inside of pwg_query in function_mysqli.inc.php it is being used at line 141 without any escaping:
($result = $mysqli->query($query)) or my_error($query, $conf['die_on_sql_error']);

Would it make sense to maybe make an Database abstraction layer so all queries going through the MYSQLI interfaces go through the escape method? like some sort of DAO?

I expected to see at least a mysqli_stmt_bind_param, but maybe this is my lack of php knowledge about PDO mysqli_stmt_bind_param and real_escape_string

Again, there could be a corner case here that I am not following and maybe that insert does not need to be escaped for some reason, although i would say all inserts should be.

Many thanks,
Martha

Last edited by VictoriqueMoe (2020-05-13 14:15:17)

Offline

 

#4 2020-05-13 16:54:48

plg
Piwigo Team
Nantes, France, Europe
2002-04-05
13786

Re: Does piwigo use prepared queries?

Hi VictoriqueMoe,

VictoriqueMoe wrote:

I use cloudflare for my CDN, and because of this, the IP address captured in the history is wrong, and i need to change the header that obtains it.

In your local/config/config.inc.php, write something like:

Code:

// load balancers
$reverse_proxies = array(
  '1.2.3.4',
  '1.2.3.5',
);

if (in_array($_SERVER['REMOTE_ADDR'], $reverse_proxies))
{
  if (isset($_SERVER['HTTP_X_FORWARDED_FOR']))
  {
    $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_FORWARDED_FOR'];
  }

  // in case we have several IP addr, we only keep the last one. Examples
  // seen in Apache log files:
  //
  // unknown, 8.9.10.11
  // unknown, unknown, 12.13.14.15
  if (strpos($_SERVER['REMOTE_ADDR'], ',') !== false)
  {
    $_SERVER['REMOTE_ADDR'] = trim(array_pop(explode(',', $_SERVER['REMOTE_ADDR'])));
  }
}

[...] the variables by concatenation instead of prepared statements:
I am just worried about security with SQL injection here.

That's a good concern and you're fully right to ask this technical question. As erAck said earlier, we use the pwg_db_real_escape_string() function before using a string in an SQL query, if this variable comes from user.

I would also add that we use the check_input_parameter() function nearly everywhere. For example, on page search.php, you can find:

Code:

check_input_parameter('mode', $_POST, false, '/^(OR|AND)$/');
check_input_parameter('fields', $_POST, true, '/^(name|comment|file)$/');
check_input_parameter('tags', $_POST, true, PATTERN_ID);
check_input_parameter('tag_mode', $_POST, false, '/^(OR|AND)$/');
check_input_parameter('cat', $_POST, true, PATTERN_ID);
check_input_parameter('date_type', $_POST, false, '/^date_(creation|available)$/');

if the user input does not validate the filter, the execution is stopped and of course, the variable won't be used in any SQL query.

[...] i was surprised of the lack of an obvious architecture of the code, classes, abstraction, interfaces, etc... or even MVC.

There is an architecture, but maybe not what you're expecting :-) Remember that Piwigo codebase is 18 years old. We have been adding classes (like the management of multiple size, or plugins) but the basic architecture is procedural.

A very long time ago, we've tried to replace it by an MVC model but with 10% of features it was 1000% slower... so we sticked to the original, simple and efficient architecture. Now with triggers everywhere, it's very complicated to reset all this and switch to something else, because it would break all plugins and Piwigo without its plugins is far less interesting.

[...] but putting that aside, this software works VERY well and is really awesome, I love it

Thank you :-)

Offline

 

#5 2020-05-13 17:16:11

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Does piwigo use prepared queries?

plg wrote:

A very long time ago, we've tried to replace it by an MVC model but with 10% of features it was 1000% slower... so we sticked to the original, simple and efficient architecture. Now with triggers everywhere, it's very complicated to reset all

It's complicated and it's a long time job but it's possible! ;-) And the render time is similar.

plg wrote:

this and switch to something else, because it would break all plugins and Piwigo without its plugins is far less interesting.

Without the end of that sentence I could have argue something. I hope I can prove you some day it's possible.

Offline

 

#6 2020-05-13 17:45:01

VictoriqueMoe
Member
UK
2020-05-12
3

Re: Does piwigo use prepared queries?

Wow! thank you for such a detailed response! I feel much more happy now you explained it to me :)

as for the IP issue, i solved that by using a simple ternary statement:

Code:

$ip = (isset($_SERVER["HTTP_CF_CONNECTING_IP"]) ? $_SERVER["HTTP_CF_CONNECTING_IP"] : $_SERVER['REMOTE_ADDR']);

I did not see that check_input_parameter, but then again, I did not check out how the mysql commands are wired together with the common functions, so that is bad investigation on my end :/ haha

wow, that old? that is crazy! I really love this software and will be using it forever and hope it does not stop development! I am a full stack dev as occupation and know what you mean by building on top of legacy or older systems, so I 100% understand that!

I take my hat off to you guys for making such a wonderful and modulated media application! the use of html templates and plugins is really awesome!

I do understand how changing the architecture of the codebase would break plugins, I found some of the plugins already are starting to cause php exceptions.

Something like php cake or a very efficient MVC engine and factory, engine, manager pattern would be really quick, with the addition of Dependency injection frameworks would make plugin development so much easier and concise, instead of a file of functions, it would be like "just implement this plugin interface and the manager will register the plugin with the plugin factory" decoupling the different layers of the architecture

THAT ASIDE, that would be a whole rewrite of the software and heh, as it is right now, it works beautifully


Sorry for the rant. just geeking out, but this issue i would say is solved! and you deffo kept SQL injection in mind, and that is good to know!

Many thanks,
Martha

Last edited by VictoriqueMoe (2020-05-13 17:49:39)

Offline

 

#7 2020-05-13 18:13:00

plg
Piwigo Team
Nantes, France, Europe
2002-04-05
13786

Re: Does piwigo use prepared queries?

nicolas wrote:

plg wrote:

A very long time ago, we've tried to replace it by an MVC model but with 10% of features it was 1000% slower... so we sticked to the original, simple and efficient architecture. Now with triggers everywhere, it's very complicated to reset all

It's complicated and it's a long time job but it's possible! ;-) And the render time is similar.

Changing everything for the sake of matching a technical trend is pointless, in my opinion. What matters is usage. Piwigo 2.9 and 2.10 have been improved with mainly usage (UI/UX) in mind and so will 2.11. I'm 100% confident that it's what matters to users. Far more than using such technical architecture.

To make a comparison with more "real world" stuff, what is the most important selection criteria people consider for a car:

1) color/beauty
2) price : pricey if you want to show you have money, cheap if you don't want to spend money
3) practicality
4) horse power (only for men and 0.1% women)
...
145) material used for the front wheels brake calibers

I'm sure you understand what I mean. Users don't care about that kind of details. They care about the design, the user experience, the speed.

On the other side, developers, like you nicolas, care about such technical details. I know it. You have explained it several times and I understand :-) But we have to make choices on where our energy goes and for now, what matters most is what matters to users.

Offline

 

#8 2020-06-07 04:03:46

pcronin
Member
Albany, New York, USA
2019-09-29
5

Re: Does piwigo use prepared queries?

I'd like to add my voice to those concerned with the lack of bind params. Using bind parameters is essentially a guarantee that SQL injection attempts won't work, but without bind parameters, it only takes one missed pwg_db_real_escape_string to create an SQL injection vector.

There may not be a security vulnerability now, but there could be one in the future. From my perspective, it'd be easy enough to move the codebase to using bind parameters in place of the existing string concatenation.

If the dev team would be interested in discussing and approach, or reviewing a small PR, I'd love to work on this. It could be done in small bits; no massive single PR.

FWIW, I found it really funny that you selected the brake caliper as an example of what car owners don't care about. Here's a question of mine regarding brake calipers from a couple weeks ago: https://mechanics.stackexchange.com/que … ssurizing. I guess I'm also not a regular "user," but I do think I could be helpful!

Offline

 

#9 2020-06-08 12:01:03

plg
Piwigo Team
Nantes, France, Europe
2002-04-05
13786

Re: Does piwigo use prepared queries?

pcronin wrote:

From my perspective, it'd be easy enough to move the codebase to using bind parameters in place of the existing string concatenation.

If the dev team would be interested in discussing and approach, or reviewing a small PR, I'd love to work on this. It could be done in small bits; no massive single PR.

OK, I'm interested in a Pull-Request which uses a prepared query for this query:

Code:

  $query = '
SELECT '.$conf['user_fields']['id'].' AS id,
       '.$conf['user_fields']['password'].' AS password
  FROM '.USERS_TABLE.'
  WHERE '.$conf['user_fields']['username'].' = \''.pwg_db_real_escape_string($username).'\'
;';

in include/functions_user.inc.php, function pwg_login. Can you give it a try?

Offline

 

#10 2020-06-08 13:48:49

pcronin
Member
Albany, New York, USA
2019-09-29
5

Re: Does piwigo use prepared queries?

I would be happy to give it a go!

Offline

 

#11 2021-05-31 22:47:36

mangodan2003
Member
2021-05-29
1

Re: Does piwigo use prepared queries?

It seems even now SQL injection can be achieved quite trivially simply by adding files with specially crafted names.  Possibly not the files themselves but the directories they are in. I just tried to add an existing directory tree of photos structured <year>/<date> - <description>. I had to allow some additional chars in the regex filter due to hyphens, ampersands etc being used in the descriptions. Now I get all manor of SQL errors coming up where MariaDB is treating the descriptions as SQL queries! Ok I have administrator rights to 1, amend the filters, 2  add an existing tree of named directories, but this would all be fine is prepared statements were being used.

Offline

 

#12 2021-06-01 02:21:02

erAck
Only trying to help
2015-09-06
1998

Re: Does piwigo use prepared queries?

While parameterized prepared statements could defy some apostrophe trickstery (which is why you as admin should never add the ' single quote apostrophe character to sync_chars_regex) and other "data as program" problems, they are not an all-healing magic wand that would lead to a "this would all be fine" situation; it would not help against an interpretation of & ampersand as HTML query parameter separator if it was stored as part of a path name, which is not even related to SQL injection.

However, Piwigo surely would hugely benefit from prepared statements and if done consistently one day could allow ' apostrophe in directory and file names..


Running Piwigo at https://erack.net/gallery/

Offline

 

#13 2022-03-06 13:19:32

tomchiverton
Member
2017-07-30
13

Re: Does piwigo use prepared queries?

Using prepared queries or at least calling addslashes would also allow ' in album names, at least one open issue in the public issue tracker  : [Github] Piwigo issue #1572

But it's been like this for 6 months now, so I wonder if the project cares that much.

Are at least all the public (unauthenticated) calls parametrised ?

Last edited by tomchiverton (2022-03-06 13:25:50)

Offline

 
  •  » Engine
  •  » Does piwigo use prepared queries?

Board footer

Powered by FluxBB

github twitter newsletter Donate Piwigo.org © 2002-2024 · Contact