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

SCARD_SCOPE_USER is probably safer #30

Open
adricasti opened this issue Feb 23, 2020 · 1 comment · May be fixed by #31
Open

SCARD_SCOPE_USER is probably safer #30

adricasti opened this issue Feb 23, 2020 · 1 comment · May be fixed by #31

Comments

@adricasti
Copy link

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.

result = SCardEstablishContext(SCARD_SCOPE_SYSTEM,

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

pokusew added a commit that referenced this issue Feb 23, 2020
@pokusew
Copy link
Owner

pokusew commented Feb 25, 2020

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 (Node 12 fixes follow-up #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. 🙂

petrzjunior pushed a commit to petrzjunior/node-pcsclite that referenced this issue Feb 27, 2020
@petrzjunior petrzjunior linked a pull request Feb 27, 2020 that will close this issue
petrzjunior pushed a commit to petrzjunior/node-pcsclite that referenced this issue Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants