Moodle
  1. Moodle
  2. MDL-16950

Global search has broken title and url stored when using adoDB::mysqli

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.9, 1.9.1, 1.9.2, 1.9.3
    • Fix Version/s: None
    • Component/s: Global search
    • Labels:
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      2210

      Description

      Install Moodle using the mysqli driver. Global search expects the mysql driver, so doc titles and doc urls won't be quoted in SQL queries.
      See bug MDL-13331 for detailed description.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          This still seems to be the case in 1.9.12.

          It's clear this should be resolved, but I'm not sure from your description how this could lead to an SQL injection attack. Could you please elaborate.

          Michael;

          Show
          Michael de Raadt added a comment - This still seems to be the case in 1.9.12. It's clear this should be resolved, but I'm not sure from your description how this could lead to an SQL injection attack. Could you please elaborate. Michael;
          Hide
          François Gannaz added a comment -

          If the DB is MySQL, then the function search_escape_string() defined in lib/search.php always tries to use mysql_real_escape_string(). This is an error, probably due to a confusion between the db type (MySQL) and the PHP driver (ext/mysql). When using ext/mysqli (still with a MySQL DB), the call to an ext/mysql function cannot succeed as no connection was initialized with mysql (it was done with mysqli). Hence, the calls to search_escape_string() have no effect when using mysqli.

          This escaping function search_escape_string() is only used in addDocument(), defined in "search/indexlib.php". Looking at the code, for each $document added to Lucene, it means $document->title and $document->url are not protected before being inserted/updated in the DB. As far as I know, when a SQL parameter should be escaped but isn't, that's an SQL injection possibility. I did not take the time to experiment it myself nor to evaluate the potential damage.

          Show
          François Gannaz added a comment - If the DB is MySQL, then the function search_escape_string() defined in lib/search.php always tries to use mysql_real_escape_string(). This is an error, probably due to a confusion between the db type (MySQL) and the PHP driver (ext/mysql). When using ext/mysqli (still with a MySQL DB), the call to an ext/mysql function cannot succeed as no connection was initialized with mysql (it was done with mysqli). Hence, the calls to search_escape_string() have no effect when using mysqli. This escaping function search_escape_string() is only used in addDocument(), defined in "search/indexlib.php". Looking at the code, for each $document added to Lucene, it means $document->title and $document->url are not protected before being inserted/updated in the DB. As far as I know, when a SQL parameter should be escaped but isn't, that's an SQL injection possibility. I did not take the time to experiment it myself nor to evaluate the potential damage.
          Hide
          Aparup Banerjee added a comment - - edited

          hm, it may be that this isn't a security issue but is definitely broken as its unhandled -
          http://php.net/manual/en/function.mysql-real-escape-string.php :-
          'A MySQL connection is required before using mysql_real_escape_string() otherwise an error of level E_WARNING is generated, and FALSE is returned'

          Show
          Aparup Banerjee added a comment - - edited hm, it may be that this isn't a security issue but is definitely broken as its unhandled - http://php.net/manual/en/function.mysql-real-escape-string.php :- 'A MySQL connection is required before using mysql_real_escape_string() otherwise an error of level E_WARNING is generated, and FALSE is returned'
          Hide
          Aparup Banerjee added a comment -

          Thanks for the report - resolved.

          Show
          Aparup Banerjee added a comment - Thanks for the report - resolved.
          Hide
          Aparup Banerjee added a comment -

          not going to fix this in 1.9 stable as its not a security issue and this is an experimental feature.

          i've checked and this issue doesn't apply to master.

          Show
          Aparup Banerjee added a comment - not going to fix this in 1.9 stable as its not a security issue and this is an experimental feature. i've checked and this issue doesn't apply to master.
          Hide
          François Gannaz added a comment -

          I didn't understand how this ticket could be both "resolved" and "won't fix". So after searching in the VCS, I put here a few details:

          • In Moodle 2.0, it was fixed by commit 1829e015e6e6eb266dfc6e061f642ab022bc9706 (git) by skodak at Mon Jun 2 21:25:40 2008.
            This is a huge commit for MDL-14679 that cannot be reused in 1.9.
          • In Moodle 1.9, the bug is still there. It means choosing the driver "mysqli" will break the global search.
          • I don't understand why "its (sic) not a security issue".
            I didn't test it myself, but it seems to me that, with a sub-query, one could put any data in the global search documents.
            So these unescaped fields could give to anyone who has the ability to create a document (e.g. post in a forum), an access to any SQL data.

          I know 1.9 is superseeded by 2.0 and global search isn't a high priority feature, but wouldn't it be safer to just disable global search when mysqli is used?
          Or, even simpler, add a warning in the description of global search.

          Show
          François Gannaz added a comment - I didn't understand how this ticket could be both "resolved" and "won't fix". So after searching in the VCS, I put here a few details: In Moodle 2.0, it was fixed by commit 1829e015e6e6eb266dfc6e061f642ab022bc9706 (git) by skodak at Mon Jun 2 21:25:40 2008. This is a huge commit for MDL-14679 that cannot be reused in 1.9. In Moodle 1.9, the bug is still there. It means choosing the driver "mysqli" will break the global search. I don't understand why "its (sic) not a security issue". I didn't test it myself, but it seems to me that, with a sub-query, one could put any data in the global search documents. So these unescaped fields could give to anyone who has the ability to create a document (e.g. post in a forum), an access to any SQL data. I know 1.9 is superseeded by 2.0 and global search isn't a high priority feature, but wouldn't it be safer to just disable global search when mysqli is used? Or, even simpler, add a warning in the description of global search.
          Hide
          Aparup Banerjee added a comment - - edited

          Hi François,
          The warnings that you are facing in the MDL-13331 :

          "Access denied for user 'www-data'@'localhost' (using password: NO) in /.../search/lib.php on line 105
          Warning: mysql_real_escape_string() [function.mysql-real-escape-string]: A link to the server could not be established in /.../moodle-peage/search/lib.php on line 105"

          Those warnings are indicating that connection has failed, hence "FALSE is returned" by the function call. No injection of any data infact is happening, you can't put in a sub-query, only broken title and url due to data being set to 'FALSE'.

          It would be nice to fix that function so that the data is cleaned AND returned, the function is obviously not getting the right (or any) db details ('www-data'@'localhost' (using password: NO)).

          Does this patch get rid of your warnings for you? -> https://github.com/nebgor/moodle/compare/MOODLE_19_STABLE...MDL-16950_m19

          Show
          Aparup Banerjee added a comment - - edited Hi François, The warnings that you are facing in the MDL-13331 : "Access denied for user 'www-data'@'localhost' (using password: NO) in /.../search/lib.php on line 105 Warning: mysql_real_escape_string() [function.mysql-real-escape-string] : A link to the server could not be established in /.../moodle-peage/search/lib.php on line 105" Those warnings are indicating that connection has failed, hence "FALSE is returned" by the function call. No injection of any data infact is happening, you can't put in a sub-query, only broken title and url due to data being set to 'FALSE'. It would be nice to fix that function so that the data is cleaned AND returned, the function is obviously not getting the right (or any) db details ('www-data'@'localhost' (using password: NO)). Does this patch get rid of your warnings for you? -> https://github.com/nebgor/moodle/compare/MOODLE_19_STABLE...MDL-16950_m19
          Hide
          François Gannaz added a comment - - edited

          Hi Aparup

          No, your patch does not fix anything. I'm afraid you didn't catch what the bug is.
          Please read my note from 2011-07-30 on this ticket: the problem is that you can't use mysql_real_escape_string() when the PHP driver is mysqli.
          Even if the DB is MySQL, the driver mysql is not used (please take notice of the caps!), that's where the bug is.
          BTW, PHP plan to mark mysql "obsolete" in favor of mysqli and PDO, so making sure Moodle can live without ext/mysql seems increasingly important.

          Now suppose the title of a forum post is "injection', (SELECT GROUP_CONCAT( * ) FROM user LIMIT 1), '', '', '', '') – " (with no surrounding quotes).
          I did not try to find the exact syntax, that's just the concept.
          With mysqli, mysql_real_escape_string() won't do anything, not even a warning unless Moodle is in debug mode. So on indexing, a SQL query will insert the unescaped title.
          The pseudo-code "INSERT INTO ... VALUES ('$title', '$date', ...)" would become "INSERT INTO ('injection', (SELECT GROUP_CONCAT( * ) FROM user LIMIT 1), '', '', '', '') – ', ...)".
          This SQL injection would put all the content of the first row of the table "user" into the global search table. Isn't that a security problem?

          (edited to remove the smileys in the SQL code)

          Show
          François Gannaz added a comment - - edited Hi Aparup No, your patch does not fix anything. I'm afraid you didn't catch what the bug is. Please read my note from 2011-07-30 on this ticket: the problem is that you can't use mysql_real_escape_string() when the PHP driver is mysqli. Even if the DB is MySQL, the driver mysql is not used (please take notice of the caps!), that's where the bug is. BTW, PHP plan to mark mysql "obsolete" in favor of mysqli and PDO, so making sure Moodle can live without ext/mysql seems increasingly important. Now suppose the title of a forum post is "injection', (SELECT GROUP_CONCAT( * ) FROM user LIMIT 1), '', '', '', '') – " (with no surrounding quotes). I did not try to find the exact syntax, that's just the concept. With mysqli, mysql_real_escape_string() won't do anything, not even a warning unless Moodle is in debug mode. So on indexing, a SQL query will insert the unescaped title. The pseudo-code "INSERT INTO ... VALUES ('$title', '$date', ...)" would become "INSERT INTO ('injection', (SELECT GROUP_CONCAT( * ) FROM user LIMIT 1), '', '', '', '') – ', ...)" . This SQL injection would put all the content of the first row of the table "user" into the global search table. Isn't that a security problem? (edited to remove the smileys in the SQL code)
          Hide
          Aparup Banerjee added a comment -

          Hi François,
          Thanks! I read that comment again and its clearer to me now . I'll make a patch with a call to the mysqli_real_escape_string() to fix the data being indexed.

          Although, I still don't understand how with $CFG->dbfamily == 'mysql', a failed call to mysql_real_escape_string() could return a string instead of 'FALSE' as documented.

          Show
          Aparup Banerjee added a comment - Hi François, Thanks! I read that comment again and its clearer to me now . I'll make a patch with a call to the mysqli_real_escape_string() to fix the data being indexed. Although, I still don't understand how with $CFG->dbfamily == 'mysql', a failed call to mysql_real_escape_string() could return a string instead of 'FALSE' as documented.
          Hide
          François Gannaz added a comment -

          Hi Aparup

          You're right, mysql_real_escape_string() would return false with mysqli. As I said, I didn't test the injection, it seemed a logical consequence of the error declared in MDL-1331. But I was wrong. I wish I had read the doc of this function earlier.

          Please consider this bug is closed. But your patch will still be useful to fix MDL-13331. It's the same bug, without the security concern.

          Show
          François Gannaz added a comment - Hi Aparup You're right, mysql_real_escape_string() would return false with mysqli. As I said, I didn't test the injection, it seemed a logical consequence of the error declared in MDL-1331 . But I was wrong. I wish I had read the doc of this function earlier. Please consider this bug is closed. But your patch will still be useful to fix MDL-13331 . It's the same bug, without the security concern.
          Hide
          Aparup Banerjee added a comment -

          Thanks man. closing this. ... looking at MDL-13331

          Show
          Aparup Banerjee added a comment - Thanks man. closing this. ... looking at MDL-13331

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: