Moodle

64 bit support for subnet mask of type 1 in quiz

Details

  • Type: Bug Bug
  • Status: Closed 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

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: