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 checks for TLS_FALLBACK_SCSV #816

Closed

Conversation

andresag01
Copy link
Contributor

The code in library/ssl_srv.c looks for the cipher suite value
TLS_FALLBACK_SCSV in the list of supported cipher suites of the
ClientHello as decribed in RFC7507. However, Section 4 says that the
client SHOULD place the TLS_FALLBACK_SCSV at the end of the list of
cipher suites, a condition that was not checked. This patch introduces
the additional check.

NOTES:

  • The PR needs to be backported to mbed TLS versions 1.3 and 2.1.
  • This change addresses Bug in SCSV #810
  • The PR does not include automated tests because to enforce the path through the code we need to craft a special ClientHello that does not have the TLS_FALLBACK_SCSV at the end of the cipher suites list. However, the change was tested manually.

The code in library/ssl_srv.c looks for the cipher suite value
TLS_FALLBACK_SCSV in the list of supported cipher suites of the
ClientHello as decribed in RFC7507. However, Section 4 says that the
client SHOULD place the TLS_FALLBACK_SCSV at the end of the list of
cipher suites, a condition that was not checked. This patch introduces
the additional check.
@andresag01
Copy link
Contributor Author

@yanesca and @sbutcher-arm: Could you please review this change?

@andresag01 andresag01 mentioned this pull request Feb 22, 2017
@ghost
Copy link

ghost commented Feb 22, 2017

As far as I can see, this solves the issue. Many thanks!!

@simonbutcher
Copy link
Contributor

Hi @hsleisink,

You're obviously welcome to use the fix, but I feel I have to point out, we haven't finished it yet. And it's still pending review and potentially rework.

@ghost
Copy link

ghost commented Feb 23, 2017

Ok, thanks. I will wait until it's considered 'finished'. Any indication about when it will be finished?

@ghost
Copy link

ghost commented Apr 5, 2017

Can this patch already be considered as 'finished'?

@gilles-peskine-arm
Copy link
Contributor

After further investigation, it turns out that the offset calculation was the problem. The server should not care about the position of TLS_FALLBACK_SCSV in the ciphersuite list; the offset calculation bug made it accidentally care. I have submitted PR #924 with only the offset calculation fix, and a non-regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants