http2: convert an assert to run-time check #2514

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented Apr 21, 2018

Fuzzing has proven we can reach code in on_frame_recv with status_code
not having been set, so let's detect that in run-time (instead of with
assert) and error error accordingly.

Detected by OSS-Fuzz
Bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7903

http2: convert an assert to run-time check
Fuzzing has proven we can reach code in on_frame_recv with status_code
not having been set, so let's detect that in run-time (instead of with
assert) and error error accordingly.

Detected by OSS-Fuzz
Bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7903

@bagder bagder added the HTTP/2 label Apr 21, 2018

@cmeister2

This comment has been minimized.

Show comment Hide comment
@cmeister2

cmeister2 Apr 21, 2018

Contributor

Still present with master nghttp2 (as opposed to 1.24). @tatsuhiro-t: does this assert indicate a potential problem in nghttp2?

Contributor

cmeister2 commented Apr 21, 2018

Still present with master nghttp2 (as opposed to 1.24). @tatsuhiro-t: does this assert indicate a potential problem in nghttp2?

@tatsuhiro-t

This comment has been minimized.

Show comment Hide comment
@tatsuhiro-t

tatsuhiro-t Apr 21, 2018

Contributor

I have no permission to view the page, but it looks like https://github.com/curl/curl/pull/2514/files#diff-7dcf04be672466b7a56e6a81df098c6bR635 is suspicious to me. I think we should set -1 to status_code only for 1xx status code.

Contributor

tatsuhiro-t commented Apr 21, 2018

I have no permission to view the page, but it looks like https://github.com/curl/curl/pull/2514/files#diff-7dcf04be672466b7a56e6a81df098c6bR635 is suspicious to me. I think we should set -1 to status_code only for 1xx status code.

@cmeister2

This comment has been minimized.

Show comment Hide comment
@cmeister2

cmeister2 Apr 21, 2018

Contributor

For reference, the relevant http2 data we're receiving in our error case is in this pcap file: http2_5380578661629952.zip

Contributor

cmeister2 commented Apr 21, 2018

For reference, the relevant http2 data we're receiving in our error case is in this pcap file: http2_5380578661629952.zip

@cmeister2

This comment has been minimized.

Show comment Hide comment
@cmeister2

cmeister2 Apr 21, 2018

Contributor

@tatsuhiro-t: looking at this a bit more; it looks like status_code is only being set in on_header. In the test program we never actually call this function at all. Gist from verbose output is here (I added an explicit infof to the top of on_header to make sure it was being called.)

I don't have any more knowledge here about whether the status_code comment is correct or not; however, I think this fix is likely to solve our problems by basically being defensive.

Contributor

cmeister2 commented Apr 21, 2018

@tatsuhiro-t: looking at this a bit more; it looks like status_code is only being set in on_header. In the test program we never actually call this function at all. Gist from verbose output is here (I added an explicit infof to the top of on_header to make sure it was being called.)

I don't have any more knowledge here about whether the status_code comment is correct or not; however, I think this fix is likely to solve our problems by basically being defensive.

@tatsuhiro-t

This comment has been minimized.

Show comment Hide comment
@tatsuhiro-t

tatsuhiro-t Apr 21, 2018

Contributor

It looks like something is wrong in nghttp2 code. Will look into it.

Contributor

tatsuhiro-t commented Apr 21, 2018

It looks like something is wrong in nghttp2 code. Will look into it.

@tatsuhiro-t

This comment has been minimized.

Show comment Hide comment
@tatsuhiro-t

tatsuhiro-t Apr 22, 2018

Contributor

I identified bug in nghttp2, and fixed it now.

Contributor

tatsuhiro-t commented Apr 22, 2018

I identified bug in nghttp2, and fixed it now.

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Apr 22, 2018

Member

Awesome @tatsuhiro-t, its great to see that our fuzzing indirectly helped you as well! =) So, with that new find and fix of yours in mind, do you think my PR here then makes sense for curl when using older libnghttp2 installations?

Member

bagder commented Apr 22, 2018

Awesome @tatsuhiro-t, its great to see that our fuzzing indirectly helped you as well! =) So, with that new find and fix of yours in mind, do you think my PR here then makes sense for curl when using older libnghttp2 installations?

@tatsuhiro-t

This comment has been minimized.

Show comment Hide comment
@tatsuhiro-t

tatsuhiro-t Apr 22, 2018

Contributor

@bagder Yes, I do. It is better than failing with assertion error. And this PR works as well with latest nghttp2 code.

Contributor

tatsuhiro-t commented Apr 22, 2018

@bagder Yes, I do. It is better than failing with assertion error. And this PR works as well with latest nghttp2 code.

@bagder bagder closed this in 0a3589c Apr 23, 2018

@bagder bagder deleted the bagder/http2-assert-to-run-time branch Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment