Skip to content

System provided scards support #252

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

Merged
merged 5 commits into from
Jun 11, 2024
Merged

Conversation

TheBestTvarynka
Copy link
Collaborator

@TheBestTvarynka TheBestTvarynka commented Jun 10, 2024

Hi,
In this PR I've implemented the system provided smart cards support. Now the user can use real smart cards using our library as well as emulated ones.

More information you can read in those pull requests descriptions:

Implementation notes

  1. The sspi loads all additional libraries (like libpcsclite or winscard.dll) in the runtime. Related discussion: PCSC-lite bindings #246 (comment).
  2. The system scards implementation is hidden under the WinScard and WinScardContext. The system scard is just another implementation of these traits. Our custom FFI layer works only with high-lever Rust API. It gives us a few advantages:
    • Easier to maintain the system scard implementation.
    • Easier to maintain our FFI layer.
    • Easier to debug.
    • Fewer if/match expressions in the FFI layer.
  3. We've introduced the RequestBufferType and OutBuff types to improve the memory management.

Configuration

The user should set the WINSCARD_USE_SYSTEM_SCARD env variable with the value set to true to use system-provided smart cards.

The sspi library uses the PCSC Lite library on Linux and macOS to access the system-provided smart card. This library is loaded in the runtime. The user can use the PCSC_LITE_LIB_PATH env variable to specify the libpcsclite location.
The standard WinSCard will be used on Windows. But the user has an ability to confiture the winscard.dll location using the WINSCARD_LIB_PATH env variable. It can be helpful in situations when the original winscard.dll is hooked and we need to call the original one anyway.

Demo

Using mstscex:

Windows.11.x64.smart.card.Running.-.Oracle.VM.VirtualBox.2024-06-10.18-18-28.mp4

Using FreeRDP:

Windows.11.x64.smart.card.Running.-.Oracle.VM.VirtualBox.2024-06-10.18-15-43.mp4

Note

Currently, only the winscard part pf the sspi-rs is modified. Currently, our CredSSP implementation can use only the emulated smart card. We plan to improve the CredSSP module in next iterations.

Docs & references:

cc @awakecoding

@TheBestTvarynka TheBestTvarynka self-assigned this Jun 10, 2024
@TheBestTvarynka TheBestTvarynka marked this pull request as ready for review June 10, 2024 15:20
@TheBestTvarynka TheBestTvarynka requested a review from CBenoit June 10, 2024 15:20
@CBenoit
Copy link
Member

CBenoit commented Jun 10, 2024

@TheBestTvarynka Do you want to rebase or squash this PR? If rebasing, can you reword the commits? Thank you!

@awakecoding
Copy link
Contributor

On Windows, with mstscex, I'm always getting E_INVALIDARG (0x80070057):

image

I'm trying to use a Windows virtual smartcard device with this in my .RDP file, similar to what it would be without sspi-rs:

username:s:@@BooZeoydRgfEwgrQROH1cL4sWZlA
ClearTextPassword:s:12345678

My environment variables:

MSRDPEX_LOG_LEVEL=DEBUG
MSRDPEX_LOG_ENABLED=1
MSRDPEX_CREDSSP_DLL=C:\wayk\dev\sspi-rs\target\release\sspi.dll
MSRDPEX_WINSCARD_DLL=C:\wayk\dev\sspi-rs\target\release\sspi.dll
SSPI_LOG_LEVEL=trace
SSPI_LOG_PATH=C:\wayk\dev\sspi-rs\target\release\sspi.log
SSPI_KDC_URL=IT-HELP-DC.ad.it-help.ninja
WINSCARD_USE_SYSTEM_SCARD=1

@awakecoding
Copy link
Contributor

From my testing in Windows, it is still checking all the WINSCARD_ environment variables required for virtual smartcards. I would have expected WINSCARD_USE_SYSTEM_SCARD=1 to be the only one required? I also expects the certificate file and key which makes no sense for the system-provided smartcard.

@TheBestTvarynka
Copy link
Collaborator Author

TheBestTvarynka commented Jun 10, 2024

Currently, only the winscard part pf the sspi-rs is modified. Currently, our CredSSP implementation can use only the emulated smart card. We plan to improve the CredSSP module in next iterations

@TheBestTvarynka
Copy link
Collaborator Author

@TheBestTvarynka Do you want to rebase or squash this PR? If rebasing, can you reword the commits? Thank you!

I would like to rebase this PR. Yes, I'll reword commits

To support system-provided smart cards, we need to somehow access
them from `sspi-rs`. On Windows, we will use native WinSCard API.
We plan to use the `pcsc-lite` library on Linux and macOS; to do so,
we need these bindings.

_Why can't we just use the pcsc-sys crate?_

The main reason is that it links with the PCSC framework on macOS
which was deprecated for years.

_How the bindings work?_

The `sspi-rs` loads the pcsc-lite in the runtime, builds the functions
table, and returns it to the user. It allows us to:

* simplify build process;
* replace the library in the runtime.
Added new methods for the `WinScard` and `WinScardContext` traits and
implement them for the emulated smart card:

`WinScard` trait:

* `control_with_output`;
* `reconnect`;
* `get_attribute`;
* `set_attribute`;
* `disconnect`.

`WinScardContext` trait:

* `list_cards`;
* `list_reader_groups`;
* `calcel`;
* `get_status_change`;
* `get_card_type_provider_name`;
Now we can call system API to access system-provided smart cards. Also,
the dynamic `winscard.dll` loading is implemented. We can't always use
the `winscard-sys` crate, because if the `winscard.dll` is hooked
(in the case of https://github.com/devolutions/msrdpex/), then the
`winscard-sys` will just redirect calls to our library and we'll get
a stack overflow.

The system scards implementation is hidden under the `WinScard` and
`WinScardContext` traits. The system scard is just another implementation
of these traits.
_Implementation notes_

* Move the `clippy::undocumented_unsafe_blocks` lint to the `ffi/winscard` module
  lever instead of `ffi/winscard/system_scard`. Now all winscard-related code is checked.
* Refactor the code and remove all panics.
* Improve the memory management.

_Memory management improvements_

Many winscard functions can treat the resulting data differently based on the input parameters.

* When the input data pointer is `NULL`, we should write out data len in the input data
  len pointer.
* When the input data length pointer value is `SCARD_AUTOALLOCATE`, we should allocate
  memory by ourselves, cast the output data pointer to pointer to pointer to memory, and
  write the memory address.
* Otherwise, we copy the data and data length into corresponding pointers.

And there are a lot of functions with such behavior. To simplify this process and reduce
many `if`s/`match`s, We've introduced the `RequestBufferType` and `OutBuff` types.
They are well documented, so you can read its description in the code.

Moreover, we put the effort into keeping all memory management inside the `WinScardContexthandle`
structure implementation. Now it's easier to manage the memory and search for problems if there are any.
…f_wide` functions (#251)

Previously, these function always check that the data pointer is not
`null`. But actually we need to check it only in the `OutBuffer::Allocated`
case. Move this check into the corresponding `match` branch and updated
the safety comment.

Improve logging for the `WinScardContextHandle` and `WinScardHandle`.
@TheBestTvarynka TheBestTvarynka force-pushed the feat/system-provided-scards-support branch from d2257b4 to 041664c Compare June 10, 2024 21:13
@TheBestTvarynka
Copy link
Collaborator Author

If rebasing, can you reword the commits?

@CBenoit, done!

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.

LGTM! I see no problem in merging this now, so I’ll go ahead. As always, very good job!

@CBenoit CBenoit merged commit 27cbbdb into master Jun 11, 2024
40 checks passed
@CBenoit CBenoit deleted the feat/system-provided-scards-support branch June 11, 2024 12:01
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