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

Make `SSL_SESSION_cmp` use `CRYPTO_memcmp` #591

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

3 participants
@bgw

bgw commented Apr 10, 2014

There could be potential issues with leaking session ids between clients, using a timing attack. This patch doesn't guarantee constant time for SSL_SESSION_cmp, but at least makes it constant time if the ssl_version and session_id_length are equal, which would make certain types of attacks more difficult.

For example, if one was attempting to determine the number of active SSL sessions on a server, this would largely thwart such an attacker.

To be clear: I do not believe this is a significant security issue, but rather a place where we might be able to more closely match a developer's expectations of the function.

For reference, see:

Make SSL_SESSION_cmp use CRYPTO_memcmp
There could be potential issues with leaking session ids between
clients, using a timing attack. This patch doesn't guarantee constant
time for SSL_SESSION_cmp, but at least makes it constant time if the
ssl_version and session_id_length are equal, which would make certain
types of attacks more difficult.

For example, if one was attempting to determine the number of active SSL
sessions on a server, this would largely thwart such an attacker.

To be clear: I do not believe this is a significant security issue, but
rather a place where we might be able to more closely match a
developer's expectations of the function.

For reference, see:
- http://seclists.org/fulldisclosure/2014/Apr/117
- https://news.ycombinator.com/item?id=7570043
@nobu

This comment has been minimized.

Show comment
Hide comment
@nobu

nobu May 24, 2014

Member

@emboss, how do you think?

Member

nobu commented May 24, 2014

@emboss, how do you think?

@emboss

This comment has been minimized.

Show comment
Hide comment
@emboss

emboss May 25, 2014

Member

@nobu @pipeep I'm pretty sure it doesn't hurt to use a constant time comparison in this place - better to play safe than sorry. Agreed!

Member

emboss commented May 25, 2014

@nobu @pipeep I'm pretty sure it doesn't hurt to use a constant time comparison in this place - better to play safe than sorry. Agreed!

@hsbt hsbt closed this in 2758be2 Jan 3, 2015

ayumin pushed a commit to ayumin/ruby that referenced this pull request Jan 4, 2015

* ext/openssl/ossl.h: Make `SSL_SESSION_cmp` use `CRYPTO_memcmp`
  [fix GH-591] Patch by @pipeep
* ext/openssl/ossl_ssl_session.c: ditto.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@49112 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

mmasaki pushed a commit to mmasaki/ruby that referenced this pull request May 30, 2015

* ext/openssl/ossl.h: Make `SSL_SESSION_cmp` use `CRYPTO_memcmp`
  [fix GH-591] Patch by @pipeep
* ext/openssl/ossl_ssl_session.c: ditto.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@49112 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment