classification
Title: ipaddress netmask/hostmask parsing bugs
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: eric.smith, jongfoster, ncoghlan, pmoody, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2013-08-22 00:38 by jongfoster, last changed 2014-02-08 13:47 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
ipaddress_masks_v1.patch jongfoster, 2013-08-22 00:38 review
issue18805_standard_library.diff ncoghlan, 2014-02-08 13:26 Patch as committed to 3.3 review
Messages (11)
msg195846 - (view) Author: Jon Foster (jongfoster) * Date: 2013-08-22 00:38
Short version:
Validation of netmasks and hostmasks in IPv4Network is wrong: it rejects many
valid netmasks, it accepts many invalid netmasks and hostmasks, and it sometimes
throws the wrong exception.  Patch attached.


Long version:


Wrongly rejecting hostmasks
---------------------------

Creating IPv4Network objects using a hostmask only works for 3 of the 31 possible hostmasks.
It works fine for /8, /16, and /24 networks, but anything else fails.  E.g. first let's
calculate the hostmask for a /21 network:

  >>> from ipaddress import IPv4Network
  >>> IPv4Network('0.0.0.0/21').hostmask
  IPv4Address('0.0.7.255')

Then try using it:
  >> IPv4Network('0.0.0.0/0.0.7.255')
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
    File "c:\Python33\lib\ipaddress.py", line 1443, in __init__
      % addr[1])
  ipaddress.NetmaskValueError: '0.0.7.255' is not a valid netmask

The problem is that there is a list of "_valid_mask_octets".  Although the values listed
are correct for netmasks, they are wrong for host masks.  In binary, a netmask has 1s
in the most significant <prefixlen> bits and 0s everywhere else; a hostmask has 0s
in the most significant <prefixlen> bits and 1s everywhere else.  So netmasks have
octet values 0b11111111, 0b11111110, 0b11111100, etc, whereas hostmasks have
octet values 0b11111111, 0b01111111, 0b00111111, etc.


Wrongly accepting hostmasks
---------------------------

Once the individual octets have been validated, the hostmask validation just checks
the first octet is less than the last octet.  This accepts values like "0.255.0.255",
which is not a valid hostmask.  The IPv4Network object then has wierd nonsensical
values.


Wrongly accepting netmasks
---------------------------

Once the individual octets have been validated, the netmask validation just checks
each octet is no greater than the one before.  This accepts values like "254.192.128.0",
which is not a valid netmask.  The IPv4Network object then has wierd nonsensical
values.


Inconsistent parsing
--------------------

The existing validation code includes its own parsing code.  If the netmask/hostmask
passes that vaildation, it then goes into _ip_int_from_string() to be parsed and
used.  _ip_int_from_string() checks things that aren't caught by the validation
code, leading to AddressValueError being thrown when NetmaskValueError was expected:

  >>> IPv4Network('1.2.0.0/0.0.0255.255')
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
    File "c:\Python33\lib\ipaddress.py", line 1440, in __init__
      self._ip_int_from_string(addr[1]) ^ self._ALL_ONES)
    File "c:\Python33\lib\ipaddress.py", line 1055, in _ip_int_from_string
      raise AddressValueError("%s in %r" % (exc, ip_str)) from None
  ipaddress.AddressValueError: At most 3 characters permitted in '0255' in '0.0.0255.255'

The correct fix for this one is obviously to use the same parser in all the places
we parse the netmask/hostmask.


The patch
---------

I'm attaching a patch, with tests, that fixes these issues.  Reusing the existing
_ip_int_from_string() function to parse the netmask/hostmask simplified the
validation code a lot.

My hope is that this patch is suitable for a backport to 3.3, as well as trunk.
msg195854 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2013-08-22 01:23
I haven't had time to review this yet, but at least several of these, if true, are definitely bugs that should be backported. "254.192.128.0" as a netmask? Descending octets as the condition? Yikes.
msg195856 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-08-22 02:49
Right, I don't see any issues with backporting these fixes. One of the
reasons for the PEP 411 provisional status of the API was to ensure
there were no questions about what should be backported.
msg198901 - (view) Author: pmoody (pmoody) * (Python committer) Date: 2013-10-03 17:05
I've got a patch from pmarks that I've applied to ipaddr and the google code version of ipaddress-py. I'll get it applied to the hg ipaddress.
msg209842 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-01-31 22:35
bump?
msg210614 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-08 11:11
I've found the patch Peter mentions above (git diff HEAD^ on ipaddress-py), and will apply it to 3.3 and default.
msg210616 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-08 11:16
Oh, that's annoying - it doesn't apply cleanly to the stdlib version, even after adjusting the filenames :P
msg210625 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-08 13:05
OK, what I have done is translated the new *tests* from the upstream patch to the standard library tests, and then used the upstream functional patch as a guide for fixing the standard library version.

I'll upload the final patch here, but I won't wait for review before committing it (I'd like to ensure it makes the 3.4rc1 tag tomorrow).
msg210631 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-08 13:21
New changeset ca5ea7c24370 by Nick Coghlan in branch '3.3':
Issue #18805: better netmask validation in ipaddress
http://hg.python.org/cpython/rev/ca5ea7c24370

New changeset 90dfb26869a7 by Nick Coghlan in branch 'default':
Merge fix for #18805 from 3.3
http://hg.python.org/cpython/rev/90dfb26869a7
msg210632 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-08 13:26
Attaching my patch for reference (although I just realised I need to tweak the NEWS entry slightly to indicate that valid prefixes that were previously rejected are now accepted).

Sorry we weren't able to make use of your patch Jon, but thanks for the detailed description of the parsing problems!
msg210638 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-08 13:40
New changeset 19ca11099f07 by Nick Coghlan in branch '3.3':
Fix #18805 NEWS entry
http://hg.python.org/cpython/rev/19ca11099f07

New changeset e0b1c937e57c by Nick Coghlan in branch 'default':
Merge #18805 NEWS fix from 3.3
http://hg.python.org/cpython/rev/e0b1c937e57c
History
Date User Action Args
2014-02-08 13:47:12ncoghlansetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2014-02-08 13:40:37python-devsetmessages: + msg210638
2014-02-08 13:26:41ncoghlansetfiles: + issue18805_standard_library.diff

messages: + msg210632
2014-02-08 13:21:13python-devsetnosy: + python-dev
messages: + msg210631
2014-02-08 13:05:42ncoghlansetmessages: + msg210625
2014-02-08 11:16:31ncoghlansetmessages: + msg210616
2014-02-08 11:11:36ncoghlansetassignee: ncoghlan
messages: + msg210614
2014-01-31 22:35:58yselivanovsetnosy: + yselivanov
messages: + msg209842
2013-10-03 17:05:03pmoodysetmessages: + msg198901
2013-08-22 02:49:48ncoghlansetmessages: + msg195856
2013-08-22 01:23:23eric.smithsetnosy: + eric.smith

messages: + msg195854
stage: patch review
2013-08-22 00:38:38jongfostercreate