Smartcard implementation with PCSCLITE_CSOCK_NAME

[zorgluf]

Mainly based on the great work from jsorg71 (https://github.com/neutrinolabs/xrdp/pull/963). Fix issues with recent version of pcsc (1.8.8). Merge with a more recent version of xrdp (0.9.14).

Made it working on RHEL7.9 with a gemalto MD840 and omnikey reader. Feel free to suggest improvement, my dev skills are pretty low.

[matt335672]

Hi @zorgluf

Thanks for taking the time to contribute this, and for getting involved.

I'll dig out a spare yubikey and take a look at this when I can. At the moment however, I'm a bit snowed under with other PRs, which is great, but means I'll take a while to get to this, for which I apologise.

I see you've got a couple of minor issues above at the moment:-

1. There's a conflict with a couple of files.
2. The FreeBSD CI check is failing

I also see you've merged in to 0.9.14 above, which is fine, but will lead to a complicated git history when this (eventually) gets to devel.

Are you able to remove the last commit above and rebase the others on the latest devel? I'll be happy to give you a hand with this if you need it.

[aquesnel]

Hi @zorgluf I'd be happy to help test this out too, but I don't know how to setup xrdp and a client to use a smart card for authentication. Could you please help by writing up your server and client configuration and maybe adding it to the wiki pages?

[zorgluf]

Hi @zorgluf

Thanks for taking the time to contribute this, and for getting involved.

I'll dig out a spare yubikey and take a look at this when I can. At the moment however, I'm a bit snowed under with other PRs, which is great, but means I'll take a while to get to this, for which I apologise.

I see you've got a couple of minor issues above at the moment:-

1. There's a conflict with a couple of files.
2. The FreeBSD CI check is failing

I also see you've merged in to 0.9.14 above, which is fine, but will lead to a complicated git history when this (eventually) gets to devel.

Are you able to remove the last commit above and rebase the others on the latest devel? I'll be happy to give you a hand with this if you need it.

Thanks for the review ! The FreeBSD CI check is now OK. I am not familiar with complexe manipulation with git commit, so I didn't manage to remove cleanly the last commit. Instead, I merge the devel branch into mine and create a new commit. Is it enough ?

[zorgluf]

Hi @zorgluf I'd be happy to help test this out too, but I don't know how to setup xrdp and a client to use a smart card for authentication. Could you please help by writing up your server and client configuration and maybe adding it to the wiki pages?

Hi @aquesnel This pull request is not about smart card authentication against xrdp, but forwarding a smartcard reader through the rdp session to the remote host. A basic test for this feature is to use the pcsc-tools on the server : a pcsc_scan command should detect the readers and card inserted on your client side (don't forget to activate smartcard reader forwarding on your xrdp client). But this test is so basic that it cannot test all the functionnalities. Fully testing this feature highly depends on your smartcard, since you will need :

• for some exotic readers drivers on your client side
• the middleware of your smartcard installed on the server side, with the pcsc-lite package. The pcsc-lite lib will read the PCSCLITE_CSOCK_NAME env variable, which communicate with the client reader. The middleware on server side communicate with the pcsc-lite lib, at least all middleware I am aware of working on linux. Hope it answer your questions. If it's still relevant, I can add this information on wiki pages, just tell me where...

[avolkov-astra]

Hi. Here are these changes rebased onto devel branch: https://github.com/avolkov-astra/xrdp/commits/pcsc_devel This way they are easier to review. The correctness of rebasing can be checked by comparing final source trees (e.g. with kdiff3) The most ineteresting difference is the line "scard_send_cancel(0, context->context, context->context_bytes);" which was removed by a commit of jsorg71, but returned later, probably by mistake.

[matt335672]

Hi @avolkov-astra

Thanks for taking the trouble to do this.

@zorgluf - thank you for sticking with this. As @avolkov-astra says, it's well worth learning the basics of github rebasing as it makes it easier to communicate with folks both here and on gitlab. Furthermore, when you get the hang of that, a lot of the weirdness of git will make more sense (it did for me anyway).

There's a description of rebasing here. It's a bit dry though. I'd recommend just having a play with a copy of a repo and the git rebase -i command which helps a lot.

[matt335672]

I've just started taking a look at this, and working out where we are - I haven't tried it yet.

@zorgluf - as far as I can tell, your main changes to @jsorg71's original branch are those in commit 2fe0a4ebd7d521555f6fc1bcd8321fd524b35662. Is that correct?

[zorgluf]

@zorgluf - as far as I can tell, your main changes to @jsorg71's original branch are those in commit 2fe0a4e. Is that correct?

Correct. Not so much changes. The other commit add comments and some raw int value converted to const variables. 98d5c8b remove also a part of dead code that cannot be accessed in code logic. Sorry for the mess with all those commit. If it's not reviewable enough, I can try to push an other pull request with less commits.

[EmperorArthur]

I'd git isn't working for you, here's the simple solution.

• First, clone the official devel branch to a different folder.
• Make a new branch in that new folder.
• Use git remote to add your GitHub repository to the folder.
• Now use a diff tool to find everything that's changed between your folder and the devel folder.
• Either copy the files over, or manually cut and paste the changes. Committing at logical times.
• Do a git push to the new branch in your GitHub repo.

[matt335672]

@zorgluf - I'm having trouble getting pcsc_scan working with a USB-A Yubikey 5 (which is all I've got).

Before I dive in with a debugger, I just thought I'd check with you that I'm not doing anything stupid.

I'm running off of @avolkov-astra's branch at the moment.

On a native Mint machine (pcsc-tools 1.5.5-1) I'm getting this:-

$ pcsc_scan -rv
Using reader plug'n play mechanism
Scanning present readers...
0: Yubico YubiKey OTP+FIDO+CCID 00 00

On an xrdp session to an Ubuntu 20.04 VM (pcsc-tools 1.5.5-1) the pcsc_scan -rv hangs, generating no output.

• I've redirected smartcards in mstsc.exe
• PCSCLITE_CSOCK_NAME is set to /tmp/.xrdp/xrdp_pcsc_socket_11 in my session.
• The last lines in the chansrv log are as follows:-

  [20210611-14:55:43] [DEBUG] [scard_process_msg(smartcard_pcsc.c:1972)] scard_process_msg: command 0x0013
  [20210611-14:55:43] [INFO ] [scard_process_msg(smartcard_pcsc.c:2062)] scard_process_msg: CMD_WAIT_READER_STATE_CHANGE
  [20210611-14:55:43] [DEBUG] [scard_process_cmd_wait_reader_state_change(smartcard_pcsc.c:1693)] scard_process_cmd_wait_reader_state_change:
  [20210611-14:55:43] [DEBUG] [scard_process_cmd_wait_reader_state_change(smartcard_pcsc.c:1695)] scard_process_cmd_wait_reader_state_change: timeOut 2
  [20210611-14:55:43] [DEBUG] [scard_pcsc_get_wait_objs(smartcard_pcsc.c:450)] scard_pcsc_get_wait_objs:
  [20210611-14:55:43] [DEBUG] [get_timeout(chansrv.c:153)] get_timeout:
  

Is there anything else obvious I need to do which I haven't?

Thanks.

[matt335672]

@zorgluf - what platform are you running on?

I see you've added changes for pcsc-lite 1.8.8. I'm running 1.8.26 (on Ubuntu 20.04) and this won't work.

The main problem seems to be that in the pcsc-lite 1.8.26 in winscard_clnt.c, an additional CMD_WAIT_READER_STATE_CHANGE message is sent here.

Your routine which is correspondingly called here doesn't handle this well. There's no return message, and also the timeout read from the stream doesn't exist.

Are you able to try running with pcsc-lite 1.8.26? The error can then be demonstrated by running pcsc_scan with no smartcard at all attached.

[zorgluf]

@matt335672 thanks for the report. I was using rhel7 for my dev, and yes it's probable due to a newer pcsclite version introducing a new behavior badly handled, since my yubikey works on a 1.8.8 version. Thanks for pinning the part of the faulty code, I will propose a fix. Unfortunatly, it will take me a few days, quite busy at the moment, but will definitively do it.

[matt335672]

Thanks @zorgluf

I think I owe you an apology. The faulty routine scard_process_cmd_wait_reader_state_change() isn't anything to do with you - it was added as part of commit fe60eebe2ebff894b5c33361fff2a8ac7eed931d.

A couple of things which may be useful to you:-

• If you're happier working on RHEL/CentOS/Etc, RHEL8 has pcsc-lite 1.8.23. There are minor differences with 1.8.26 but from a first glance nothing looks that different.
• If you rebase to the latest xrdp devel, or merge in the latest changes, you'll find that the configure option --enable-xrdpdebug has been replaced with --enable-devel-all. This includes extra checks when developing which may be useful to you. The routine scard_process_cmd_wait_reader_state_change() currently reads a value timeOut from the stream which isn't there. With --enable-devel-all this is now detected at runtime.

If you're interested in the latter option, I've set up a branch in my own repository which rebases the work by @avolkov-astra onto the latest devel. You can use it in your own working repository with these commands:-

git remote add matt335672 https://github.com/matt335672/xrdp.git
git fetch matt335672
git checkout -b aa_pcsc_devel --track matt335672/aa_pcsc_devel

If you go down this route, at some point you'll be able to overwrite your existing pcsc_0.9.14 branch with these commands (reference here):-

git checkout pcsc_0.9.14
git tag old_pcsc_0.9.14  ; # So we've got a copy
git reset --hard aa_pcsc_devel

You can then update your PR in git hub using git push as normal, but specifying the -f flag.

I appreciate that's a lot to take in! If you've got any questions, please let me know.

[matt335672]

A quick addition to the above; at the moment my branch aa_pcsc_devel isn't passing CI checks.

That's probably something you don't need to worry about yet; we can fix that later.

[zorgluf]

@matt335672 thank you very much for your time on this request, it helps a lot. I followed your instructions and add 2 commits from your rebased repo. There was in fact two issues :

• A change of behavior in pcsc lite libraries. Now the code is tracking protocol version to handle both behaviors
• Redhat package of pcsc-lite has a patch to rise the PCSCLITE_MAX_READERS_CONTEXTS value of pcsc-lite. So now the code read this value form the devel version of the lib (requirement added to configure.ac). I also modified the Makefile.am in chansrv, maybe need some review (hard coded /usr/include directory).

Code tested on my original RHEL7 server and also on ubuntu 20.04.

[matt335672]

Hi @zorgluf

Thanks for sticking with this.

I'm not at all happy (sadly) with the 2nd of your commits I'm afraid. The reason is that if we're going to pull in the pcsc development package, we should really use the library where possible in the same way as the daemon. That's a lot of work! The current approach is a bit flaky in that it won't cope well with version changes, but we don't have a dependency.

I'd suggest (instead), just increasing the MAX_READERS value and adding a comment, e,g, :-

/* Fedora increases the MAX_READERS value from the pcsc-lite default */
# define MAX_READERS 48

You could also add a log message to the test in scard_readers_to_list() if we overrun this limit. So replace this code:-

            reader_index++;
            hold_reader = uds_client->readerStates[reader_index];
            if (reader_index > (MAX_READERS - 1))
            {
                return 0;
            }

with:-

            reader_index++;
            if (reader_index > (MAX_READERS - 1))
            {
                 LOG(LOG_LEVEL_WARNING,  "Max smart card readers exceeded");
                return 0;
            }
            hold_reader = uds_client->readerStates[reader_index];

I've moved the assignment to hold_reader, as if the overflow occurs at the moment the behaviour is undefined.

[zorgluf]

Hi @matt335672 , Thank you also to stick to this long pull request...

The problem is that the readerStates array (of size PCSCLITE_MAX_READERS_CONTEXTS) is send to the local pcsc lib and this lib is expecting the exact size of this array (there is no size check). On xrdp size :

reader_state_bytes = sizeof(uds_client->readerStates);
init_stream(out_s, reader_state_bytes);
out_uint8a(out_s, uds_client->readerStates, reader_state_bytes);

and in the pcsc lib : rv = MessageReceive(&readerStates, sizeof(readerStates), dwClientID); If the size of the array differs (below or over), it doesn't work (tested).

I really don't have any more idea to fix this. Even between RHEL7 and RHEL8, this value change... Maybe make this dependency and the smartcard redirection channel optional in configure ?

[matt335672]

Thanks for your clear explanation of the MAX_READERS_CONTEXTS problem.

I think the situation is as follows. Please correct me if I'm wrong:-

• At the moment smartcard processing doesn't work in the xrdp devel branch
• The current design aims to emulate pcsd without introducing a dependency on pcsd
• It's not really practical to emulate pcscd without having the pcsc-devel package installed

If that's the case, a sensible way forward seems to be to be:-

• Introduce an optional dependency on pcsc-devel, so that users who do not need smartcard support (currently all of them) do not have to introduce the dependency.
• Disable smartcard processing entirely if pcsc-devel is not present
• Over time, replace some of the pcscd emulation code in xrdp with calls to the pcsc library, to reduce our dependency surface.

I'm not expecting you by any means to attempt this all on your own by the way - I'll be happy to help.

If that sounds reasonable to you, we can broaden this discussion out.

[EmperorArthur]

Might I suggest that everything except the MAX_READERS_CONTEXTS be merged. That may give anyone using the official (non OS) release a working feature.

While I have not examined this in detail, changing a protocol implementation without adjusting a version or otherwise giving an indication to the other party is what I would consider a major bug.

As I said, I have not looked at this in detail. However, if xrdp is sending data to such a buggy program, then you will have to deal with the potential for the client and server to have different protocol versions. And a host of other issues.

While I suggest ignoring it for this PR, you can also solve the issue by dynamically determining size at runtime. Just start by sending one and keep incrementing the number until the recipient stops failing.

[matt335672]

Thanks for the suggestion @EmperorArthur

It's worth pointing out that the interface we're implementing here is not public - it's built in to the pcsc-lite project. I can see why the xrdp design has taken the route it has, but I'm personally not happy with it, hence my suggestion that we move towards using the pcsc-lite library if possible.

Also, I don't believe the approach you are also suggesting is going to work, as the data in question is sent in response to a client request. On the first failure we've lost the client.

[zorgluf]

I withdrew the patch with dependency against libpcsclite. Instead I suggest to fix the MAX_READERS value to 16, which is the "official" value PCSCLITE_MAX_READERS_CONTEXTS in libpcsclite. For RHEL releases, this value has to be handle externaly with a patch, as what is made actually with libpcsclite. At least it makes a "working version", that might interest some users.

@matt335672 : I totaly agree with you, the interface used to handle smartcard communication (internal socket protocol of libpcsclite and unsupported interface) is not a good and long term way to get the functionality. But the interface is quite close the way RDP handle smartcard redirection, maybe it was more convenient ? An other way wight be to develop a PCSC virtual driver (like http://frankmorgner.github.io/vsmartcard/virtualsmartcard/README.html). But at first lecture, interfacing pcsc driver primitives and RDP smartcard channel need major rewrite. I will have a look... for an other pull...

[matt335672]

Hi @zorgluf - thanks for sticking with this. I can't get to my Windows machine at the moment to test this, but I will do so as soon as I can.

@metalefty , @jsorg71 - I need some input from one of you on this. This PR moves us forward from where we are with smartcard support (which doesn't work at all at the moment), but more work will be needed in the future to support big-endian machines, and possibly to use pcsc-lite libraries directly to reduce the maintenance overhead (this is discussed in more detail above). Assuming the testing works out I'm happy to merge this as it is, but I'd appreciate your input before I do.

[metalefty]

@matt335672 Unfortunately, I've never used the smartcard feature. I can do very few things on this.

[matt335672]

@metalefty - understood

Would you be happy if I worked with @zorgluf on merging this straight after 0.9.17 is released? There's a lot to do here, but I think having something working for some people would be a great step forward.

[metalefty]

Yes, I'm happy with that.

[spstarr]

What is the status of this PR?

[matt335672]

It's not been looked at I'm afraid. We're looking at other things at the moment for the next major release, and there's no effort available to working on this one.