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

url: revert the removal of trailing dot from host name #8320

Closed
wants to merge 5 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 22, 2022

Reverts 5de8d84 (May 2014, shipped in 7.37.0) and the
follow-up changes done afterward.

Keep the dot in names for everything except the SNI to make curl behave
more similar to current browsers. This means 'name' and 'name.' send the
same SNI for different 'Host:' headers.

Updated test 1322 accordingly

Fixes #8290
Reported-by: Charles Cazabon
Closes #

@bagder bagder added HTTP URL name lookup labels Jan 22, 2022
@jay

This comment has been minimized.

@bagder
Copy link
Member Author

@bagder bagder commented Jan 22, 2022

Another thing that struck me is that while we agree the SNI host is without the trailing dot, but how about the name we use to verify the server cert? At least one TLS library won't let us use different names for those two different uses. Does it mean we should use the SNI name there or the resolve host name?

RFC 6125 doesn't seem to say

@bagder
Copy link
Member Author

@bagder bagder commented Jan 22, 2022

The SNI requirement leaves no room for other interpretations:

"HostName" contains the fully qualified DNS hostname of the server,
as understood by the client. The hostname is represented as a byte
string using ASCII encoding without a trailing dot.

RFC 6066 section 3

@jay
Copy link
Member

@jay jay commented Jan 22, 2022

Yes that part is clear. Are there any server certificates that have the trailing dot in the altname? I don't know of any.

@bagder
Copy link
Member Author

@bagder bagder commented Jan 22, 2022

Do you know any HTTPS site that cares about the trailing dot in a Host: header? I don't.

@jay
Copy link
Member

@jay jay commented Jan 22, 2022

I don't either but the reported issue looks legit.

@bagder
Copy link
Member Author

@bagder bagder commented Jan 22, 2022

But yes, browsers seem to use the SNI name too (no dot) for checking the server cert.

@bagder bagder force-pushed the bagder/keep-trailing-dot branch from 826bb39 to dfa5fc1 Compare Jan 23, 2022
bagder added a commit that referenced this issue Jan 23, 2022
Reverts 5de8d84 (May 2014, shipped in 7.37.0) and the
follow-up changes done afterward.

Keep the dot in names for everything except the SNI to make curl behave
more similar to current browsers. This means 'name' and 'name.' send the
same SNI for different 'Host:' headers.

Updated test 1322 accordingly

Fixes #8290
Reported-by: Charles Cazabon
Closes #8320
@bagder bagder force-pushed the bagder/keep-trailing-dot branch from dfa5fc1 to dbe35d6 Compare Jan 23, 2022
@krono
Copy link

@krono krono commented Jan 23, 2022

@bagder (not nearly having as much experience, but maybe this is helpful:)

At one point, I had to hand-implement the hostname-matching part for a programming language VM's SSL plugin and tired to make it as similar behaving as other clients I had at hand. So I toyed around: https://github.com/krono/x509hostmatch

And to make that work, had to dot-drop both the cert-sAN and the incoming hostname…
https://github.com/krono/x509hostmatch/blob/master/x509hostmatch.c#L69

HTH

@jay
Copy link
Member

@jay jay commented Jan 23, 2022

I think more changes will be needed for hostname verification. For some backends we call Curl_cert_hostcheck (which strips the trailing dot) and that should be fine. For others though we call a backend specific function which may or may not handle the trailing dot. Tests of the backends I build running curld https://httpbin.org./:

OpenSSL: Works. Done by Curl_cert_hostcheck.
Schannel in manual verify mode (ie ca bundle specified): Works. Done by Curl_cert_hostcheck.
Schannel in auto verify mode (ie no ca bundle, use os certs): Works. Done by Schannel's InitializeSecurityContext.
wolfSSL: Doesn't work. Done by wolfSSL's wolfSSL_check_domain_name.

curl/lib/vtls/wolfssl.c

Lines 602 to 607 in 801bd51

/* Enable RFC2818 checks */
if(SSL_CONN_CONFIG(verifyhost)) {
ret = wolfSSL_check_domain_name(backend->handle, hostname);
if(ret == SSL_FAILURE)
return CURLE_OUT_OF_MEMORY;
}


Another thing, the commit squashme: more sni conversions for schannel adds two more SNI conversions for other InitializeSecurityContext calls. The problem is those calls are not necessarily done at the beginning, and currently Curl_ssl_snihost uses data->state.buffer to store the SNI temporarily, however it relies on data->state.buffer not actually being used for something else during this time. I don't know if a separate buffer is needed to avoid overwriting data->state.buffer.

char *buffer; /* download buffer */

@bagder
Copy link
Member Author

@bagder bagder commented Jan 24, 2022

The problem is those calls are not necessarily done at the beginning

Then I think the name needs to be copied from when it is called in the beginning. I mean, we don't necessarily have to make the function do that generically for all backends since SNI sort of implies it is done before the transfer starts (and the buffer is used for download data).

bagder added 2 commits Jan 24, 2022
Reverts 5de8d84 (May 2014, shipped in 7.37.0) and the
follow-up changes done afterward.

Keep the dot in names for everything except the SNI to make curl behave
more similar to current browsers. This means 'name' and 'name.' send the
same SNI for different 'Host:' headers.

Updated test 1322 accordingly

Fixes #8290
Reported-by: Charles Cazabon
Closes #8320
The TLS backends convert the host name to SNI name and need to use that.

Assisted-by: Jay Satiro
Closes #8320
@bagder bagder force-pushed the bagder/keep-trailing-dot branch from 5380919 to 5fee758 Compare Jan 24, 2022
@jay
Copy link
Member

@jay jay commented Jan 25, 2022

Then I think the name needs to be copied from when it is called in the beginning.

Ok redid the schannel sni code so that now Curl_ssl_snihost is only called once, right after schannel_acquire_credential_handle in step 1, and it copies the SNI hostname needed by the credential.

I still don't like how Curl_ssl_snihost uses the download buffer as the temporary space, I think that could lead to a bug one day. For example I have the sni hostname retrieved separately from schannel_acquire_credential_handle but it really is needed for any Schannel credential handle, and what if a credential handle needs to be created at some point other than step 1? Who is going to remember that Curl_ssl_snihost can only be called from step 1?

I suggest adding a dedicated buffer to an ssl struct, which shouldn't be that large for example as fqdn 255 max so char snihost[255+1].

@bagder
Copy link
Member Author

@bagder bagder commented Jan 25, 2022

I suggest adding a dedicated buffer to an ssl struct, which shouldn't be that large for example as fqdn 255 max so char snihost[255+1].

I checked the spec. The SNI field can be 64K. I don't think it is always limited to the max DNS name size. Name resolving does not always mean DNS and there's no 255 byte limit in host names in URLs.

It feels wrong to me to "waste" such a large data field in a long-living struct for something that is only needed when setting up the connection and then isn't needed anymore. The download buffer has been used for this purpose for well over seven years now without problems, which could be an indicator that it might be okay.

@bagder
Copy link
Member Author

@bagder bagder commented Jan 25, 2022

One additional detail struck me with the kept trailing dot. A connection to example.com. (with a trailing dot) will not be reused/shared for example.com (without a trailing dot). But I also don't think we should change that.

@jay
Copy link
Member

@jay jay commented Jan 25, 2022

I checked the spec. The SNI field can be 64K. I don't think it is always limited to the max DNS name size. Name resolving does not always mean DNS and there's no 255 byte limit in host names in URLs.

Yes but currently and practically we are only sending the DNS hostname, which is all that is allowed.

One additional detail struck me with the kept trailing dot. A connection to example.com. (with a trailing dot) will not be reused/shared for example.com (without a trailing dot). But I also don't think we should change that.

agree

@bagder
Copy link
Member Author

@bagder bagder commented Jan 25, 2022

Yes but currently and practically we are only sending the DNS hostname, which is all that is allowed.

curl doesn't have any such limit and can certainly use a longer name than what DNS supports. If you use something else than DNS to resolve (like /etc/hosts for example), the name can be longer for TLS. Practically that might of course be extremely rare. But you can bet that someone will run into that limit if we would impose it! 😀

@bagder
Copy link
Member Author

@bagder bagder commented Jan 26, 2022

This causes a problem for two appveyor builds:

schannel.c(1035,16): warning C4189: 'hostname': local variable is initialized but not referenced 

@bagder
Copy link
Member Author

@bagder bagder commented Jan 26, 2022

I'm getting ready to merge. How do you feel about this @jay ?

bagder added a commit that referenced this issue Jan 27, 2022
The TLS backends convert the host name to SNI name and need to use that.
This involves cutting off any trailing dot and lowercasing.

Co-authored-by: Jay Satiro
Closes #8320
@bagder bagder deleted the bagder/keep-trailing-dot branch Jan 27, 2022
@jay
Copy link
Member

@jay jay commented Jan 27, 2022

'hostname': local variable is initialized but not referenced

Sorry did not see this until after you merged. The debug statement should be left in, it is part of a series of debug messages schannel_connect_step{1,2,3} and so it is problematic to take out just step 2. I will open a PR to readd it, with a (void)hostname to rid of the warning for release builds.

@bagder
Copy link
Member Author

@bagder bagder commented Jan 27, 2022

I figured they're "just debug messages" and as such they tend to linger around for longer than necessary and they're very easy to put back in there when you actually need them for debugging... but if you think it's better to keep it there, I'm fine with it.

jay added a commit to jay/curl that referenced this issue Jan 27, 2022
This is a follow-up to the parent commit 2218c3a which removed the debug
message to avoid an unused variable warning. The message has been
reworked to avoid the warning.

Ref: curl#8320 (comment)

Closes #xxxx
jay added a commit that referenced this issue Jan 28, 2022
This is a follow-up to recent commit 2218c3a which removed the debug
message to avoid an unused variable warning. The message has been
reworked to avoid the warning.

Ref: #8320 (comment)

Closes #8336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP name lookup URL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants