changes #1929
Closed
changes #1929
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tatsuhiro-t
added a commit
that referenced
this pull request
Jul 14, 2023
This commit fixes memory leak that happens when PUSH_PROMISE or HEADERS frame cannot be sent, and nghttp2_on_stream_close_callback fails with a fatal error. For example, if GOAWAY frame has been received, a HEADERS frame that opens new stream cannot be sent. This issue has already been made public via CVE-2023-35945 [1] issued by envoyproxy/envoy project. During embargo period, the patch to fix this bug was accidentally submitted to nghttp2/nghttp2 repository [2]. And they decided to disclose CVE early. I was notified just 1.5 hour before disclosure. I had no time to respond. PoC described in [1] is quite simple, but I think it is not enough to trigger this bug. While it is true that receiving GOAWAY prevents a client from opening new stream, and nghttp2 enters error handling branch, in order to cause the memory leak, nghttp2_session_close_stream function must return a fatal error. nghttp2 defines 2 fatal error codes: - NGHTTP2_ERR_NOMEM - NGHTTP2_ERR_CALLBACK_FAILURE NGHTTP2_ERR_NOMEM, as its name suggests, indicates out of memory. It is unlikely that a process gets short of memory with this simple PoC scenario unless application does something memory heavy processing. NGHTTP2_ERR_CALLBACK_FAILURE is returned from application defined callback function (nghttp2_on_stream_close_callback, in this case), which indicates something fatal happened inside a callback, and a connection must be closed immediately without any further action. As nghttp2_on_stream_close_error_callback documentation says, any error code other than 0 or NGHTTP2_ERR_CALLBACK_FAILURE is treated as fatal error code. More specifically, it is treated as if NGHTTP2_ERR_CALLBACK_FAILURE is returned. I guess that envoy returns NGHTTP2_ERR_CALLBACK_FAILURE or other error code which is translated into NGHTTP2_ERR_CALLBACK_FAILURE. If a client needs to close a connection, it should call nghttp2_session_terminate_session or its family functions, instead of returning NGHTTP2_ERR_CALLBACK_FAILURE. [1] GHSA-jfxv-29pc-x22r [2] #1929
tatsuhiro-t
added a commit
that referenced
this pull request
Jul 14, 2023
This commit fixes memory leak that happens when PUSH_PROMISE or HEADERS frame cannot be sent, and nghttp2_on_stream_close_callback fails with a fatal error. For example, if GOAWAY frame has been received, a HEADERS frame that opens new stream cannot be sent. This issue has already been made public via CVE-2023-35945 [1] issued by envoyproxy/envoy project. During embargo period, the patch to fix this bug was accidentally submitted to nghttp2/nghttp2 repository [2]. And they decided to disclose CVE early. I was notified just 1.5 hours before disclosure. I had no time to respond. PoC described in [1] is quite simple, but I think it is not enough to trigger this bug. While it is true that receiving GOAWAY prevents a client from opening new stream, and nghttp2 enters error handling branch, in order to cause the memory leak, nghttp2_session_close_stream function must return a fatal error. nghttp2 defines 2 fatal error codes: - NGHTTP2_ERR_NOMEM - NGHTTP2_ERR_CALLBACK_FAILURE NGHTTP2_ERR_NOMEM, as its name suggests, indicates out of memory. It is unlikely that a process gets short of memory with this simple PoC scenario unless application does something memory heavy processing. NGHTTP2_ERR_CALLBACK_FAILURE is returned from application defined callback function (nghttp2_on_stream_close_callback, in this case), which indicates something fatal happened inside a callback, and a connection must be closed immediately without any further action. As nghttp2_on_stream_close_error_callback documentation says, any error code other than 0 or NGHTTP2_ERR_CALLBACK_FAILURE is treated as fatal error code. More specifically, it is treated as if NGHTTP2_ERR_CALLBACK_FAILURE is returned. I guess that envoy returns NGHTTP2_ERR_CALLBACK_FAILURE or other error code which is translated into NGHTTP2_ERR_CALLBACK_FAILURE. [1] GHSA-jfxv-29pc-x22r [2] #1929
Merged
tatsuhiro-t
added a commit
that referenced
this pull request
Jul 14, 2023
This commit fixes memory leak that happens when PUSH_PROMISE or HEADERS frame cannot be sent, and nghttp2_on_stream_close_callback fails with a fatal error. For example, if GOAWAY frame has been received, a HEADERS frame that opens new stream cannot be sent. This issue has already been made public via CVE-2023-35945 [1] issued by envoyproxy/envoy project. During embargo period, the patch to fix this bug was accidentally submitted to nghttp2/nghttp2 repository [2]. And they decided to disclose CVE early. I was notified just 1.5 hours before disclosure. I had no time to respond. PoC described in [1] is quite simple, but I think it is not enough to trigger this bug. While it is true that receiving GOAWAY prevents a client from opening new stream, and nghttp2 enters error handling branch, in order to cause the memory leak, nghttp2_session_close_stream function must return a fatal error. nghttp2 defines 2 fatal error codes: - NGHTTP2_ERR_NOMEM - NGHTTP2_ERR_CALLBACK_FAILURE NGHTTP2_ERR_NOMEM, as its name suggests, indicates out of memory. It is unlikely that a process gets short of memory with this simple PoC scenario unless application does something memory heavy processing. NGHTTP2_ERR_CALLBACK_FAILURE is returned from application defined callback function (nghttp2_on_stream_close_callback, in this case), which indicates something fatal happened inside a callback, and a connection must be closed immediately without any further action. As nghttp2_on_stream_close_error_callback documentation says, any error code other than 0 or NGHTTP2_ERR_CALLBACK_FAILURE is treated as fatal error code. More specifically, it is treated as if NGHTTP2_ERR_CALLBACK_FAILURE is returned. I guess that envoy returns NGHTTP2_ERR_CALLBACK_FAILURE or other error code which is translated into NGHTTP2_ERR_CALLBACK_FAILURE. [1] GHSA-jfxv-29pc-x22r [2] #1929
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.