pokusew / node-pcsclite

Bindings over pcsclite to access Smart Cards
ISC License
60 stars 54 forks source link

SCARD_SCOPE_USER is probably safer #30

Open adricasti opened 4 years ago

adricasti commented 4 years ago

Hi @pokusew , I noticed that the wrapper is establishing the PC/SC context as system which in most cases is not needed and could create elevated privilege risks. The default should be SCARD_SCOPE_USER and maybe with an optional parameter, a developer could request system scope if is absolutely needed.

https://github.com/pokusew/node-pcsclite/blob/ca9b0ba7ec814ce4c55c71b0ca85239795c872fa/src/pcsclite.cpp#L71

If you agree, I can send a Pull Request although the change is trivial in that line.

pokusew commented 4 years ago

Hi @cheburashka,

thank you for taking your time to inspect the code and opening the issue. 👍 I really appreciate it. I am sorry for the delayed (and little bit long 😄) response.

In the first place, I have to say that I am not an expert on the pcsclite. I created (forked) this library mainly because of the needs for a stable and maintained foundation for my higher-level nfc-pcsc. Nevertheless I've invested a lot of time over the years maintaining it and ensuring compatibility with the latest Node.js. 😄 And I appreciate every contribution because it might address the things I might never come across.

My thoughts:

  1. dwScope argument passed to the SCardEstablishContext should be possible to overwrite by user (developer). I think that the best solution to this would be adding an options argument to the constructor. Then the usage might look like:

    const pcsc = pcsclite(); // without options
    const pcsc = pcsclite({ scope: SCARD_SCOPE_USER }); // overwriting default scope

    In order to use the SCARD_SCOPE_* constants on the JS side, they need to be exported in the C++ code, but that's no big deal.

    @petrzjunior agreed that he will take care of it and implement all needed changes on the C++ side (allow passing options to the constructor and exporting the constants). (He helped me rewrite the native part to make it compatible with the newest Node.js (#27), so he's the best man for the job.)

  2. However, I don't know if it is a good idea to change the default value from SCARD_SCOPE_SYSTEM to SCARD_SCOPE_USER. I would not like to introduce an invisible breaking change that might affect some users.

    Could you please elaborate on what differences does the dwScope value make? e.g. Are there any functions or behavior that differ when SCARD_SCOPE_USER is used compared to the usage of SCARD_SCOPE_SYSTEM? Is the behavior platform dependent (i.e. Windows vs Linux/UNIX vs MacOS?) And how exactly could the SCARD_SCOPE_SYSTEM create elevated privilege risks?

    I tried to test SCARD_SCOPE_USER on macOS and it seemed to work (or like there was no difference at all). I am not able to test it on Linux/UNIX right now (but @petrzjunior will), but I found the following statement in the OSS pcsclite implementation docs for SCardEstablishContext():

    SCARD_SCOPE_USER - Not used.

    So I am wondering whether the dwScope's value does really matter (or it has an effect only on Windows?). Any comments or thoughts on this would be helpful.

Looking forward to hearing from you.

Thanks again. 🙂