Check if length of frame is valid #135

jdecuyper · 2015-05-29T04:02:32Z

Fix for #69

Before processing a frame, check if its length is valid using the range defined by the spec.
Allow endpoint to update the max frame size setting.

When receiving a frame from endpoint, check if length of frame is below the maximum size setting. If frame size is invalid then send a reset frame to endpoint. Add method to the connection class that sends a reset frames with an error code.

If the setting frame contains a new valid max frame size then update hyper's settings. Add tests to confirm that this setting is updated only with values between the accepted range.

Lukasa · 2015-05-29T15:57:45Z

hyper/http20/connection.py

+                frame.stream_id,
+                length,
+                frame_size
+            )


It'd be safer to log first, I think: that way the log is emitted before we explode due to any traceback.

Oh, this check also isn't right. The value in self._settings[] is not a limit on the frame size the other side can emit, it's a limit on the size it can receive. hyper doesn't currently make the max frame size it can tolerate configurable (and I don't really feel like it should at this stage), so I think we just want to confirm that we tolerate MAX_FRAME_SIZE frames.

We then want to add a similar check to our own code for sending.

Lukasa · 2015-05-29T16:03:19Z

Brilliant stuff @jdecuyper! Great start. I've made some notes inline. =)

When hyper is sending a frame, it should validate its length against the constant MAX_FRAME_SIZE and not the endpoint's settings. Add error code parameter to the connection's close method. Check if socket is still alive before sending an ACK to the endpoint. Fix minor spelling errors.

…assume it is a gracefull shutdown.

jdecuyper · 2015-05-31T13:21:29Z

@Lukasa just to make sure I understand this right, when serializing a frame, we should be checking the length of the body against the maximum frame size. I think it would be nicer to apply this check when calling serialize from the send data callback. Is it okay to have serialize also return the body length?

Lukasa · 2015-05-31T13:27:48Z

hyper/http20/connection.py

+                    new_size)
+                # Tear the connection down with error code PROTOCOL_ERROR
+                self.close(1)
+


I think, rather than tie ourselves in knots trying to handle teardown mid-connection, we should throw exceptions whenever we have to tear a connection down ourselves. That means, still call self.close(), but then immediately throw an exception.

While we're here, it might be good to define an exception that we throw whenever we encounter a HTTP/2 connection error.

We have already such an exception available so I have used that one.

Lukasa · 2015-05-31T13:50:05Z

@jdecuyper That sounds reasonable to me =)

jdecuyper · 2015-06-01T03:48:48Z

Have that function serialize return 2 values was breaking too many existing code so I took a different approach and expose the length of the body through self.body_len.

Lukasa · 2015-06-01T09:28:24Z

hyper/packages/hyperframe/frame.py

@@ -63,7 +65,7 @@ def parse_flags(self, flag_byte):

    def serialize(self):
        body = self.serialize_body()
-        body_len = len(body)
+        self.body_len = len(body)


Let's make sure we initialise this to zero in __init__.

All set! Anything else I should cover?

Lukasa · 2015-06-03T22:17:05Z

hyper/http20/connection.py

@@ -482,6 +497,9 @@ def _send_cb(self, frame, tolerate_peer_gone=False):

        data = frame.serialize()

+        if frame.body_len > FRAME_MAX_LEN:  # pragma: no cover


So this is the point where we need to abort based on the set max frame size. Can you change this check to do that?

Sorry about having to make this note. I've had lots of trouble in the past keeping track of which setting applies to which direction in the communication, so I'm not entirely surprised that we've had to go back and forth on this a few times.

I also don't think there's any good reason to #pragma: no cover this, we can hit it pretty directly to prove the case.

Lukasa · 2015-06-21T18:53:14Z

@jdecuyper What's the status here?

jdecuyper · 2015-06-22T22:43:59Z

Status is: will work on fixes tomorrow if that's alright :)

Lukasa · 2015-06-23T06:59:03Z

Sure thing. =) There's no rush, just wanted to make sure this didn't get lost! 👍

…size

When sending a frame to the server, care must be taken to not exceed the maximum frame size setting. If the size is higher than the defined setting then a ValueError is thrown.

jdecuyper · 2015-06-27T03:27:09Z

@Lukasa
When sending a frame, the size is checked against the maximum frame size setting (which can be updated by the server). As you asked, I have also added a test for this case.

jdecuyper · 2015-06-27T22:11:59Z

Cool! Curious to see what the fix is about =)
In the meantime I have create the PR: python-hyper/hyperframe#2

Lukasa · 2015-06-28T10:45:56Z

The fix already landed, see python-hyper/hyperframe@2363947.

If you rebase on top of development you should find the code you need has been vendored in from hyperframe. =D

When receiving a frame from endpoint, check if length of frame is below the maximum size setting. If frame size is invalid then send a reset frame to endpoint. Add method to the connection class that sends a reset frames with an error code.

If the setting frame contains a new valid max frame size then update hyper's settings. Add tests to confirm that this setting is updated only with values between the accepted range.

When hyper is sending a frame, it should validate its length against the constant MAX_FRAME_SIZE and not the endpoint's settings. Add error code parameter to the connection's close method. Check if socket is still alive before sending an ACK to the endpoint. Fix minor spelling errors.

…assume it is a gracefull shutdown.

When sending a frame to the server, care must be taken to not exceed the maximum frame size setting. If the size is higher than the defined setting then a ValueError is thrown.

jdecuyper · 2015-06-29T03:10:04Z

@Lukasa the rebase worked, let me know if it looks alright to you!

Lukasa · 2015-06-29T07:25:05Z

This is generally good, though the commit log has gone a bit wacky. Do you feel comfortable enough in git to fix it up, or do you want me to handle that on my end?

jdecuyper · 2015-06-29T18:30:15Z

Sorry to bother you with this, it would be great if you could help me out here and even better if you could explain how you do it (if you have some time of course). Thanks!

Lukasa · 2015-06-29T18:33:46Z

Of course! I'll see if I can screen record the fix.

sigmavirus24 · 2015-06-29T18:49:12Z

Asciinema is pretty great fwiw

Lukasa · 2015-06-29T19:40:31Z

@jdecuyper The fix was easy, as you can see here. All I did was take the last commit that contained actual work from you (a031605) and merge it into the development branch. In that video you can see me scrolling down the terminal to ensure that none of your commits are there twice, which they aren't, so we know we're good.

Lukasa · 2015-06-29T19:47:38Z

Alrighty, merged manually. Thanks @jdecuyper!

jdecuyper · 2015-06-29T22:36:31Z

@Lukasa many thanks for taking the time to explain 👍 👍

jdecuyper · 2015-06-29T22:48:25Z

And btw thanks for the mentions in the v0.4.0 release notes =)

jdecuyper added 2 commits May 28, 2015 22:28

Validate frame size

c22cadc

When receiving a frame from endpoint, check if length of frame is below the maximum size setting. If frame size is invalid then send a reset frame to endpoint. Add method to the connection class that sends a reset frames with an error code.

Allow endpoint to update max frame size

ea57148

If the setting frame contains a new valid max frame size then update hyper's settings. Add tests to confirm that this setting is updated only with values between the accepted range.

Lukasa reviewed May 29, 2015
View reviewed changes

jdecuyper added 4 commits May 30, 2015 16:15

If an error code is not provided to the stream's close function then …

9c04e5d

…assume it is a gracefull shutdown.

Fix confusion between MAX_FRAME_SIZE and endpoint settings (unit tests)

c98fa9a

Fix spelling error

0ed42ca

Lukasa reviewed May 31, 2015
View reviewed changes

jdecuyper added 3 commits May 31, 2015 22:42

Throw connection error when the frame size setting is out of range

ee5fa4d

Expose the body length when serializing a frame

77796ab

Make sure GoAway frame is sent when tearing down the connection

7ff0c52

Lukasa reviewed Jun 1, 2015
View reviewed changes

Initialize body length to zero in frame init method

9cff560

Lukasa reviewed Jun 3, 2015
View reviewed changes

Merge branch 'development' of github.com:Lukasa/hyper into max-frame-…

1801faf

…size

Validate size when sending out of range frames

cc0843f

When sending a frame to the server, care must be taken to not exceed the maximum frame size setting. If the size is higher than the defined setting then a ValueError is thrown.

jdecuyper mentioned this pull request Jun 27, 2015

Expose frame's length python-hyper/hyperframe#2

Merged

Update hyperframe to 1.1.0

5d8b321

jdecuyper added 10 commits June 28, 2015 21:40

Validate frame size

4f051a4

When receiving a frame from endpoint, check if length of frame is below the maximum size setting. If frame size is invalid then send a reset frame to endpoint. Add method to the connection class that sends a reset frames with an error code.

Allow endpoint to update max frame size

25f10db

If the setting frame contains a new valid max frame size then update hyper's settings. Add tests to confirm that this setting is updated only with values between the accepted range.

If an error code is not provided to the stream's close function then …

5ba393f

…assume it is a gracefull shutdown.

Fix confusion between MAX_FRAME_SIZE and endpoint settings (unit tests)

b62fa4f

Throw connection error when the frame size setting is out of range

adc6418

Make sure GoAway frame is sent when tearing down the connection

4bdd0e3

Validate size when sending out of range frames

a031605

When sending a frame to the server, care must be taken to not exceed the maximum frame size setting. If the size is higher than the defined setting then a ValueError is thrown.

Rebase my patch on top of development

105006d

Merge my updates with latest from development

9f239b0

Lukasa added a commit that referenced this pull request Jun 29, 2015

Add changelog for #135

f3fecc7

Lukasa closed this Jun 29, 2015

jdecuyper deleted the max-frame-size branch June 29, 2015 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if length of frame is valid #135

Check if length of frame is valid #135

jdecuyper commented May 29, 2015

Lukasa May 29, 2015

Lukasa May 29, 2015

Lukasa commented May 29, 2015

jdecuyper commented May 31, 2015

Lukasa May 31, 2015

jdecuyper Jun 1, 2015

Lukasa commented May 31, 2015

jdecuyper commented Jun 1, 2015

Lukasa Jun 1, 2015

jdecuyper Jun 3, 2015

Lukasa Jun 3, 2015

Lukasa Jun 3, 2015

Lukasa Jun 3, 2015

Lukasa commented Jun 21, 2015

jdecuyper commented Jun 22, 2015

Lukasa commented Jun 23, 2015

jdecuyper commented Jun 27, 2015

jdecuyper commented Jun 27, 2015

Lukasa commented Jun 28, 2015

jdecuyper commented Jun 29, 2015

Lukasa commented Jun 29, 2015

jdecuyper commented Jun 29, 2015

Lukasa commented Jun 29, 2015

sigmavirus24 commented Jun 29, 2015

Lukasa commented Jun 29, 2015

Lukasa commented Jun 29, 2015

jdecuyper commented Jun 29, 2015

jdecuyper commented Jun 29, 2015

		@@ -482,6 +497,9 @@ def _send_cb(self, frame, tolerate_peer_gone=False):

		data = frame.serialize()

		if frame.body_len > FRAME_MAX_LEN: # pragma: no cover

Check if length of frame is valid #135

Check if length of frame is valid #135

Conversation

jdecuyper commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented May 29, 2015

jdecuyper commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented May 31, 2015

jdecuyper commented Jun 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jun 21, 2015

jdecuyper commented Jun 22, 2015

Lukasa commented Jun 23, 2015

jdecuyper commented Jun 27, 2015

jdecuyper commented Jun 27, 2015

Lukasa commented Jun 28, 2015

jdecuyper commented Jun 29, 2015

Lukasa commented Jun 29, 2015

jdecuyper commented Jun 29, 2015

Lukasa commented Jun 29, 2015

sigmavirus24 commented Jun 29, 2015

Lukasa commented Jun 29, 2015

Lukasa commented Jun 29, 2015

jdecuyper commented Jun 29, 2015

jdecuyper commented Jun 29, 2015