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 invalid SNI handling #100

Merged
merged 1 commit into from
Apr 28, 2022
Merged

Conversation

max-b
Copy link
Contributor

@max-b max-b commented Feb 2, 2022

In #96, @AxbB36 pointed out that this library was sending IP addresses as server name extension values.

There's also another issue where this library was sending empty string values as server name extension values. I'm not sure that RFC 6066 Section 3 is identically as prescriptive about empty values, but the default go implementation does not send empty sni extensions:
https://github.com/golang/go/blob/36b81acfa19d9fedf6a0cd60c394fd7a7703834e/src/crypto/tls/handshake_client.go#L75
https://github.com/golang/go/blob/36b81acfa19d9fedf6a0cd60c394fd7a7703834e/src/crypto/tls/handshake_messages.go#L124-L135

You can see this empty server name extension value in the previous testdata recordings. Here's the client hello from testdata/Client-TLSv12-UTLS-AES128-GCM-SHA256-Chrome-58:

00000000  16 03 01 00 d7 01 00 00  d3 03 03 00 00 00 00 00  |................|
00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000020  00 00 00 00 00 00 00 00  00 00 00 20 00 00 00 00  |........... ....|
00000030  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000040  00 00 00 00 00 00 00 00  00 00 00 00 00 1c 0a 0a  |................|
00000050  c0 2b c0 2f c0 2c c0 30  cc a9 cc a8 c0 13 c0 14  |.+./.,.0........|
00000060  00 9c 00 9d 00 2f 00 35  00 0a 01 00 00 6e 0a 0a  |...../.5.....n..|
00000070  00 00 ff 01 00 01 00 00  00 00 05 00 03 00 00 00  |................|
00000080  00 17 00 00 00 23 00 00  00 0d 00 14 00 12 04 03  |.....#..........|
00000090  08 04 04 01 05 03 08 05  05 01 08 06 06 01 02 01  |................|
000000a0  00 05 00 05 01 00 00 00  00 00 12 00 00 00 10 00  |................|
000000b0  0e 00 0c 02 68 32 08 68  74 74 70 2f 31 2e 31 75  |....h2.http/1.1u|
000000c0  50 00 00 00 0b 00 02 01  00 00 0a 00 0a 00 08 0a  |P...............|
000000d0  0a 00 1d 00 17 00 18 1a  1a 00 01 00              |............|

Using text2pcap we can translate this into a pcap for analysis:

$ text2pcap -T 1011,443 client-hello.hex client-hello.pcap

The wireshark analysis of the tls client hello:

Transport Layer Security
    TLSv1 Record Layer: Handshake Protocol: Client Hello
        Content Type: Handshake (22)
        Version: TLS 1.0 (0x0301)
        Length: 215
        Handshake Protocol: Client Hello
            Handshake Type: Client Hello (1)
            Length: 211
            Version: TLS 1.2 (0x0303)
            Random: 000000000000000000000000000000000000000000000000…
            Session ID Length: 32
            Session ID: 000000000000000000000000000000000000000000000000…
            Cipher Suites Length: 28
            Cipher Suites (14 suites)
            Compression Methods Length: 1
            Compression Methods (1 method)
            Extensions Length: 110
            Extension: Reserved (GREASE) (len=0)
            Extension: renegotiation_info (len=1)
            Extension: server_name (len=5)
                Type: server_name (0)
                Length: 5
                Server Name Indication extension
                    Server Name list length: 3
                    Server Name Type: host_name (0)
                    Server Name length: 0
                        [Expert Info (Warning/Protocol): Vector length 0 is smaller than minimum 1]
                            [Vector length 0 is smaller than minimum 1]
                            [Severity level: Warning]
                            [Group: Protocol]
                    Server Name: 
            Extension: extended_master_secret (len=0)
            Extension: session_ticket (len=0)
            Extension: signature_algorithms (len=20)
            Extension: status_request (len=5)
            Extension: signed_certificate_timestamp (len=0)
            Extension: application_layer_protocol_negotiation (len=14)
            Extension: channel_id (len=0)
            Extension: ec_point_formats (len=2)
            Extension: supported_groups (len=10)
            Extension: Reserved (GREASE) (len=1)

This PR uses the hostnameInSNI function to convert and validate names and then makes marshalling a SNIExtension with an invalid or empty e.ServerName a no-op

SNIExtension was previously marshalling both ip addresses and empty
strings, which are not allowed. See RFC 6066, Section 3.

All of the utls specific testdata replays needed to be rebuilt to
properly accomodate this change since they had previously been including
empty server name extension values

Addresses refraction-networking#96
@gaukas gaukas added the bug Unexpected behavior confirmed and should be fixed label Apr 24, 2022
@max-b
Copy link
Contributor Author

max-b commented Apr 25, 2022

Hey I don't have any write access to this repo at all so if you think this is good/ready I think you'll have to merge it yourself

@gaukas gaukas merged commit 9d36ce3 into refraction-networking:master Apr 28, 2022
@gaukas
Copy link
Contributor

gaukas commented Apr 28, 2022

Thank you @max-b for supporting this project. PR Approved & Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected behavior confirmed and should be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants