[Git][NTPsec/ntpsec][master] Ripped packet validation code out of getresponse into its own method.
Ian Bruene
gitlab at mg.gitlab.com
Fri Aug 18 01:07:57 UTC 2017
Ian Bruene pushed to branch master at NTPsec / ntpsec
Commits:
c835171b by Ian Bruene at 2017-08-17T19:55:39-05:00
Ripped packet validation code out of getresponse into its own method.
Added tests for new method __validate_packet().
Removed redundant validataion test.
- - - - -
2 changed files:
- pylib/packet.py
- tests/pylib/test_packet.py
Changes:
=====================================
pylib/packet.py
=====================================
--- a/pylib/packet.py
+++ b/pylib/packet.py
@@ -1073,69 +1073,10 @@ class ControlSession:
except struct.error:
raise ControlException(SERR_UNSPEC)
- if ((rpkt.version() > ntp.magic.NTP_VERSION or
- rpkt.version() < ntp.magic.NTP_OLDVERSION)):
- warndbg("Fragment received with version %d\n"
- % rpkt.version(), 1)
+ # Validate that packet header is sane, and the correct type
+ valid = self.__validate_packet(rpkt, rawdata, opcode, associd)
+ if valid is False:
continue
- if rpkt.mode() != ntp.magic.MODE_CONTROL:
- warndbg("Fragment received with mode %d\n" % rpkt.mode(), 1)
- continue
- if not rpkt.is_response():
- warndbg("Received request, wanted response\n", 1)
- # continue
-
- # Check opcode and sequence number for a match.
- # Could be old data getting to us.
- # =======
- # These had the continues inside an if debug block. Probably
- # shouldn't have been there, but if there is a problem move
- # them back.
- if rpkt.sequence != self.sequence:
- warndbg("Received sequence number %d, wanted %d\n" %
- (rpkt.sequence, self.sequence), 1)
- continue
- if rpkt.opcode() != opcode:
- warndbg("Received opcode %d, wanted %d\n" %
- (rpkt.opcode(), opcode), 1)
- continue
-
- # Check the error code. If non-zero, return it.
- if rpkt.is_error():
- if rpkt.more():
- warn("Error %d received on non-final fragment\n"
- % rpkt.errcode())
- self.keyid = self.passwd = None
- raise ControlException(
- SERR_SERVER
- % ControlSession.server_errors[rpkt.errcode()],
- rpkt.errcode())
-
- # Check the association ID to make sure it matches what we expect
- if rpkt.associd != associd:
- warn("Association ID %d doesn't match expected %d\n"
- % (rpkt.associd, associd))
-
- # validate received payload size is padded to next 32-bit
- # boundary and no smaller than claimed by rpkt.count
- if len(rawdata) & 0x3:
- warn("Response fragment not padded, size = %d\n"
- % len(rawdata))
- continue
-
- shouldbesize = (ControlPacket.HEADER_LEN + rpkt.count + 3) & ~3
- if len(rawdata) < shouldbesize:
- warn("Response fragment claims %u octets payload, "
- "above %d received\n"
- % (rpkt.count, len(rawdata) - ControlPacket.HEADER_LEN))
- raise ControlException(SERR_INCOMPLETE)
-
- if rpkt.count > (len(rawdata) - ControlPacket.HEADER_LEN):
- warn("Received count of %u octets, data in "
- " fragment is %ld\n"
- % (rpkt.count,
- len(rawdata) - ControlPacket.HEADER_LEN))
- continue
# Someday, perhaps, check authentication here
@@ -1219,6 +1160,70 @@ class ControlSession:
return None
break
+ def __validate_packet(self, rpkt, rawdata, opcode, associd):
+ # TODO: refactor to simplify while retaining semantic info
+ if self.logfp is not None:
+ warn = self.logfp.write
+ else:
+ warn = (lambda x: x)
+ warndbg = (lambda txt, th: ntp.util.dolog(self.logfp, txt,
+ self.debug, th))
+
+ if ((rpkt.version() > ntp.magic.NTP_VERSION or
+ rpkt.version() < ntp.magic.NTP_OLDVERSION)):
+ warndbg("Fragment received with version %d\n"
+ % rpkt.version(), 1)
+ return False
+ if rpkt.mode() != ntp.magic.MODE_CONTROL:
+ warndbg("Fragment received with mode %d\n" % rpkt.mode(), 1)
+ return False
+ if not rpkt.is_response():
+ warndbg("Received request, wanted response\n", 1)
+ return False
+
+ # Check opcode and sequence number for a match.
+ # Could be old data getting to us.
+ if rpkt.sequence != self.sequence:
+ warndbg("Received sequence number %d, wanted %d\n" %
+ (rpkt.sequence, self.sequence), 1)
+ return False
+ if rpkt.opcode() != opcode:
+ warndbg("Received opcode %d, wanted %d\n" %
+ (rpkt.opcode(), opcode), 1)
+ return False
+
+ # Check the error code. If non-zero, return it.
+ if rpkt.is_error():
+ if rpkt.more():
+ warn("Error %d received on non-final fragment\n"
+ % rpkt.errcode())
+ self.keyid = self.passwd = None
+ raise ControlException(
+ SERR_SERVER
+ % ControlSession.server_errors[rpkt.errcode()],
+ rpkt.errcode())
+
+ # Check the association ID to make sure it matches what we expect
+ if rpkt.associd != associd:
+ warn("Association ID %d doesn't match expected %d\n"
+ % (rpkt.associd, associd))
+
+ # validate received payload size is padded to next 32-bit
+ # boundary and no smaller than claimed by rpkt.count
+ if len(rawdata) & 0x3:
+ warn("Response fragment not padded, size = %d\n"
+ % len(rawdata))
+ return False
+
+ shouldbesize = (ControlPacket.HEADER_LEN + rpkt.count + 3) & ~3
+ if len(rawdata) < shouldbesize:
+ warn("Response fragment claims %u octets payload, "
+ "above %d received\n"
+ % (rpkt.count, len(rawdata) - ControlPacket.HEADER_LEN))
+ raise ControlException(SERR_INCOMPLETE)
+
+ return True
+
def doquery(self, opcode, associd=0, qdata="", auth=False):
"send a request and save the response"
if not self.havehost():
=====================================
tests/pylib/test_packet.py
=====================================
--- a/tests/pylib/test_packet.py
+++ b/tests/pylib/test_packet.py
@@ -492,6 +492,132 @@ class TestControlSession(unittest.TestCase):
finally:
ntp.packet.select = select
+ def test___validate_packet(self):
+ logjig = FileJig()
+ # Init
+ cls = self.target()
+ cls.debug = 5
+ cls.logfp = logjig
+ # Test good packet, empty data
+ raw = "\x0E\x81\x00\x00\x00\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ True)
+ self.assertEqual(logjig.data, [])
+ # Test good packet, with data
+ logjig.data = []
+ raw = "\x0E\xA1\x00\x01\x00\x02\x00\x03\x00\x00\x00\x09" \
+ "foo=4223,\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ cls.sequence = 1
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 3),
+ True)
+ self.assertEqual(logjig.data, [])
+ # Test bad packet, bad version
+ # Need to fix version logic 0x46 can be ver == 5, or 0
+ cls.sequence = 0
+ logjig.data = []
+ raw = "\x46\x81\x00\x00\x00\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ False)
+ self.assertEqual(logjig.data, ["Fragment received with version 0\n"])
+ # Test bad packet, bad mode
+ logjig.data = []
+ raw = "\x0D\x81\x00\x00\x00\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ False)
+ self.assertEqual(logjig.data, ["Fragment received with mode 5\n"])
+ # Test bad packet, bad response bit
+ logjig.data = []
+ raw = "\x0E\x01\x00\x00\x00\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ False)
+ self.assertEqual(logjig.data, ["Received request, wanted response\n"])
+ # Test bad packet, bad sequence
+ logjig.data = []
+ raw = "\x0E\x81\x00\x01\x00\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ False)
+ self.assertEqual(logjig.data,
+ ["Received sequence number 1, wanted 0\n"])
+ # Test bad packet, bad opcode
+ logjig.data = []
+ raw = "\x0E\x80\x00\x00\x00\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ False)
+ self.assertEqual(logjig.data,
+ ["Received opcode 0, wanted 1\n"])
+ # Test error packet
+ logjig.data = []
+ raw = "\x0E\xC1\x00\x00" + \
+ chr(ntp.control.CERR_BADVALUE) + \
+ "\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ try:
+ cls._ControlSession__validate_packet(pkt, raw, 1, 2)
+ self.assertEqual(False, True) # it should have errored here
+ except ntp.packet.ControlException as e:
+ self.assertEqual(e.errorcode, ntp.control.CERR_BADVALUE)
+ self.assertEqual(logjig.data, [])
+ # Test error packet, with more bit
+ logjig.data = []
+ errcs = chr(ntp.control.CERR_BADVALUE)
+ raw = "\x0E\xE1\x00\x00" + errcs + "\x03\x00\x02\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ try:
+ cls._ControlSession__validate_packet(pkt, raw, 1, 2)
+ self.assertEqual(False, True) # it should have errored here
+ except ntp.packet.ControlException as e:
+ self.assertEqual(e.errorcode, ntp.control.CERR_BADVALUE)
+ errstr = "Error " + str(ntp.control.CERR_BADVALUE) + \
+ " received on non-final fragment\n"
+ self.assertEqual(logjig.data, [errstr])
+ # Test ok-ish packet, bad associd
+ logjig.data = []
+ raw = "\x0E\x81\x00\x00\x00\x03\x00\xFF\x00\x00\x00\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ True)
+ self.assertEqual(logjig.data,
+ ["Association ID 255 doesn't match expected 2\n"])
+ # Test bad data padding
+ logjig.data = []
+ raw = "\x0E\x81\x00\x00\x00\x03\x00\x02\x00\x00\x00\x01@"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ self.assertEqual(cls._ControlSession__validate_packet(pkt, raw, 1, 2),
+ False)
+ self.assertEqual(logjig.data,
+ ["Response fragment not padded, size = 13\n"])
+ # Test too little data
+ logjig.data = []
+ raw = "\x0E\x81\x00\x00\x00\x03\x00\x02\x00\x00\x00\x10foo\x00"
+ pkt = ntp.packet.ControlPacket(cls)
+ pkt.analyze(raw)
+ try:
+ cls._ControlSession__validate_packet(pkt, raw, 1, 2)
+ self.assertEqual(True, False) # should have errored here
+ except ntp.packet.ControlException as e:
+ self.assertEqual(e.message, ntp.packet.SERR_INCOMPLETE)
+ self.assertEqual(logjig.data,
+ ["Response fragment claims 16 octets payload, "
+ "above 4 received\n"])
+
def test_doquery(self):
sends = []
def sendrequest_jig(opcode, associd, qdata, auth):
View it on GitLab: https://gitlab.com/NTPsec/ntpsec/commit/c835171b6b46a106c5b3896e8bf1fbe3c3764913
---
View it on GitLab: https://gitlab.com/NTPsec/ntpsec/commit/c835171b6b46a106c5b3896e8bf1fbe3c3764913
You're receiving this email because of your account on gitlab.com.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ntpsec.org/pipermail/vc/attachments/20170818/c6d8db16/attachment.html>
More information about the vc
mailing list