sslgetcert: Add EC Point Formats extension to TLS handshake (fix connections to Vercel servers) #15
Conversation
|
So, if I understand you correctly, this is basically a workaround for a bug in someone else's implementation. In other areas (in particular DNS nowadays) that's a hot topic, and consensus seems to be to avoid doing it, because ultimately it complicates things rather than encourage bug fixing. I could not immediately find what "deprecated in TLS 1.3" really means, i.e. is it now a MUST NOT or a SHOULD NOT or something else. How sure are we that adding this will not actually upset other implementations, especially compliant ones? |
|
it's not a MUST NOT or a SHOULD NOT -- and when it comes to the ClientHello flight (the initial flight) of a TLS handshake from a client willing to do TLS 1.2 or TLS 1.3, it's entirely reasonable to include the extension, knowing that a TLS 1.3-only server won't care. At any rate, the sslgetcert probe itself can't do TLS 1.3 at the moment (it would need to be able to decrypt the incoming response since most of the server's first flight is ephemerally encrypted in TLS 1.3), so omitting that extension just because TLS 1.3 doesn't use it isn't a great reason. I do agree with you that vercel's implementation should be more gracious in handling the lack of an extension, regardless of the client's behavior, but this is not about working around a nasty bug by being non-compliant with the standards. The sslgetcert client isn't even a standards-compliant TLS endpoint, since the only thing it does is emit the ClientHello of a TLS handshake and record the response from the server. But even sslgetcert was a full TLS client, emitting the At any rate, i've opened a discussion over on vercel's community forum (i'm not a Vercel customer, and i don't know where else to do this) to encourage them to fix their server implementation as well, but i think until |
|
@dkg do you have the option of fixing the above comments and resubmitting the patch? Brgrds Michel |
|
@michel-stam i'm not sure i understand what comments need to be fixed.. If you can point me toward anything wrong with the patch, i'd be happy to fix it. |
Hi @dkg, Will tag you on the comments if thats ok with you. If anything is unclear, please reach out. Cheers, Michel |
|
@michel-stam wrote:
Sure, it is certainly OK to tag me on comments, but if you've already done that then i am not seeing them. Sorry if i'm just being confused about how github works. any explanation or pointers to specific comments would be welcome! |
eperd/sslgetcert.c
Outdated
| struct hsbuf elliptic_curves_buf; | ||
|
|
||
| /* SNI */ | ||
| hsbuf_init(&sni_buf); | ||
| if (state->sni) | ||
| sni(&sni_buf, state->sni); | ||
|
|
||
| /* EC point formats */ |
There was a problem hiding this comment.
The extension is only valid for TLS 1.0-1.2, so an "if ( (state->major_version == 3) && (state->minor_version >= 1) && (state->minor_version <= 3))" would be warranted. RFC 8422 specifically excludes it for any other version of TLS. TLS1.3 has a fixed point format instead of negotiating.
There was a problem hiding this comment.
@dkg this comment will need fixing.
| @@ -702,12 +726,16 @@ static void add_extensions(struct state *state, struct hsbuf *hsbuf) | |||
| hsbuf_init(&elliptic_curves_buf); | |||
| elliptic_curves(&elliptic_curves_buf); | |||
|
|
|||
| size_extensions= hsbuf_len(&sni_buf) + hsbuf_len(&ext_sigs_buf) + | |||
| size_extensions= hsbuf_len(&sni_buf) + | |||
There was a problem hiding this comment.
If statement applies here too.
There was a problem hiding this comment.
@dkg this comment will need fixing.
eperd/sslgetcert.c
Outdated
| hsbuf_len(&elliptic_curves_buf); | ||
|
|
||
| hsbuf_add_u16(hsbuf, size_extensions); | ||
| hsbuf_copy(hsbuf, &sni_buf); | ||
| hsbuf_cleanup(&sni_buf); | ||
| hsbuf_copy(hsbuf, &ec_point_formats_buf); |
There was a problem hiding this comment.
If statement applies here too.
There was a problem hiding this comment.
@dkg this comment will need fixing.
eperd/sslgetcert.c
Outdated
|
|
||
| epflen= len; | ||
| epfextlen = len + 1; | ||
| hsbuf_add(hsbuf, &c, 1); |
There was a problem hiding this comment.
I'm not sure what the hsbuff_add does here, u16 for ec_point_formats, u16 for length, 1 for the point_formats length and 1 for the point_formats itself. Can you explain?
There was a problem hiding this comment.
@dkg this comment will need fixing.
There was a problem hiding this comment.
thanks for catching this, this was just a silly mistake on my part.
…ections to Vercel servers) As described in Section 5.1.2 of RFC 8422, the EC Point Formats extension is valid for TLS 1.2 and earlier. It is deprecated in TLS 1.3 (RFC 8446), but that doesn't stop some servers from requiring it. There is no harm in supplying the extension to a TLS 1.3 server. In particular, Vercel (https://vercel.com/) TLS terminators seem to respond with a handshake failure alert if the EC Point Format extension is not present in the Client Hello. This can be seen with measurement 49131334: https://atlas.ripe.net/measurements/49131334/#probes This changeset should add the EC Point Format extension to the probe, which will result in successful certificate harvesting from Vercel servers, without introducing any incompatibilities to other servers.
dkg
force-pushed
the
sslgetcert-ec-point-format-extension
branch
from
aa65cf6
to
aabb55e
Compare
|
Thanks for the review, @michel-stam! I've pushed a new version that i think resolves all the issues. |
As described in Section 5.1.2 of RFC 8422, the EC Point Formats extension is valid for TLS 1.2 and earlier. It is deprecated in TLS 1.3 (RFC 8446), but that doesn't stop some servers from requiring it. There is no harm in supplying the extension to a TLS 1.3 server.
In particular, Vercel (https://vercel.com/) TLS terminators seem to respond with a handshake failure alert if the EC Point Format extension is not present in the Client Hello.
This can be seen with measurement 49131334:
https://atlas.ripe.net/measurements/49131334/#probes
This changeset should add the EC Point Format extension to the probe, which will result in successful certificate harvesting from Vercel servers, without introducing any incompatibilities to other servers.
I have not been able to test these changes as i don't have a full build environment set up for atlas probe. I welcome feedback or edits.