Smartcard implementation with PCSCLITE_CSOCK_NAME #1825
Conversation
sesman/chansrv/smartcard.c
Outdated
| val = recv_ior->cbPciLength > 0 ? 0x0002000c : 0; | ||
| val = recv_ior_is_null ? 0 : 0x00020008; | ||
| out_uint32_le(s, val); /* map 4 */ |
There was a problem hiding this comment.
Can you please add a comment or a constant for the 0x00020008 value to say where this value comes from. (Eg. If this value is a constant from a specification, please reference the specification, or if this value is derived, then add a comment to show the calculation.)
| @@ -144,6 +144,8 @@ env_set_user(const char *username, char **passwd_file, int display, | |||
| g_setenv("XRDP_SESSION", "1", 1); | |||
| /* XRDP_SOCKET_PATH should be set even here, chansrv uses this */ | |||
| g_setenv("XRDP_SOCKET_PATH", XRDP_SOCKET_PATH, 1); | |||
| g_sprintf(text, XRDP_PCSC_STR, display); | |||
| g_setenv("PCSCLITE_CSOCK_NAME", text, 1); | |||
There was a problem hiding this comment.
Is this environment variable name used only by xrdp chansrv, of is there another program/library that reads this environment variable?
There was a problem hiding this comment.
Another program reads it (pssc-lite).
sesman/session.c
Outdated
| g_snprintf(file, 255, XRDP_PCSC_STR, display); | ||
| if (g_file_exist(file)) | ||
| { | ||
| log_message(LOG_LEVEL_DEBUG, "cleanup_sockets: deleting %s", file); |
There was a problem hiding this comment.
Please use the logging macro LOG instead of the log_message function directly everywhere in your pull request because the LOG macro adds extra debug info when xrdp is compiled with the debug flag.
sesman/chansrv/smartcard_pcsc.c
Outdated
| out_s = uds_client->con->out_s; | ||
| init_stream(out_s, 8192); | ||
| out_uint32_le(out_s, 4); | ||
| out_uint32_le(out_s, 3); | ||
| out_uint32_le(out_s, 0x80100001); /* result */ | ||
| s_mark_end(out_s); |
There was a problem hiding this comment.
For all of the places in the code where you have added sending or receiving a message, can you please add a comment or trace log message to say what is the message type that is being transmitted.
Can you please add comments and/or constants for the values in the message to clarify what is the meaning of the message.
There was a problem hiding this comment.
I fully understand your point, but unfortunately most of the code has been written by jsorg71 and I don't master it all... However I can manage to grab information through microsoft documentation about rdp protocol, but I need some extra time...
There was a problem hiding this comment.
I added comment on this part of code, and on 2 others parts (or fix them). Don't hesitate to report any other uncommented parts.
| if (status == 0) | ||
| { | ||
| in_uint8s(in_s, 16); | ||
| in_uint32_le(in_s, return_code); | ||
| } | ||
| out_s = trans_get_out_s(con, 8192); | ||
| s_push_layer(out_s, iso_hdr, 8); | ||
| out_uint32_le(out_s, status); /* SCARD_S_SUCCESS status */ | ||
| init_stream(out_s, 0); | ||
| out_uint32_le(out_s, 0); /* hContext */ | ||
| out_uint32_le(out_s, return_code); /* rv */ | ||
| s_mark_end(out_s); | ||
| bytes = (int) (out_s->end - out_s->data); | ||
| s_pop_layer(out_s, iso_hdr); | ||
| out_uint32_le(out_s, bytes - 8); | ||
| out_uint32_le(out_s, 0x02); /* SCARD_RELEASE_CONTEXT 0x02 */ | ||
| return trans_force_write(con); | ||
| rv = trans_write_copy(con); | ||
| uds_client->ref_count--; | ||
| free_uds_client(uds_client); | ||
| return rv; |
There was a problem hiding this comment.
Todo for reviewer: review this section in more detail because this diff seems odd that the steam is being reinitialized and it looks like there was data in the stream in the old version. Is this just an artifact of the diff?
|
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:-
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. |
|
Hi @zorgluf |
Thanks for the review ! |
Hi @aquesnel
|
|
Hi. |
|
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 |
Correct. Not so much changes. The other commit add comments and some raw int value converted to const variables. |
|
I'd git isn't working for you, here's the simple solution.
|
|
@zorgluf - I'm having trouble getting 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:- On an xrdp session to an Ubuntu 20.04 VM (pcsc-tools 1.5.5-1) the
Is there anything else obvious I need to do which I haven't? Thanks. |
|
@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 |
|
@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 @zorgluf I think I owe you an apology. The faulty routine A couple of things which may be useful to you:-
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:- 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):- You can then update your PR in git hub using I appreciate that's a lot to take in! If you've got any questions, please let me know. |
|
A quick addition to the above; at the moment my branch That's probably something you don't need to worry about yet; we can fix that later. |
|
@matt335672 thank you very much for your time on this request, it helps a lot.
Code tested on my original RHEL7 server and also on ubuntu 20.04. |
sesman/chansrv/smartcard_pcsc.c
Outdated
| @@ -1686,18 +1689,30 @@ scard_process_cmd_wait_reader_state_change(struct trans *con, | |||
| struct stream *in_s) | |||
| { | |||
| int rv; | |||
| //struct stream *out_s; | |||
| int timeOut; | |||
| int timeOut __attribute__((unused)); | |||
There was a problem hiding this comment.
Shouldn't need __attribute__((unused)) here.
There was a problem hiding this comment.
On RHEL, without this dircective, I have the following error :
smartcard_pcsc.c: In function âscard_process_cmd_wait_reader_state_changeâ:
smartcard_pcsc.c:1688:9: error: variable âtimeOutâ set but not used [-Werror=unused-but-set-variable]
int timeOut;
There was a problem hiding this comment.
Sorry - you are of course quite correct, and this comes about because of the LOG_DEVEL() macro.
We've got a temporary macro you can use to do this, rather than the attribute marking. Can you try the following:-
int timeOut;
. .
UNUSED_VAR(timeOut);If you grep through for UNUSED_VAR you'll find a couple of uses of it.
Even better would be to move the declarations closer to the point-of-use (i.e.) :-
int rv;
<some declarations deleted here>
struct pcsc_uds_client *uds_client;
LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_cmd_wait_reader_state_change:");
uds_client = (struct pcsc_uds_client *) (con->callback_data);
uds_client->waiting = 1;
if ((uds_client->vMajor == 4) && (uds_client->vMinor > 2)) {
//In these versions, return reader states immediately
struct stream *out_s = con->out_s;
int reader_state_bytes = sizeof(uds_client->readerStates);
init_stream(out_s, reader_state_bytes);
out_uint8a(out_s, uds_client->readerStates, reader_state_bytes);
s_mark_end(out_s);
rv = trans_write_copy(con);
} else {
int timeOut;
#ifndef USE_DEVEL_LOGGING
/* TODO: remove UNUSED_VAR once the `timeOut` variable is used for more
than logging in debug mode */
UNUSED_VAR(timeOut);
#endif
in_uint32_le(in_s, timeOut);
LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_cmd_wait_reader_state_change: timeOut %d",
timeOut);
in_uint32_le(in_s, rv);
LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_cmd_wait_reader_state_change: rv %d", rv);
}It's not pretty, but it's the same as we've used elsewhere.
There was a problem hiding this comment.
Thanks, modified with UNUSED_VAR macro.
sesman/chansrv/smartcard_pcsc.c
Outdated
| uds_client = (struct pcsc_uds_client *) (con->callback_data); | ||
| uds_client->waiting = 1; | ||
| rv = 0; | ||
| if ((uds_client->vMajor == 4) && (uds_client->vMinor > 2)) { | ||
| //In these versions, return reader states immediatly |
sesman/chansrv/smartcard_pcsc.c
Outdated
| @@ -1650,7 +1652,8 @@ scard_process_cmd_version(struct trans *con, struct stream *in_s) | |||
| LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_version: major %d minor %d", major, minor); | |||
There was a problem hiding this comment.
Please replace this with LOG(LOG_LEVEL_INFO so we get useful error reports from users!
|
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, :- You could also add a log message to the test in 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 |
|
Hi @matt335672 , 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). and in the pcsc lib : I really don't have any more idea to fix this. Even between RHEL7 and RHEL8, this value change... |
|
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:-
If that's the case, a sensible way forward seems to be to be:-
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. |
|
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. |
|
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. |
|
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. @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 ? |
|
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. |
|
@matt335672 Unfortunately, I've never used the smartcard feature. I can do very few things on this. |
|
@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. |
|
Yes, I'm happy with that. |
|
What is the status of this PR? |
|
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. |
|
Ping on this, is this possible to be raised in a priority request? More and more people are using 2FA for security and this is a good thing. I can test builds, but I do not know the pcsc framework to write any code here. |
|
@zorgluf - my apologies for not getting to this sooner. I've rebased this to the latest devel at https://github.com/matt335672/xrdp/tree/pcsc_updates. I've kept attributions to @jsorg71 and @zorgluf as appropriate. An additional commit fixes potential memory leaks found by the latest cppcheck. The good news is it seems to work from some very limited testing. I can see my Yubikey using the Looking at PRs in this area over the last few years, I see that #2454 has now been effectively removed. The PCSC socket is created in the XRDP socket directory and although the socket file has 660 permissions, the following statement should be borne in mind from UNIX(7) under 'Pathname socket ownership and permissions' :-
In other words this isn't production ready yet. I think I need to go back to revisit #2731 at this point and get that merged. That will give me the tools to fix any potential permissions issues. I'll also try to get a more satisfactory solution to the MAX_READERS problem mentioned above. |
|
I did create https://github.com/jsorg71/libpcscd in an attempt to isolate the communication with libpcsclite. I was thinking to use it as a submodule. Maybe I can get Ludovic interested in it. It would be great if pcscd also used this library. I also tried to document the pcsclite protocol, I'll see if I can find that. |
|
I've rebased https://github.com/matt335672/xrdp/tree/pcsc_updates on the latest devel again. That now includes #2731, which means the PCSC socket is better protected than it was. There's also a parallel discussion going on in #2625 where we're trying to work out what the best long-term architecture for this is. |
|
Hi @matt335672 , I wish to use Yubikey as webauthn through XRDP, the key is plugged on my NB, and I want to use the key in the remote Ubuntu VM. I follow your instruction to recompile XRDP on Ubuntu VM from the repo https://github.com/matt335672/xrdp/tree/pcsc_updates, however my RDP linking will shutdown immediately after I finish my account/password login on Windows remote desktop connection. I also recompile the xorgxrdp according to its repo (devel branch). Looking infos in the .xorgxrdp.10.log, there's an error: (EE) systemd-logind: failed to take device /dev/dri/card0: Operation not permitted Wish you could give me some instructions, thank you. |
|
That error's pretty normal. Look in the sesman log. It's likely your session is terminating very quickly. You're not logged in on the console are you? As a warning, it's very unlikely this branch will be merged now. We're working on a new approach which doesn't have the same dependency on PCSC-Lite internal interfaces. |
|
Also, and more importantly, this interface only works with authentication stacks using libpcsclite. Your WebAuthn stack may or may not use this interface. |
|
@matt335672 Thx for the reply :) , I wish to run the compiled XRDP successfully then check WebAuthN is valid or not then. After I compiled xrdp, I Then I run the xrdp and xrdp-sesman by type the command in the terminal manually, no sure it is a correct way or not. |
|
I try to compile xrdp on another brand new Ubuntu. |
|
@tymaoa2 - there's a lot you need to get working before you can even look at this.
Bear in mind also that this is a development branch. If you want to work on something like that you need to be aware of things like the above. You may be better off waiting until something more substantial is ready for some more user-focussed (rather than developer-focussed) testing. Otherwise I can see you wasting a great deal of time on this and not achieving anything useful. |
|
I have a problem with incorrect identification |
|
@matt335672 will you plan to rebase pcsc_updates branch to 0.10.1 ? |
|
@sibskull - I haven't got any plans to do this at the moment. It's a fair bit of work, and this branch does represent a dead-end on development. |
Mainly based on the great work from jsorg71 (#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.