Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http2: writePing does not verify data.readableBytes() == 8 #3721

Closed
ejona86 opened this issue May 4, 2015 · 10 comments
Closed

http2: writePing does not verify data.readableBytes() == 8 #3721

ejona86 opened this issue May 4, 2015 · 10 comments
Assignees
@Scottmitch
Labels
improvement
Milestone
4.1.0.Beta7

Comments

@ejona86
Copy link
Member

ejona86 commented May 4, 2015

When writing the ping frame, we aren't verifying that the length is 8, as required by the spec:

Receipt of a PING frame with a length field value other than 8 MUST be treated as a connection error (Section 5.4.1) of type FRAME_SIZE_ERROR.

The check is being performed for inbound PING frames.
@normanmaurer
Copy link
Member

@ejona86 @Scottmitch @nmittler @louiscryan is this something we need to fix before 4.1.0.Final ?

@nmittler
Copy link
Member

nmittler commented Sep 9, 2015

I don't think so. We seem to be conforming to the spec since we're checking the size on the inbound frame.

@normanmaurer
Copy link
Member

@nmittler so closing ?

@nmittler
Copy link
Member

nmittler commented Sep 9, 2015

@ejona86 thoughts on closing?

@Scottmitch
Copy link
Member

wouldn't hurt to check the write size...make it harder for people to put bad stuff out on the network.

@normanmaurer
Copy link
Member

@Scottmitch care to submit a pr ;) ?

@Scottmitch Scottmitch self-assigned this Sep 9, 2015
@Scottmitch Scottmitch added this to the 4.1.0.Beta7 milestone Sep 9, 2015
@Scottmitch
Copy link
Member

@normanmaurer - Will do.

@nmittler - Any objections?

@normanmaurer
Copy link
Member

@Scottmitch 😍 ;)

@Scottmitch
Copy link
Member

pr pending

@nmittler
Copy link
Member

nmittler commented Sep 9, 2015

@Scottmitch no objections ... thanks for taking care

Scottmitch added a commit to Scottmitch/netty that referenced this issue Sep 14, 2015
@Scottmitch
DefaultHttp2FrameWriter ping payload size check
7b6e431 
Motivation:
The HTTP/2 spec states that the ping frame length must be 8 and is otherwise an error https://tools.ietf.org/html/rfc7540#section-6.7. The DefaultHttp2FrameReader enforces this, but the DefaultHttp2FrameWriter allows invalid frames to be written. We should not allow invalid ping frames to be written to the network.

Modifications:
- DefaultHttp2FrameWriter checks the frame size to be 8, or throws an exception

Result:
Fixes netty#3721
@Scottmitch Scottmitch mentioned this issue Sep 14, 2015
Closed
Scottmitch added a commit that referenced this issue Sep 16, 2015
@Scottmitch
DefaultHttp2FrameWriter ping payload size check
15450af 
Motivation:
The HTTP/2 spec states that the ping frame length must be 8 and is otherwise an error https://tools.ietf.org/html/rfc7540#section-6.7. The DefaultHttp2FrameReader enforces this, but the DefaultHttp2FrameWriter allows invalid frames to be written. We should not allow invalid ping frames to be written to the network.

Modifications:
- DefaultHttp2FrameWriter checks the frame size to be 8, or throws an exception

Result:
Fixes #3721
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Scottmitch Scottmitch

Labels
improvement
Projects
None yet
Milestone
4.1.0.Beta7
Development

No branches or pull requests
4 participants
@normanmaurer @ejona86 @nmittler @Scottmitch