Moodle
  1. Moodle
  2. MDL-15655

64 bit support for subnet mask of type 1 in quiz

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.1
    • Fix Version/s: 1.7.6, 1.8.7, 1.9.3
    • Component/s: Quiz
    • Labels:
      None
    • Environment:
      64 bit CPU architecture - Linux Intel
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      26390

      Description

      When using quiz restriction by ip address, using range type 1: xxx.xxx.xxx.xxx/xx, on 64 bit CPU, result is unexpected. Add the '+' line :

      +++ lib/moodlelib.php
      @@ -7029,6 +7029,7 @@
      if (strpos($subnet, '/') !== false)

      { /// type 1 list($ip, $mask) = explode('/', $subnet); $mask = 0xffffffff << (32 - $mask); + $mask = substr($mask,0,8); // Compatib. cpu 64 bit $found = ((ip2long($addr) & $mask) == (ip2long($ip) & $mask)); }

      else if (strpos($subnet, '-') !== false) {/// type 3
      $subnetparts = explode('.', $subnet);

        Activity

        Hide
        Tim Hunt added a comment -

        Thank you very much for reporting this issues, especially since you included a suggested fix. I'm sorry it took me so long to get around to looking at it.

        Before applying your fix, I would like to understand it better. At the moment I am confused. All we are doing with $mask is &ing it with some 32 bit integers, so why should it matter if there are some extra bits set in $mask?

        Would it be possible for you to try running the unit tests for this function on your 64-bit CPU? You just need to go to the URL .../admin/report/simpletest/index.php?&path=lib/simpletest/testmoodlelib.php on your moodle server. Do all those tests pass, both with and without your change?

        One think about your fix that I don't like is the way it uses string manipulation on $mask. Wouldn't it be better to do $mask = $mask & 0xffffffff; instead of your line of code?

        Show
        Tim Hunt added a comment - Thank you very much for reporting this issues, especially since you included a suggested fix. I'm sorry it took me so long to get around to looking at it. Before applying your fix, I would like to understand it better. At the moment I am confused. All we are doing with $mask is &ing it with some 32 bit integers, so why should it matter if there are some extra bits set in $mask? Would it be possible for you to try running the unit tests for this function on your 64-bit CPU? You just need to go to the URL .../admin/report/simpletest/index.php?&path=lib/simpletest/testmoodlelib.php on your moodle server. Do all those tests pass, both with and without your change? One think about your fix that I don't like is the way it uses string manipulation on $mask. Wouldn't it be better to do $mask = $mask & 0xffffffff; instead of your line of code?
        Hide
        Daniel Pouliot added a comment -

        Full ip address restriction in quiz
        ex.: quiz --> Require network address: 192.168.10.10

        It will be interpreted as type 2 and will accept 192.168.10.10 and so 192.168.10.100 to 192.168.10.109

        So we asked our teachers to use type 1 by adding a '/' at the end. The thing I don't remember is why we didn't ask them to add '/32'. Did it came from the doc? Note that the problem was treated on moodle 1.6.3 in 2007.

        So, the difference on 64 bit appears when ending with '/'
        ex.: quiz --> Require network address: 192.168.10.10/

        This will work well on 32 bit cpu but will not work on 64 bit because of this instruction:
        $mask = 0xffffffff << (32 - $mask);

        On 32 bit, no shift will be done and $mask will get 0xFFFFFFFF
        On 64 bit, a 32 bit shift will be apply giving the value of 0xFFFFFFFF00000000 to $mask

        Conclusion:
        '/' on 32 bit = '/32' and on 64 bit = '/0'.
        '/32' on 32 and 64 bit = '/32'.

        After many tests and research, I suggest to ignore this patch and add '/32' for full ip addresses. The doc need to be change or the code for type 2 when using full ip addresses need change. In this last case, may be using '*' for partial addresses would be a good solution.

        Show
        Daniel Pouliot added a comment - Full ip address restriction in quiz ex.: quiz --> Require network address: 192.168.10.10 It will be interpreted as type 2 and will accept 192.168.10.10 and so 192.168.10.100 to 192.168.10.109 So we asked our teachers to use type 1 by adding a '/' at the end. The thing I don't remember is why we didn't ask them to add '/32'. Did it came from the doc? Note that the problem was treated on moodle 1.6.3 in 2007. So, the difference on 64 bit appears when ending with '/' ex.: quiz --> Require network address: 192.168.10.10/ This will work well on 32 bit cpu but will not work on 64 bit because of this instruction: $mask = 0xffffffff << (32 - $mask); On 32 bit, no shift will be done and $mask will get 0xFFFFFFFF On 64 bit, a 32 bit shift will be apply giving the value of 0xFFFFFFFF00000000 to $mask Conclusion: '/' on 32 bit = '/32' and on 64 bit = '/0'. '/32' on 32 and 64 bit = '/32'. After many tests and research, I suggest to ignore this patch and add '/32' for full ip addresses. The doc need to be change or the code for type 2 when using full ip addresses need change. In this last case, may be using '*' for partial addresses would be a good solution.
        Hide
        Tim Hunt added a comment -

        Thank you for the clear analysis and explanation. I think I understand now.

        I did not realise that / was valid syntax. I guess that the best thing is to explicitly check for this case, and treat it as /32.

        Also, it was a bug if 192.168.10.10 matched 192.168.10.100. Type 2 should only apply to entire parts of the address, it should not be a general sub-string mechanism.

        I have added unit tests for these cases, and then changed the code so that they all pass.

        Hopefully that fixes the problem, however I don't have an easy way to test on a 64 bit architecture, so it would be good if you could. Thanks.

        Show
        Tim Hunt added a comment - Thank you for the clear analysis and explanation. I think I understand now. I did not realise that / was valid syntax. I guess that the best thing is to explicitly check for this case, and treat it as /32. Also, it was a bug if 192.168.10.10 matched 192.168.10.100. Type 2 should only apply to entire parts of the address, it should not be a general sub-string mechanism. I have added unit tests for these cases, and then changed the code so that they all pass. Hopefully that fixes the problem, however I don't have an easy way to test on a 64 bit architecture, so it would be good if you could. Thanks.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: