[12.x] synchronisation, quote not supported in album name

[celogeek]

I'm try to create an album with the name

Vacances d'été à la montagne

And it seems the quote breaks the request now. I have some album with quote created with piwigo 11.
I'm using mysqli.

Here the error I see on the page:

Warning:  [mysql error 1064] You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'été à la montagne','0','16.0','true','true','private','689')' at line 3

INSERT  INTO `piwigo_categories`
  (`name`,`rank`,`global_rank`,`commentable`,`visible`,`status`,`id_uppercat`)
  VALUES('Vacances d'été à la montagne','0','16.0','true','true','private','689') in /app/include/dblayer/functions_mysqli.inc.php on line 847



Warning:  [mysql error 1064] You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ')
    AND status = 'private'' at line 3

SELECT id
  FROM piwigo_categories
  WHERE id IN ()
    AND status = 'private'
; in /app/include/dblayer/functions_mysqli.inc.php on line 847


Fatal error: Uncaught Error: Call to a member function fetch_assoc() on bool in /app/include/dblayer/functions_mysqli.inc.php:908 Stack trace:
#0 /app/admin/include/functions.php(2879): query2array()
#1 /app/admin/include/functions.php(1583): add_permission_on_category()
#2 /app/admin/cat_list.php(163): create_virtual_category()
#3 /app/admin.php(314): include('...')
#4 {main} thrown in /app/include/dblayer/functions_mysqli.inc.php on line 908

As far as I can see, the parameters that come from the browser are injected like that in the query with no protection.
I think the best would be to use binding when doing query, this is dangerous !

[tomchiverton]

Same here still, Fedora 35

Very frustrating, as no errors, just the green "created" bar points at id 0

Piwigo 12.2.0
Operating system: Linux
PHP: 8.0.16
MySQL: 10.5.13-MariaDB
Graphics Library: External ImageMagick 6.9.12-37

[klaoun]

Hi @celogeek
try changing your album name to "Vacance"
it will give a search lead, if this is concluded there will be a code formula to add in localfiles editor hence the regex.
thanks

[celogeek]

Well, this works:

Vacances d\'été à la montagne

It really looks like SQL injections is possible.

[tomchiverton]

Oh dear.

Unless I miss something, cat_list.php passed the POST of the name direct. Maybe the parent id too

elseif (isset($_POST['submitAdd']))
{
  $output_create = create_virtual_category(
    $_POST['virtual_name'],
    @$_GET['parent_id']
  );

functions.php then does totally the wrong thing.

 WHERE id_uppercat '.(empty($parent_id) ? 'IS NULL' : '= '.$parent_id).'

multiple other occurrences, simple SQL injection.

I've contacted Piwigo via the page on their web site. I can't see a way to tag this issue as being sensitive and hide it.

At least it requires authorised user (I hope, not checked)...

[klaoun]

hello
try in "localfiles editor"
for example ==> $conf['sync_chars_regex'] ='/^[a-zA-Z0-9-_.é ]+$/;

[tomchiverton]

@MatthieuLP @plegall #security

[tomchiverton]

It might be able to "fix" this with addslashes in the right place... Seems to be used very sparingly in the current code however.

Would the project be open to a patch that added this ?

[plegall]

@tomchiverton in admin/cat_list.php did you see this line:

check_input_parameter('parent_id', $_GET, false, PATTERN_ID);

Maybe you did, but certainly you don't know what happens if $_GET['parent_id'] contains anything else than digits :-) It would immediately fail. It would never go into the SQL query.

Security matters to us. It's not because it's not done "the way you expect it to be" that it's not done another way :-) So unless you have a case where a faulty $_GET['parent_id'] breaks a SQL query, we can consider this as a non-problem.

[plegall]

I've changed the title of this issue. It's not an SQL injection per say. It's only a “problem" when adding albums by synchronisation (based on filesystem). I personally think it's a really bad idea to use quotes, spaces, mixing small and capital letters... in file names. You can do it. Your filesystem will let you do it. It remains a bad idea. Who has tried to use "/" character in a filename on Linux?

In Piwigo, when you use the synchronisation system to add albums/photos, there is the "physical" (files in the filesystem) layer and the "logical" (database) layer. You can prefectly set the title of your album to "what'ever You Want 专辑" once added in your Piwigo, because it's only in the database, not on the filesystem.

[celogeek]

I use the interface in Admin > Albums > Manage
I click on the button "Create", then enter this title.
I've got an error.

When I add a backslash, it works.
So nothing related to the local system, it is only on the Category in the database.

[celogeek]

By the way I've made a tools to sync a full struct with album name, and I temporary add the backslash on the parameters Category Name, until the SQL issue is solve.

We can see the SQL with the string replaced directly in the error message. So it is a SQL injection issue.

Usually it is highly recommanded to use the binding feature of the SQL drivers to avoid any possible sql injection.

What if I use a name like this directly in the interface:

Vacances'; DROP DATABASE photos; '-- '

I won't try, but I'm sure it is possible to drop a table or the database just by trying a name like that.

[celogeek]

INSERT  INTO `piwigo_categories`
  (`name`,`rank`,`global_rank`,`commentable`,`visible`,`status`,`id_uppercat`)
  VALUES('Vacances d'été à la montagne'

We can clearly see here what it try to do (just build the SQL string with the input from the browser)

[tomchiverton]

I created a test table, then tried to create a new album called

Vacances'; DROP table newTable; '-- '

but the new table was not removed

Some command line fiddling e.g. ...virtual_name=Vacances'; DROP table newTable; '... didn't yield an exploit either.

[tomchiverton]

single_insert(CATEGORIES_TABLE, $insert); appears to protect the virtual_name parameter but I'm not sure how or where this happens to test it fully

[tomchiverton]

We can see the SQL with the string replaced directly in the error message. So it is a SQL injection issue.

How did you do that ? Here when I try to create an album in the admin with a ' in the name, it just refreshes the page and has a green banner saying "Album added Edit album" but the edit link is ..../admin.php?page=album-0

My versions are above.

[plegall]

I use the interface in Admin > Albums > Manage I click on the button "Create", then enter this title. I've got an error.

No problem for me.

The difference I see between you and me is that you're on PHP 8 while I'm on PHP 7. We're going to make some test specifically on PHP 8. Could be related to magic_quotes being removed from PHP 8.

[MatthieuLP]

After testing on PHP8 environment, we find that creating albums with a quote breaks the page.
We'll work on a fix for PHP8 users.