Moodle
  1. Moodle
  2. MDL-18141

Calculated question formula validation allows syntactical incorrect code

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.9.4
    • Fix Version/s: None
    • Component/s: Questions
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      4993

      Description

      Calculated question formula validation think that formulas like

      {a}

      ?

      {b}

      is valid, which then results in parse errors in eval'ed code.

      Easy workaround for this would be this method of syntax validation ($code is full code you need to execute, not just formula):
      function check_syntax($code) {
      return @eval('return true;' . $code);
      }

      The function will return true if code is syntactical correct and false if it is not.

      To get more accurate error reporting you'll need to use tools like http://netevil.org/blog/2006/nov/parser-and-lexer-generators-for-php
      This may not be easy at first, thought it'll provide you with an ability to create any possible features in you formulas and (eventually) avoid eval at all (this is good, as passing users input to eval may lead to very serious security vulnerabilities). Actually tools will made most complex job for you.

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment -

          $code is full code you need to execute, not just formula):
          I.e. with the

          {a}

          parameters replace by their real values .
          this is not so simple. the actual code replace everything by 1 as a first check.
          Not perfect but better than nothing.
          Working on this could include MDL-7198 proposal as a complete solution.

          Show
          Pierre Pichet added a comment - $code is full code you need to execute, not just formula): I.e. with the {a} parameters replace by their real values . this is not so simple. the actual code replace everything by 1 as a first check. Not perfect but better than nothing. Working on this could include MDL-7198 proposal as a complete solution.
          Hide
          Oleg Sychev added a comment -

          You may replace

          {a}

          with 1 for syntax check, it doesn't matter. Full code means that you'll need to add at least "return " or "$something = " before formula and a semicolon after, to receive valid PHP operator.

          This check should find you any sintactical incorrect things, but doesn't report what they are.

          Show
          Oleg Sychev added a comment - You may replace {a} with 1 for syntax check, it doesn't matter. Full code means that you'll need to add at least "return " or "$something = " before formula and a semicolon after, to receive valid PHP operator. This check should find you any sintactical incorrect things, but doesn't report what they are.
          Hide
          Pierre Pichet added a comment -

          Thanks for the cue.
          I will see how I can use it.

          Show
          Pierre Pichet added a comment - Thanks for the cue. I will see how I can use it.
          Hide
          Pierre Pichet added a comment -

          From Oleg ( MDL-18034)
          "P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done)."

          Effectively there is no checking there.

          I will look at this.

          Show
          Pierre Pichet added a comment - From Oleg ( MDL-18034 ) "P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done)." Effectively there is no checking there. I will look at this.
          Hide
          Pierre Pichet added a comment -

          There is already $question->import_process parameter that could be used when saving options to do the validate process on formulas that is done in edit_calculated_form,
          as it is used to import datassets
          if( isset($question->import_process)&&$question->import_process)

          { $this->import_datasets($question); }

          I will work on this, so get back to your main project...

          Show
          Pierre Pichet added a comment - There is already $question->import_process parameter that could be used when saving options to do the validate process on formulas that is done in edit_calculated_form, as it is used to import datassets if( isset($question->import_process)&&$question->import_process) { $this->import_datasets($question); } I will work on this, so get back to your main project...
          Hide
          Pierre Pichet added a comment -

          Once the filtering is done I don't think that there are problems with "malignant" code because only math functions are allowed.
          function qtype_calculated_find_formula_errors($formula) {
          /// Validates the formula submitted from the question edit page.
          /// Returns false if everything is alright.
          /// Otherwise it constructs an error message
          // Strip away dataset names
          while (ereg('

          {[[:alpha:]][^>}

          <

          {"\']*\\}

          ', $formula, $regs))

          { $formula = str_replace($regs[0], '1', $formula); }

          // Strip away empty space and lowercase it
          $formula = strtolower(str_replace(' ', '', $formula));

          $safeoperatorchar = '-+/%>:^~<?=&|!'; / */
          $operatorornumber = "[$safeoperatorchar.0-9eE]";

          while (ereg("(^|[$safeoperatorchar,(])([a-z0-9_]*)\\(($operatorornumber+(,$operatorornumber+((,$operatorornumber+)+)?)?)?
          )",
          $formula, $regs)) {

          switch ($regs[2]) {
          // Simple parenthesis
          case '':
          if ($regs[4] || strlen($regs[3])==0)

          { return get_string('illegalformulasyntax', 'quiz', $regs[0]); }

          break;

          // Zero argument functions
          case 'pi':
          if ($regs[3])

          { return get_string('functiontakesnoargs', 'quiz', $regs[2]); }

          break;

          // Single argument functions (the most common case)
          case 'abs': case 'acos': case 'acosh': case 'asin': case 'asinh':
          case 'atan': case 'atanh': case 'bindec': case 'ceil': case 'cos':
          case 'cosh': case 'decbin': case 'decoct': case 'deg2rad':
          case 'exp': case 'expm1': case 'floor': case 'is_finite':
          case 'is_infinite': case 'is_nan': case 'log10': case 'log1p':
          case 'octdec': case 'rad2deg': case 'sin': case 'sinh': case 'sqrt':
          case 'tan': case 'tanh':
          if ($regs[4] || empty($regs[3]))

          { return get_string('functiontakesonearg','quiz',$regs[2]); }

          break;

          // Functions that take one or two arguments
          case 'log': case 'round':
          if ($regs[5] || empty($regs[3]))

          { return get_string('functiontakesoneortwoargs','quiz',$regs[2]); }

          break;

          // Functions that must have two arguments
          case 'atan2': case 'fmod': case 'pow':
          if ($regs[5] || empty($regs[4]))

          { return get_string('functiontakestwoargs', 'quiz', $regs[2]); }

          break;

          // Functions that take two or more arguments
          case 'min': case 'max':
          if (empty($regs[4]))

          { return get_string('functiontakesatleasttwo','quiz',$regs[2]); }

          break;

          default:
          return get_string('unsupportedformulafunction','quiz',$regs[2]);
          }

          // Exchange the function call with '1' and then chack for
          // another function call...
          if ($regs[1])

          { // The function call is proceeded by an operator $formula = str_replace($regs[0], $regs[1] . '1', $formula); }

          else

          { // The function call starts the formula $formula = ereg_replace("^$regs[2]\\([^)]*\\)", '1', $formula); }

          }

          if (ereg("[^$safeoperatorchar.0-9eE]+", $formula, $regs))

          { return get_string('illegalformulasyntax', 'quiz', $regs[0]); }

          else

          { // Formula just might be valid return false; }

          }
          However the maths results can be bad but this is detected elsewhere when creating the datasets.

          Show
          Pierre Pichet added a comment - Once the filtering is done I don't think that there are problems with "malignant" code because only math functions are allowed. function qtype_calculated_find_formula_errors($formula) { /// Validates the formula submitted from the question edit page. /// Returns false if everything is alright. /// Otherwise it constructs an error message // Strip away dataset names while (ereg(' {[[:alpha:]][^>} < {"\']*\\} ', $formula, $regs)) { $formula = str_replace($regs[0], '1', $formula); } // Strip away empty space and lowercase it $formula = strtolower(str_replace(' ', '', $formula)); $safeoperatorchar = '-+/ %>:^~<?=&|!'; / */ $operatorornumber = " [$safeoperatorchar.0-9eE] "; while (ereg("(^| [$safeoperatorchar,(] )( [a-z0-9_] *)\\(($operatorornumber+(,$operatorornumber+((,$operatorornumber+)+)?)?)? )", $formula, $regs)) { switch ($regs [2] ) { // Simple parenthesis case '': if ($regs [4] || strlen($regs [3] )==0) { return get_string('illegalformulasyntax', 'quiz', $regs[0]); } break; // Zero argument functions case 'pi': if ($regs [3] ) { return get_string('functiontakesnoargs', 'quiz', $regs[2]); } break; // Single argument functions (the most common case) case 'abs': case 'acos': case 'acosh': case 'asin': case 'asinh': case 'atan': case 'atanh': case 'bindec': case 'ceil': case 'cos': case 'cosh': case 'decbin': case 'decoct': case 'deg2rad': case 'exp': case 'expm1': case 'floor': case 'is_finite': case 'is_infinite': case 'is_nan': case 'log10': case 'log1p': case 'octdec': case 'rad2deg': case 'sin': case 'sinh': case 'sqrt': case 'tan': case 'tanh': if ($regs [4] || empty($regs [3] )) { return get_string('functiontakesonearg','quiz',$regs[2]); } break; // Functions that take one or two arguments case 'log': case 'round': if ($regs [5] || empty($regs [3] )) { return get_string('functiontakesoneortwoargs','quiz',$regs[2]); } break; // Functions that must have two arguments case 'atan2': case 'fmod': case 'pow': if ($regs [5] || empty($regs [4] )) { return get_string('functiontakestwoargs', 'quiz', $regs[2]); } break; // Functions that take two or more arguments case 'min': case 'max': if (empty($regs [4] )) { return get_string('functiontakesatleasttwo','quiz',$regs[2]); } break; default: return get_string('unsupportedformulafunction','quiz',$regs [2] ); } // Exchange the function call with '1' and then chack for // another function call... if ($regs [1] ) { // The function call is proceeded by an operator $formula = str_replace($regs[0], $regs[1] . '1', $formula); } else { // The function call starts the formula $formula = ereg_replace("^$regs[2]\\([^)]*\\)", '1', $formula); } } if (ereg(" [^$safeoperatorchar.0-9eE] +", $formula, $regs)) { return get_string('illegalformulasyntax', 'quiz', $regs[0]); } else { // Formula just might be valid return false; } } However the maths results can be bad but this is detected elsewhere when creating the datasets.
          Hide
          Oleg Sychev added a comment -

          "Once the filtering is done I don't think that there are problems with "malignant" code because only math functions are allowed. " - are you sure that not malignant things can be done via modifying values of variables?

          Show
          Oleg Sychev added a comment - "Once the filtering is done I don't think that there are problems with "malignant" code because only math functions are allowed. " - are you sure that not malignant things can be done via modifying values of variables?
          Hide
          Oleg Sychev added a comment -

          I suggests adding syntactical check mentioned in description (not necessary as separate function) to qtype_calculated_find_formula_errors. It'll catch any additional errors, that are not identifyed by now.

          Show
          Oleg Sychev added a comment - I suggests adding syntactical check mentioned in description (not necessary as separate function) to qtype_calculated_find_formula_errors. It'll catch any additional errors, that are not identifyed by now.
          Hide
          Pierre Pichet added a comment -

          On import however, additional checking is needed as students could be allowed to import questions.
          I put this on my to-do list...

          Show
          Pierre Pichet added a comment - On import however, additional checking is needed as students could be allowed to import questions. I put this on my to-do list...
          Hide
          Pierre Pichet added a comment -
          Show
          Pierre Pichet added a comment - See http://moodle.org/mod/forum/discuss.php?d=122222 for a related problem.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          lqjjLKA0p6

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
          Hide
          Oleg Sychev added a comment -

          Pierre - did you do anything about this problem in new Moodle version to have it fixed?

          I don't beleive it is fixed, but have no time to re-test for now...

          Show
          Oleg Sychev added a comment - Pierre - did you do anything about this problem in new Moodle version to have it fixed? I don't beleive it is fixed, but have no time to re-test for now...
          Hide
          Pierre Pichet added a comment -

          This has been fixed in calculated/question.php i.e. 2.1 when Tim build this file

          /**
               * Evaluate an expression after the variable values have been substituted.
               * @param string $expression the expression. A PHP expression with placeholders
               *      like {a} for where the variables need to go.
               * @return float the computed result.
               */
              protected function calculate_raw($expression) {
                  // This validation trick from http://php.net/manual/en/function.eval.php
                  if (!@eval('return true; $result = ' . $expression . ';')) {
                      throw new moodle_exception('illegalformulasyntax', 'qtype_calculated', '', $expression);
                  }
                  return eval('return ' . $expression . ';');
              }
          

          However I have to check if this is applied when editing the questions.

          Show
          Pierre Pichet added a comment - This has been fixed in calculated/question.php i.e. 2.1 when Tim build this file /** * Evaluate an expression after the variable values have been substituted. * @param string $expression the expression. A PHP expression with placeholders * like {a} for where the variables need to go. * @return float the computed result. */ protected function calculate_raw($expression) { // This validation trick from http://php.net/manual/en/function.eval.php if (!@eval('return true; $result = ' . $expression . ';')) { throw new moodle_exception('illegalformulasyntax', 'qtype_calculated', '', $expression); } return eval('return ' . $expression . ';'); } However I have to check if this is applied when editing the questions.
          Hide
          Pierre Pichet added a comment -

          Micheal,
          I will get through all my "not apparently active" bugs in the next 2 weeks.
          Thanks for the remainder.
          This was on my todo list for the semester so its promote at an higher level

          Show
          Pierre Pichet added a comment - Micheal, I will get through all my "not apparently active" bugs in the next 2 weeks. Thanks for the remainder. This was on my todo list for the semester so its promote at an higher level
          Hide
          Pierre Pichet added a comment -

          Testing on master, this has not been solved completely.
          i.e the error is not detected when editing the question.
          the

          {a}

          ?

          {b}

          :

          {c}

          structure is allowed in formulas but not correctly tested.

          Tim has commented somewhere that he plans to implement for calculated the evalmath.class.php library set
          for the new OU numericals questiontypes.

          So this bug should remain active

          Show
          Pierre Pichet added a comment - Testing on master, this has not been solved completely. i.e the error is not detected when editing the question. the {a} ? {b} : {c} structure is allowed in formulas but not correctly tested. Tim has commented somewhere that he plans to implement for calculated the evalmath.class.php library set for the new OU numericals questiontypes. So this bug should remain active
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          4d6f6f646c6521

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; 4d6f6f646c6521
          Hide
          Oleg Sychev added a comment -

          Reproduced on qa.moodle.net

          {a}

          ?

          {b}

          as an answer gives parse errors when generating sets and saving the question.

          Parse error: syntax error, unexpected ';' in /html/question/type/calculated/questiontype.php(1259) : eval()'d code on line 1

          Parse error: syntax error, unexpected ';' in /html/question/type/calculated/questiontype.php(1073) : eval()'d code on line 1

          Notice: Undefined variable: ansvalue in /html/question/type/calculated/questiontype.php on line 1074

          Show
          Oleg Sychev added a comment - Reproduced on qa.moodle.net {a} ? {b} as an answer gives parse errors when generating sets and saving the question. Parse error: syntax error, unexpected ';' in /html/question/type/calculated/questiontype.php(1259) : eval()'d code on line 1 Parse error: syntax error, unexpected ';' in /html/question/type/calculated/questiontype.php(1073) : eval()'d code on line 1 Notice: Undefined variable: ansvalue in /html/question/type/calculated/questiontype.php on line 1074

            People

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

              Dates

              • Created:
                Updated: