Don't set SNI by default if hostname is not dNS name; fixes #8083 by iamihalcea · Pull Request #8175 · openssl/openssl

iamihalcea · 2019-02-06T21:15:49Z

Don't set SNI by default (i.e. when -servername is not set) in clientHello if hostname is not dNS name

CLA not yet completed, will do asap

Checklist

documentation is added or updated
tests are added or updated

vdukhovni

The code looks fine. Please address the requested changes in the comments and documentation. Finally, I might note that "all-numeric" does not take "-" into account. Thus "12-34.56-78" will be considered "all-numeric", which it is not, but I don't expect we need to handle these by adding all_numeric = 0 when a hyphen is found.

vdukhovni · 2019-02-07T18:33:03Z

The second sentence of the second comment paragraph is about X.509 and does not apply here. Delete it.

vdukhovni · 2019-02-07T18:39:27Z

This text is surely wrong. When -servername <name> is given, then that name and not the name from -connect will I expect be sent. Also the above sentence is poorly phrased. Could use some grammar editing.

Updated - please have a look and let me know if it makes more sense now. Had to rearrange some bits.

vdukhovni

Provisional approval, modulo others not viewing the style nits as "must fix".

vdukhovni · 2019-02-08T18:49:16Z

Cosmetic: (@mattcaswell do we care?) We don't normally camel case internal functions in OpenSSL. To comport with the style of the existing code, perhaps this should be just is_dns_name()? Secondly, perhaps this function should be in apps.c just in case some other app will find use for it? I'm approving the PR, but if Matt or someone else wants the style issued addressed, please speak up...

There are other names like this already in this file (e.g. ldap_ExtendedResponse_parse) so I don't find it so jarring in this case. I think the function is fine in this file for now. We can always move it to apps.c later if appropriate.

mattcaswell · 2019-02-11T10:26:16Z

Close/reopen to kick CLA bot

mattcaswell · 2019-02-11T10:37:07Z

EBCDIC issue here too

mattcaswell · 2019-02-11T10:38:03Z

Drop XXX: from here

mattcaswell · 2019-02-11T10:42:36Z

MAX_LABEL_LENGTH, i, length and label_length all seem to be more appropriately declared as size_t.

In cn2dnsid the original input is ASN.1 string, and the API for converting those to UTF8 uses integer lengths, so size_t was not an option there. Here, the hostname comes from the command-line, so size_t is possible, but does not add any measure of security, I am not aware of any OS where one can execute a program with a 2GB command-line argument... Still this code could size_t just to "unask" the question.

I'll change just to make it consistent with the "contents" of the variables

Right - my point wasn't really about security. More about using an appropriate and consistent type.

mattcaswell · 2019-02-11T10:44:46Z

Oh...one other point. Please rebase this to remove the "merge" commit. We don't allow merge commits.

iamihalcea · 2019-02-11T12:47:21Z

Oh...one other point. Please rebase this to remove the "merge" commit. We don't allow merge commits.

Fixed

vdukhovni

One minor style issue. I'm leaving it to Matt to decide how much EBCDIC support is appropriate to require here.

vdukhovni · 2019-02-12T06:03:16Z

Delete the blank line here, keeping all the declarations together.

mattcaswell

Approved subject to the minor styling nit that @vdukhovni noted being fixed.

vdukhovni

Thanks for your contribution.

kaduk

Most of this could have been fixed by the committer, but I think we need to check the length of all labels and not just the last one.

kaduk · 2019-02-14T19:24:32Z

-            servername = (host == NULL) ? "localhost" : host;
-        if (!SSL_set_tlsext_host_name(con, servername)) {
+        if (servername == NULL) {
+            if(host == NULL || is_dNS_name(host)) 


nit: missing space in if (

kaduk · 2019-02-14T19:26:26Z


+/*
+ * Host dNS Name verifier: used for checking that the hostname is in dNS format 
+ * before setting it as SNI


nit: end the sentence with a period.

kaduk · 2019-02-14T19:32:43Z

+                && host[i + 1] != '.'
+                && host[i - 1] != '-'
+                && host[i + 1] != '-') {
+                label_length = 0;


Don't we need to check label_length against MAX_LABEL_LENGTH here? At present I can only see us making that check for the final label (which is probably a TLD anyway).

The check is done for all labels - see end condition on the for.

So it is, my apologies. (It seems that I got interrupted mid-review and lost some state.)
That said, the RFC allows a label of length exactly 63, but my first read of the for loop (and this check) would be that it would reject such a label.

This does, though, suggest that we may want some test cases with 63- and 64-byte labels.

I think attempting to test this would need the test itself to make calls across the network so I'll avoid doing that, but if there's a way to mock said calls I could give it a go - @mattcaswell ?

We don't really have a good way to do that at the moment.

Looking at the comment by @kaduk I also agree that the code seems to reject 63-octet labels, which are rare, but legal.

kaduk · 2019-02-14T19:33:30Z

+    }
+
+    /* dNS name must not be all numeric and labels must be shorter than 64 characters. */
+    isdnsname &= !all_numeric && !(label_length == MAX_LABEL_LENGTH);


I've been thinking of these variables as booleans, but &= is a bitwise operator.
And we don't do anything with isdnsname afterward anyway; can't we just return this expression?

It is indeed a bitwise operator, but the effect is very much the same.
And I didn't return isdnsname && something_else because for a function named is_dNS_name, I find that confusing. I could change the name of the isdnsname variable, but efficiency-wise, the compiler should take care of it.

kaduk

Having thought about this overnight, in the broader context of where this fits in, I don't think we necessarily need to hold it up on the precise details; even as-is it's a bit improvement over the previous status quo.

richsalz · 2019-02-15T17:12:45Z

The command-line is also used for testing. Using the hostname automatically as the SNI if it looks like a hostname is the requirement. Enforcing all DNS requirements is not.

Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8175) (cherry picked from commit 8e98105)

mattcaswell · 2019-02-19T17:37:19Z

Pushed to master and 1.1.1. Thanks!

Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8175)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Feb 6, 2019

kroeckx reviewed Feb 6, 2019

View reviewed changes

Comment thread apps/s_client.c Outdated

vdukhovni requested changes Feb 7, 2019

View reviewed changes

vdukhovni approved these changes Feb 8, 2019

View reviewed changes

mattcaswell closed this Feb 11, 2019

mattcaswell reopened this Feb 11, 2019

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Feb 11, 2019

mattcaswell added branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) labels Feb 11, 2019

mattcaswell requested changes Feb 11, 2019

View reviewed changes

iamihalcea force-pushed the master branch from 51a9c67 to d61d0d3 Compare February 11, 2019 12:46

vdukhovni requested changes Feb 12, 2019

View reviewed changes

mattcaswell approved these changes Feb 12, 2019

View reviewed changes

Ionut Mihalcea added 4 commits February 12, 2019 11:02

Don't set SNI by default if hostname is not dNS name

575e575

Fix docs for s_client -servername; Clean up comment on is_dNS_name

7d1cf58

Switch vars to size_t; clean up comment

b1af78a

Fix cosmetic nit

3842efb

iamihalcea force-pushed the master branch from 7bae1de to 3842efb Compare February 12, 2019 11:04

vdukhovni approved these changes Feb 12, 2019

View reviewed changes

kaduk suggested changes Feb 14, 2019

View reviewed changes

kaduk approved these changes Feb 15, 2019

View reviewed changes

mattcaswell closed this Feb 19, 2019

levitte pushed a commit that referenced this pull request Feb 19, 2019

Don't set SNI by default if hostname is not dNS name

8e98105

Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8175)

jcrowgey mentioned this pull request Feb 27, 2019

Non-compliant SNI hostname #8083

Closed

sam-github mentioned this pull request Jun 3, 2020

A possible TODO list for new (or current) contributors nodejs/node#33715

Closed

Uh oh!

Conversation

iamihalcea commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Feb 11, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Feb 11, 2019

Uh oh!

iamihalcea commented Feb 11, 2019

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamihalcea commented Feb 6, 2019 •

edited

Loading

iamihalcea Feb 14, 2019 •

edited

Loading