search

microsoft / msquic

Cross-platform, C implementation of the IETF QUIC protocol, exposed to C, C++, C# and Rust.
MIT License
4.02k stars 529 forks source link

SNI is incorrectly set when dialing an IP address #3493

Open MarcoPolo opened 1 year ago

MarcoPolo commented 1 year ago

Describe the bug

When dialing a Quinn server (Rust based) I get a handshake failure on the rust side because it fails to parse the SNI field. The SNI field is set to the ip address of the server, which I don't believe is a valid domain name.

I think we're always setting the RemoteServerName field https://github.com/microsoft/msquic/blob/main/src/core/connection.c#L1980. Maybe we shouldn't in the case that it is an ip address?

Affected OS

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

  1. Have an MsQuic client dial a Quinn server.
  2. Fail to handshake.

Expected behavior

Handshake should succeed.

Actual outcome

fails.

Additional details

Not an experienced C programmer, but happy to attempt to tackle this with some pointers.

anrossi commented 1 year ago

If the server's certificate doesn't include the IP address being used in ConnectionStart in the subject-alt-name field, or the subject-name, then setting the RemoteServerName to the IP address would cause the SNI look-up to fail, IIRC. In your case, could you try setting the remote IP address on the connection using SetParam with QUIC_PARAM_CONN_REMOTE_ADDRESS and set RemoteServerName to NULL in ConnectionStart, and see if that solves the problem? (SetParam must be called before ConnectionStart for this to work)

From: Marco Munizaga @.> Sent: Friday, March 10, 2023 12:26 PM To: microsoft/msquic @.> Cc: Subscribed @.***> Subject: [microsoft/msquic] SNI is incorrectly set when dialing an IP address (Issue #3493)

Describe the bug

When dialing a Quinn server (Rust based) I get a handshake failure on the rust side because it fails to parse the SNI field. The SNI field is set to the ip address of the server, which I don't believe is a valid domain name.

I think we're always setting the RemoteServerName field https://github.com/microsoft/msquic/blob/main/src/core/connection.c#L1980https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fmsquic%2Fblob%2Fmain%2Fsrc%2Fcore%2Fconnection.c%23L1980&data=05%7C01%7Canrossi%40microsoft.com%7C8412720db7a745ad62e708db21a5b12d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638140767761050398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=IsF3jZltz%2Fv%2BxppiuZdGxnp%2FZckzrjmww%2FR%2Bmy7Ragg%3D&reserved=0. Maybe we shouldn't in the case that it is an ip address?

Affected OS

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

  1. Have an MsQuic client dial a Quinn server.
  2. Fail to handshake.

Expected behavior

Handshake should succeed.

Actual outcome

fails.

Additional details

Not an experienced C programmer, but happy to attempt to tackle this with some pointers.

- Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fmsquic%2Fissues%2F3493&data=05%7C01%7Canrossi%40microsoft.com%7C8412720db7a745ad62e708db21a5b12d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638140767761050398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jHkskzdjJ3sIZ%2Fb8mYAZw0atgZf%2BILlH500geKFX2lA%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJ3Z7EFTQ45SIOAAWX5MQZTW3OE6JANCNFSM6AAAAAAVW5JAZE&data=05%7C01%7Canrossi%40microsoft.com%7C8412720db7a745ad62e708db21a5b12d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638140767761050398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1PsqX%2FchdkWn1x0CsMbRbeH%2BOtCbfdfcbenqF2FzH4A%3D&reserved=0. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

MarcoPolo commented 1 year ago

Thank you @anrossi. Using SetParam works great.

You're saying that in some cases it's correct to set the RemoteServerName to the ip address, so there's not much we could do in the ConnectionStart logic. Is that right?

fwiw I was looking for some alternate way to set the server address without using the server name, but I didn't think about SetParam. Would it make sense to add a note to the ConnectionStart docs under ServerName? This is where I first looked.

nibanks commented 1 year ago

I assume this was with OpenSSL? I know Schannel will automatically not put IP addresses in SNI. Perhaps OpenSSL isn't doing this automatically too?

MarcoPolo commented 1 year ago

Yes, OpenSSL.

nibanks commented 1 year ago

Walkthrough

The OpenSSL PAL logic (tls_openssl.c) that sets SNI needs to not set it if an IP literal is passed in.