-
Notifications
You must be signed in to change notification settings - Fork 292
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
Construct client interface name with additional info #3099
base: main
Are you sure you want to change the base?
Conversation
28998e4
to
abb0556
Compare
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
@@ -121,11 +123,17 @@ internal void TdsLogin( | |||
// length in bytes | |||
int length = TdsEnums.SQL2005_LOG_REC_FIXED_LEN; | |||
|
|||
string clientInterfaceName = TdsEnums.SQL_PROVIDER_NAME; | |||
// Construct client interface name in format: Microsoft SqlClient - {OS}, {Platform} - {architecture} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any security considerations in sending the precise OS version, instead of just the major OS version? A honeypot SQL Server instance would be able to collect these and discover the OS patch level of its clients.
The .NET SDK and CLI allow the user to opt out of sending telemetry. Could we have something similar here - perhaps using this string as the default application name rather than the client interface name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point.
instead of opt out flag we should err on the side of sending the least amount of information which can be significant for us. Would sending only the major version make sense?
The concern with an opt out flag is that if this information is considered sensitive then the default behavior would be considered insecure. That makes this a big problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially, yes. I'm thinking of it similarly to a browser's user agent. On Chrome, mine is Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/<major.minor> (KHTML, like Gecko) Chrome/<major>.<minor> Safari/<major>.<minor>
. I can read that and get very basic OS information and architecture details, with more detailed information about the browser information. I'm not sure what the use case is though - I've assumed that it's a request from the Azure SQL team.
I noticed that the JDBC driver added this functionality in 12.10.0 a month or so ago (microsoft/mssql-jdbc#2561). This uses the application name though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've limited to only fetching OS version and not the OSDescription as it can be very lengthy and detailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; it's mainly the OS version which I'm thinking of. If an attacker has compromised a SQL Server instance, or can intercept traffic to or impersonate a SQL Server instance, they'd be able to retrieve the IP addresses and precise OS version (including a measure of detail on the patch level) for all clients. In that kind of scenario, it'd become easier for an attacker or penetration tester to pivot to the clients with version-specific attacks.
The same approach also applies to the .NET version string, although it might be more difficult - an ASP.NET Core web service connecting to a compromised SQL Server would report its .NET version, which could then be cross-referenced to published CVEs for further exploration.
I'm thinking primarily about an on-prem environment for this kind of attack because that's probably more likely to disable SSL altogether on a local network, or to leave permissions on the SQL Server's SSL certificate wide open.
I've brought up browser user agents because it's an example of the client side reporting a truncated version - it reports an OS version of 10.0, rather than one of 10.0.26100.0. If we think there's a security consideration with sending the OS version, this'd be one way to mitigate it. Using this string as the user-customisable application name also provides a way to disable it entirely (if that's a viable option for SqlClient - it seems to be for JDBC.)
Appends additional information to the client interface name in the
TdsLogin
method that is sent to the server.The Client inferface name format with this change includes:
"Microsoft SqlClient - {OS Name} {OS Version}, {Framework Description} - {Architecture}"
e.g. on Windows , "Microsoft SqlClient - Windows 10.0.26100.0, .NET 8.0.11 - X64"