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:

      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 ...

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Petr Skoda added a comment -

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

            Show
            Petr Skoda added a comment - In Moodle 2.x all DML drivers should be reviewed for any array casting operations.
            Hide
            Petr Skoda 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 Skoda 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 Skoda added a comment -

            perfect for me!

            Show
            Petr Skoda 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 Skoda added a comment -

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

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

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

            Show
            Petr Skoda 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: