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.

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

Fix CVE-2024-5535 #634

Merged
merged 9 commits into from
Aug 19, 2024
Merged

Conversation

dongbeiouba
Copy link
Member

Fix CVE-2024-5535, SSL_select_next_proto buffer overread.

Severity: Low

Issue summary: Calling the OpenSSL API function SSL_select_next_proto with an
empty supported client protocols buffer may cause a crash or memory contents to
be sent to the peer.

Impact summary: A buffer overread can have a range of potential consequences
such as unexpected application beahviour or a crash. In particular this issue
could result in up to 255 bytes of arbitrary private data from memory being sent
to the peer leading to a loss of confidentiality. However, only applications
that directly call the SSL_select_next_proto function with a 0 length list of
supported client protocols are affected by this issue. This would normally never
be a valid scenario and is typically not under attacker control but may occur by
accident in the case of a configuration or programming error in the calling
application.

The OpenSSL API function SSL_select_next_proto is typically used by TLS
applications that support ALPN (Application Layer Protocol Negotiation) or NPN
(Next Protocol Negotiation). NPN is older, was never standardised and
is deprecated in favour of ALPN. We believe that ALPN is significantly more
widely deployed than NPN. The SSL_select_next_proto function accepts a list of
protocols from the server and a list of protocols from the client and returns
the first protocol that appears in the server list that also appears in the
client list. In the case of no overlap between the two lists it returns the
first item in the client list. In either case it will signal whether an overlap
between the two lists was found. In the case where SSL_select_next_proto is
called with a zero length client list it fails to notice this condition and
returns the memory immediately following the client list pointer (and reports
that there was no overlap in the lists).

This function is typically called from a server side application callback for
ALPN or a client side application callback for NPN. In the case of ALPN the list
of protocols supplied by the client is guaranteed by libssl to never be zero in
length. The list of server protocols comes from the application and should never
normally be expected to be of zero length. In this case if the
SSL_select_next_proto function has been called as expected (with the list
supplied by the client passed in the client/client_len parameters), then the
application will not be vulnerable to this issue. If the application has
accidentally been configured with a zero length server list, and has
accidentally passed that zero length server list in the client/client_len
parameters, and has additionally failed to correctly handle a "no overlap"
response (which would normally result in a handshake failure in ALPN) then it
will be vulnerable to this problem.

In the case of NPN, the protocol permits the client to opportunistically select
a protocol when there is no overlap. OpenSSL returns the first client protocol
in the no overlap case in support of this. The list of client protocols comes
from the application and should never normally be expected to be of zero length.
However if the SSL_select_next_proto function is accidentally called with a
client_len of 0 then an invalid memory pointer will be returned instead. If the
application uses this output as the opportunistic protocol then the loss of
confidentiality will occur.

This issue has been assessed as Low severity because applications are most
likely to be vulnerable if they are using NPN instead of ALPN - but NPN is not
widely used. It also requires an application configuration or programming error.
Finally, this issue would not typically be under attacker control making active
exploitation unlikely.

OpenSSL 3.3, 3.2, 3.1, 3.0, 1.1.1 and 1.0.2 are vulnerable to this issue.

Checklist
  • https://yuque.com/tsdoc 增加或更新了必要的文档
  • 增加或更新了必要的测试用例
  • 对于重要修改,更新了CHANGES文件
  • 当前修改存在对已有API参数或返回值的改变
  • 当前修改存在对旧版本功能的兼容性改变(如网络协议或密码算法)

Ensure that the provided client list is non-NULL and starts with a valid
entry. When called from the ALPN callback the client list should already
have been validated by OpenSSL so this should not cause a problem. When
called from the NPN callback the client list is locally configured and
will not have already been validated. Therefore SSL_select_next_proto
should not assume that it is correctly formatted.

We implement stricter checking of the client protocol list. We also do the
same for the server list while we are about it.

CVE-2024-5535

(cherry picked from commit cf6f91f6121f4db167405db2f0de410a456f260c)
@dongbeiouba dongbeiouba added bug Something isn't working master labels Jul 12, 2024
@dongbeiouba dongbeiouba requested review from InfoHunter, uudiin, wa5i, zzl360 and a team July 12, 2024 07:31
In the case where the NPN callback returns with SSL_TLEXT_ERR_OK, but
the selected_len is 0 we should fail. Previously this would fail with an
internal_error alert because calling OPENSSL_malloc(selected_len) will
return NULL when selected_len is 0. We make this error detection more
explicit and return a handshake failure alert.

Follow on from CVE-2024-5535

(cherry picked from commit 159921152fd4aa91e4c849fd281ad93ac0d0d0ba)
Follow on from CVE-2024-5535

(cherry picked from commit 707c71aa03ba968e09325d72cf1e8dcac70df2df)
Allow ourselves to configure an empty NPN/ALPN protocol list and test what
happens if we do.

Follow on from CVE-2024-5535

(cherry picked from commit 72394c9a1a6a6b07edf43eb2ad7e95e1093ada1b)
Return EXT_RETURN_NOT_SENT in the event that we don't send the extension,
rather than EXT_RETURN_SENT. This actually makes no difference at all to
the current control flow since this return value is ignored in this case
anyway. But lets make it correct anyway.

Follow on from CVE-2024-5535

(cherry picked from commit 189a7ed3e380e34ea38fe4190a7c9396bace0fb7)
The ALPN protocol selected by the server must be one that we originally
advertised. We should verify that it is.

Follow on from CVE-2024-5535

(cherry picked from commit 4b375b998798dd516d367036773073e1b88e6433)
We already had some tests elsewhere - but this extends that testing with
additional tests.

Follow on from CVE-2024-5535

(cherry picked from commit ca176d7291eb780e4ed2781342f5be5a32210a68)
It is valid according to the spec for a NextProto message to have no
protocols listed in it. The OpenSSL implementation however does not allow
us to create such a message. In order to check that we work as expected
when communicating with a client that does generate such messages we have
to use a TLSProxy test.

Follow on from CVE-2024-5535

(cherry picked from commit 99c2b6b971c302595db1801e26a202247238659d)
Copy link
Contributor

@wa5i wa5i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@InfoHunter InfoHunter merged commit eddba6c into Tongsuo-Project:master Aug 19, 2024
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants