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

Update signed_params to included hello.random data #46

Merged
merged 2 commits into from Jun 20, 2014
Merged

Update signed_params to included hello.random data #46

merged 2 commits into from Jun 20, 2014

Conversation

colmmacc
Copy link
Contributor

This change clarifies that the signature over ServerDHParams data is a
signature over the hash of the client + server hello random random data and the
ServerDHParams data.

I believe this small regression was introduced in TLS1.2 with the result
that the TLS1.2 RFC (and current TLS1.3 draft) does not contain sufficient detail
to implement TLS + DHE.

All TLS1.2 clients and servers that I have tested do include the hello random
data (and omitting it would leave implementations open to replay attacks).

This change clarifies that the signature over ServerDHParams data is a
signature over the hash of the client + server hello random random data and the
ServerDHParams data.

I believe this small regression was introduced in TLS1.2 with the result
that the TLS1.2 RFC (and current TLS1.3) does not contain sufficient detail
to implement TLS + DHE.
@ekr
Copy link
Contributor

ekr commented Jun 20, 2014

Thanks for the PR.

Note that the formal PDU definitions here appear to be correct, so there should
be enough information to implement things correctly:

          case dhe_rsa:
              ServerDHParams params;
              digitally-signed struct {
                  opaque client_random[32];
                  opaque server_random[32];
                  ServerDHParams params;
              } signed_params;

I agree it wouldn't hurt to have the explanatory text be clearer. However, in TLS 1.2 we stopped explicitly saying that things were hashed before signing (because we are treating signing as a primitive), so we probably don't want to say "hash". Maybe just say a signature over the random values
and the params?

@colmmacc
Copy link
Contributor Author

Ah, this explains my confusion. After reading section 7.4.1.4.1, I formed the opposite impression about signing; that hashing and signing are explicitly different primitive operations and may be composed in a variety of ways.

But I see how that "digitally-signed" seems to be a reserved term that implies both operations.

@ekr
Copy link
Contributor

ekr commented Jun 20, 2014

I'm not trying to discourage you here. If you were confused, others will probably be as well, so if you have some text that you think would clear this up, it would be welcome.

@colmmacc
Copy link
Contributor Author

Thanks! I've updated the PR with an even simpler diff that I think would have made me less confused when I got to it.

ekr added a commit that referenced this pull request Jun 20, 2014
Update text about DHE parameters signing to make clear that it includes the randoms.

Original commit message was: Update signed_params to included hello.random data
@ekr ekr merged commit fa0ec32 into tlswg:master Jun 20, 2014
ekr added a commit that referenced this pull request Oct 3, 2020
ekr pushed a commit that referenced this pull request Oct 3, 2020
when appliccable is redundant. Fixes #46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants