Skip to content

negotiated_encryption panics when the server declines the requested encryption level #425

Description

@north-echo

Summary

PreloginMessage::negotiated_encryption (src/tds/codec/pre_login.rs:73) calls panic!
when the client asked for EncryptionLevel::On and the server answers Off or
NotSupported:

(EncryptionLevel::On, EncryptionLevel::Off)
| (EncryptionLevel::On, EncryptionLevel::NotSupported) => {
    panic!("Server does not allow the requested encryption level.")
}

Refusing to continue is the correct decision. Doing it with panic! is the problem: a
library should hand the caller an error it can catch, log, and act on, not abort the host
process. As written, any server that declines encryption can terminate the client, and the
application has no way to distinguish "this server refused TLS" from any other crash.

Filed separately from the decoder-panic issue because this one is not malformed input and
the right fix is different: it needs a specific, catchable downgrade error, not a generic
protocol-parse error.

Reachability

Reached during PRELOGIN, so before authentication and before TLS is established. The
response that triggers it is entirely well-formed; no malformed bytes are involved. The
server simply answers ENCRYPT_OFF.

This requires a TLS feature to be enabled, which is the default (default = ["tds73", "winauth", "native-tls"]), so default-configured clients are the affected ones. Verified
against published 0.12.3 with the rustls feature, in a release build:

$ cargo run --release -- encryption-downgrade
thread 'main' panicked at tiberius-0.12.3/src/tds/codec/pre_login.rs:73:17:
Server does not allow the requested encryption level.

Suggested fix

Return a distinct error variant so callers can detect a refused downgrade specifically.
negotiated_encryption currently returns EncryptionLevel, so this changes its signature:

-    pub fn negotiated_encryption(&self, expected: EncryptionLevel) -> EncryptionLevel {
+    pub fn negotiated_encryption(
+        &self,
+        expected: EncryptionLevel,
+    ) -> crate::Result<EncryptionLevel> {
         match (expected, self.encryption) {
             ...
             (EncryptionLevel::On, EncryptionLevel::Off)
             | (EncryptionLevel::On, EncryptionLevel::NotSupported) => {
-                panic!("Server does not allow the requested encryption level.")
+                Err(Error::Protocol(
+                    "server declined the requested encryption level".into(),
+                ))
             }

and propagate at the one call site (src/client/connection.rs:97). If you would rather not
change the signature, an EncryptionLevel sentinel plus a check at the call site works too;
the point is only that the process should not abort.

Found by

Reported by North Echo Security Research.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions