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

TSSSP module #95

Merged
merged 28 commits into from
Jan 31, 2023
Merged

TSSSP module #95

merged 28 commits into from
Jan 31, 2023

Conversation

RRRadicalEdward
Copy link
Collaborator

This PR adds support for using sspi-rs as a TSSSP module. It adds the possibility to replace the native credssp.dll with sspi-rs for the mstsc. credssp.dll relies on the Schannel and the TSSSP and does not contain any protocol implementations. sspi-rs uses Rust stack instead of Schannel (rustls + picky-rs).
I didn't create another FFI for, but just extended the existing one and improved SspiContext by the new item.

Here are some important points about this implementation.
I've changed the SSPI tables. Now in the reserved fields EncryptMessage/DecryptMessage functions are specified. This is not documented anywhere and was discovered during debugging. The mstsc always uses the reserved3 and reserved4 fields of the functions table as EncryptMessage and DecryptMessage functions.
I also added support for the new attributes for the QueryContextAttributesA/W function:

  • SECPKG_ATTR_STREAM_SIZES
  • SECPKG_ATTR_REMOTE_CERT_CONTEXT
  • SECPKG_ATTR_NEGOTIATION_PACKAGE
  • SECPKG_ATTR_PACKAGE_INFO
  • SECPKG_ATTR_SERVER_AUTH_FLAGS
  • SECPKG_ATTR_CERT_TRUST_STATUS
  • SECPKG_ATTR_CONNECTION_INFO

Small FFI changes:

  • pf_qop parameter of the DecryptFunction can be null if this library is used as a CredSsp security package.
  • Improved the copy_to_c_sec_buffer function: now it also copies the buffer type and handles when the buffer pointer is null.

At this point, we use the rustls for TLS connection establishing and traffic encryption/decryption. I've placed the TLS part into a separate module so we can easily add native Schannel support.

Used Docs & References:

* add CredSsp to the SspiContext;
* add SspiCredSsp. Implement Sspi, SspiImpl, SspiEx traits for it (with a lot of TODOs). TLS connection establishing works (using rustls);
* SECPKG_ATTR_STREAM_SIZES
* SECPKG_ATTR_REMOTE_CERT_CONTEXT
* SECPKG_ATTR_NEGOTIATION_PACKAGE
* SECPKG_ATTR_PACKAGE_INFO

sspi: add query_context_remote_cert and query_context_negotiation_package methods to the Sspi trait. Implement them for all security packages;

sspi: sspi_cred_ssp: finish implementation;
…ECPKG_ATTR_CONNECTION_INFO;

fixed SSPI tables: not mstsx not crash with the memory error
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Should we also bump the version?

ffi/src/sec_winnt_auth_identity.rs Outdated Show resolved Hide resolved
ffi/src/sec_winnt_auth_identity.rs Outdated Show resolved Hide resolved
ffi/src/security_tables.rs Outdated Show resolved Hide resolved
src/credssp/sspi_cred_ssp/tls_connection.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
ffi/src/sec_winnt_auth_identity.rs Outdated Show resolved Hide resolved
src/credssp/sspi_cred_ssp/mod.rs Outdated Show resolved Hide resolved
ffi/Cargo.toml Outdated Show resolved Hide resolved
@CBenoit
Copy link
Member

CBenoit commented Jan 26, 2023

I’m unsure whether using a new feature flag is the right choice (this requires enabling / disabling the feature depending on the targeted platform). What about using #[cfg(windows)] directly?

@RRRadicalEdward
Copy link
Collaborator Author

I’m unsure whether using a new feature flag is the right choice (this requires enabling / disabling the feature depending on the targeted platform). What about using #[cfg(windows)] directly?

I think a new feature flag makes better intention. It makes code harder to read when there are a bunch of #[cfg(target)]. You would think - are different features related if they compile under the same target?
TSSSP use case is only to build sspi-rs and use it as credssp.dll in mstsc. So I wouldn't be used in crates.
I will change to use #[cfg(windows)] if you still insist on it.

@CBenoit
Copy link
Member

CBenoit commented Jan 26, 2023

TSSSP use case is only to build sspi-rs and use it as credssp.dll in mstsc. So I wouldn't be used in crates. I will change to use #[cfg(windows)] if you still insist on it.

Okay, leave it that way then.

Currently, CI builds sspi-rs with --all-features flag on all systems which makes the build fail cause tsssp can be only enabled on Windows. We should modify CI to build with the feature enabled only on Windows. I want to do it in a separate PR to not mix this PR, but I'm not sure if it's possible to merge a PR when there are some failed runners.

As you said, this have to be fixed in this PR. I can push to your branch and fix that if you want.
I can do that once remaining conversations are resolved.

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! @RRRadicalEdward
I’ll patch the ci file and merge

@CBenoit CBenoit requested review from a team as code owners January 27, 2023 23:16
@CBenoit
Copy link
Member

CBenoit commented Jan 27, 2023

Hi @RRRadicalEdward

Apparently there are tests failing under Linux for sspi-ffi:

failures:

---- sec_pkg_info::tests::enumerate_security_packages_a stdout ----
thread 'sec_pkg_info::tests::enumerate_security_packages_a' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `5`', ffi/src/sec_pkg_info.rs:339:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- sec_pkg_info::tests::enumerate_security_packages_w stdout ----
thread 'sec_pkg_info::tests::enumerate_security_packages_w' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `5`', ffi/src/sec_pkg_info.rs:357:13

@RRRadicalEdward
Copy link
Collaborator Author

Hi @CBenoit! I fixed the units tests, but CI fails due to a CI itself related issue

@CBenoit
Copy link
Member

CBenoit commented Jan 30, 2023

Thank you! I’m looking into it

@CBenoit
Copy link
Member

CBenoit commented Jan 30, 2023

When I run the command manually on Windows, I indeed get the error locally as well. It says the specified executable is not a valid Win32 application. 🤔
You can reproduce with cargo test --manifest-path ffi/Cargo.toml.

@CBenoit
Copy link
Member

CBenoit commented Jan 31, 2023

I still don’t understand why it’s failing only on Windows. Since it’s not happening in CI only, if you find anything let me know! For now I asked our Devops to relax a bit our requirements for this check and I’ll merge in order to avoid blocking this PR longer.

For the record, here is the error I get when run in my Windows VM:
image

@CBenoit CBenoit merged commit 4f5440e into master Jan 31, 2023
@CBenoit CBenoit deleted the tsssp branch January 31, 2023 02:17
@CBenoit
Copy link
Member

CBenoit commented Jan 31, 2023

Fixed CI in #98

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

Successfully merging this pull request may close these issues.

3 participants