Moodle
  1. Moodle
  2. MDL-30480

Dirty magic quotes hack uses hazardous and deprecated casting object

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.14
    • Fix Version/s: 1.9.15
    • Component/s: Libraries
    • Labels:
    • Rank:
      33152

      Description

      Dirty magic quotes hack (MDL-29033) added this code in lib/dmllib.php (lines :1467 and 1648):

      /// Extra protection against SQL injections
      foreach((array)$dataobject as $k=>$v) {
      $dataobject->$k = sql_magic_quotes_hack($v);
      }

      Casting object to array is very hazardous in PHP and deprecated (http://www.php.net/manual/en/language.types.array.php#language.types.array.casting).

      We have patched dmllib to use "get_object_vars" PHP function and not direct casting :

      /// Extra protection against SQL injections
      $dataobject_array = get_object_vars($dataobject);
      foreach($dataobject_array as $k=>$v)

      { $dataobject->$k = sql_magic_quotes_hack($v); }

      MDL-29033 was about Moodle 1.9.14 only. I did not found this code in /lib/dmllib.php file of my Moodle 2.1.2 ...

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          In Moodle 2.x all DML drivers should be reviewed for any array casting operations.

          Show
          Petr Škoda added a comment - In Moodle 2.x all DML drivers should be reviewed for any array casting operations.
          Hide
          Petr Škoda added a comment -

          Hmm, I did not find any "deprecated" word in the linked docs page. I agree we can not change how DML works so late in 1.9.x stable branch, I have used a bit different approach.

          To integrators: the 1.9 patch should be pretty straightforward, I am not sure about the 2.2 patch though, it is up to you...

          Show
          Petr Škoda added a comment - Hmm, I did not find any "deprecated" word in the linked docs page. I agree we can not change how DML works so late in 1.9.x stable branch, I have used a bit different approach. To integrators: the 1.9 patch should be pretty straightforward, I am not sure about the 2.2 patch though, it is up to you...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, no problem about the 1.9 change, after all it fixes something that was somehow "supported" (working) previously.

          But IMO now it's not the time to introduce that change in 2.2:

          • It is a new feature, since 2.0 we have not supported custom objects there.
          • I'm not sure if the way is to allow them or to debug about them and provide fallback conversion using get_object_vars().
          • It is incomplete, the _raw() alternatives surely shoould support them.
          • It lacks specific unit-tests, both stdclass and object with privates/magics should be tested if we really want support to them.

          So I'd propose to create one sister issue of MDL-29894 and try to decide and fix both after branching 22_STABLE.

          Sounds ok?

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, no problem about the 1.9 change, after all it fixes something that was somehow "supported" (working) previously. But IMO now it's not the time to introduce that change in 2.2: It is a new feature, since 2.0 we have not supported custom objects there. I'm not sure if the way is to allow them or to debug about them and provide fallback conversion using get_object_vars(). It is incomplete, the _raw() alternatives surely shoould support them. It lacks specific unit-tests, both stdclass and object with privates/magics should be tested if we really want support to them. So I'd propose to create one sister issue of MDL-29894 and try to decide and fix both after branching 22_STABLE. Sounds ok?
          Hide
          Petr Škoda added a comment -

          perfect for me!

          Show
          Petr Škoda added a comment - perfect for me!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done, MDL-30508 has been created for 2.3 about to decide & complete objects support on insert/update statements.

          Going to integrate the 19_STABLE solution now...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done, MDL-30508 has been created for 2.3 about to decide & complete objects support on insert/update statements. Going to integrate the 19_STABLE solution now...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ah, uhm...

          $dataobject = (object)$dataobject;
          

          or

          $dataobject = (object)$properties;
          

          ???

          Show
          Eloy Lafuente (stronk7) added a comment - ah, uhm... $dataobject = (object)$dataobject; or $dataobject = (object)$properties; ???
          Hide
          Petr Škoda added a comment -

          arrrrggghhh, my refactoring turned against me again, fixing....

          Show
          Petr Škoda added a comment - arrrrggghhh, my refactoring turned against me again, fixing....
          Hide
          Petr Škoda added a comment -

          amended, sorry, it is great to have you as integrator!

          Show
          Petr Škoda added a comment - amended, sorry, it is great to have you as integrator!
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Eh, I think it's one of the first times I detect such type of mess, it's really easy to skip it. And for sure it's the first time I detect it coming from you so:

          • It's really the 1st time it comes from you, aka, you're a great developer, or
          • I've skipped detecting it previously, aka, I'm an awful integrator.

          Thanks, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Eh, I think it's one of the first times I detect such type of mess, it's really easy to skip it. And for sure it's the first time I detect it coming from you so: It's really the 1st time it comes from you, aka, you're a great developer, or I've skipped detecting it previously, aka, I'm an awful integrator. Thanks, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Under my (un)responsibility

          Show
          Eloy Lafuente (stronk7) added a comment - Under my (un)responsibility
          Hide
          Eloy Lafuente (stronk7) added a comment -

          passed!

          Show
          Eloy Lafuente (stronk7) added a comment - passed!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days.

          Thanks for the hard work! Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days. Thanks for the hard work! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: