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

Minidriver EC Changes for private keys that do not need a PIN and/or ECDSA failure to sign #2722

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

dengert
Copy link
Member

@dengert dengert commented Feb 26, 2023

@llogar has been working on EOI cards with minidriver and has run into two problems which can be problem with other cards such as PIV.

A private key that does not need a PIN does not work.
ECDSA signatures failing because minidriver passed SC_ALGORITHM_RSA_PAD_PKCS1 or other SC_ALGORITHM_RSA_PAD_* flags to sc_pkcs15_compute_signature.

This PR is being submitted to take advantage of the Appveyor build for testing with PIV card.

Checklist
  • Documentation is added or updated
  • New files have a LGPL 2.1 license statement
  • PKCS#11 module is tested
  • [todo] Windows minidriver is tested
  • macOS tokend is tested

@@ -653,7 +653,7 @@ int sc_get_encoding_flags(sc_context_t *ctx,
*sflags = SC_ALGORITHM_RSA_PAD_NONE;
*pflags = iflags;

} else if ((caps & (SC_ALGORITHM_RSA_PAD_PKCS1 | SC_ALGORITHM_RSA_HASH_NONE)) &&
} else if ((caps & SC_ALGORITHM_RSA_PAD_PKCS1) && (caps & SC_ALGORITHM_RSA_HASH_NONE) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My problem with encoding flags is not covered with this. See my log in #2718:

P:20188; T:12200 2023-02-23 09:27:00.238 [cardmod] padding.c:622:sc_get_encoding_flags: iFlags 0x100002, card capabilities 0x100100

caps doesn't have SC_ALGORITHM_RSA_PAD_PKCS1 set.

I'd rather fix this in minidriver code with something like:

--- minidriver.c
+++ minidriver.c
@@ -4978,6 +4978,7 @@
         dwret = SCARD_E_INVALID_VALUE;
         goto err;
     }
+    opt_crypt_flags &= ~SC_ALGORITHM_RSA_PADS;
   } else {
     logprintf(pCardData, 0, "invalid private key\n");
     dwret = SCARD_E_INVALID_VALUE;

and exclude any padding flags there when doing ECDSA?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it does. And it looks like minidriver sets it when it should not.

In minidriver.c (line numbers):
4692 DWORD WINAPI CardSignData(__in PCARD_DATA pCardData, __inout PCARD_SIGNING_INFO pInfo)
4699 int opt_crypt_flags;

In one of you logs I see: "CARD_PADDING_INFO_PRESENT not set:

4789 logprintf(pCardData, 3, "CARD_PADDING_INFO_PRESENT not set\n");
4791 opt_crypt_flags = SC_ALGORITHM_RSA_PAD_PKCS1;

This code is from cardmod when Windows only supported RSA. This looks like the problem.

4947 r = sc_pkcs15_compute_signature(vs->p15card, pkey, opt_crypt_flags, dataToSign, dataToSignLen, pbuf, lg, NULL);

In logL sc_pkcs15_compute_signature: ECDSA using SC_ALGORITHM_ECDSA_RAW flags before 0x00000002

the "before" is the flags before they are change. which is SC_ALGORITHM_RSA_PAD_PKCS1

So my change sort of fixed it (but may break something else), but the real fix need to be in minidriver around line 4791 which should not not do any of that if not RSA.

Is that where you wanted to make the change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, See this commit:

llogar@4c31a1d

Altough this clears padding flags post festum rather than prevents setting them in the first place...

@frankmorgner frankmorgner marked this pull request as draft February 27, 2023 15:31
@dengert dengert changed the title Minidriver EC Changes for private keys with do not need a PIN and ECDSA failure to sign Minidriver EC Changes for private keys that do not need a PIN and/or ECDSA failure to sign Feb 27, 2023
@dengert dengert marked this pull request as ready for review February 27, 2023 20:28
@dengert
Copy link
Member Author

dengert commented Feb 28, 2023

@llogar have a look at the revised changes based on your work in this PR. They fix EC minidriver problems for any token and not just EOI.

@@ -6381,14 +6381,26 @@ DWORD WINAPI CardGetProperty(__in PCARD_DATA pCardData,
if (dwFlags >= MD_MAX_PINS)
MD_FUNC_RETURN(pCardData, 1, SCARD_E_INVALID_PARAMETER);

if (!vs->pin_objs[dwFlags])
if (!dwFlags && !vs->pin_objs[dwFlags])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in my case, when there is no PIN, it still returns SCARD_E_INVALID_PARAMETER, as this condition is met (dwFlags == 0 && vs->pin_objs[dwFlags] == NULL)

Possibly it should return "empty" PIN info for all vs->pin_objs[dwFlags] == NULL cases, as there may even be cards with ROLE_ADMIN and/or ROLE_USER (and not just ROLE_EVERYONE) without a PIN?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it should be:
if (dwFlags && !vs->pin_objs[dwFlags])
or to make it more readable:
if (dwFlags != ROLE_EVERYONE) && (vs->pin_objs[dwFlags] == NULL))

As for none real pins vs->pin_objs is filled in from PKCS15 structures.

Then if dwFlags == ROLE_EVERYONE it will fall through to the case ROLE_EVERYONE:' and log a message and endup
6510 with another log message and hexdump or *p

Will update and try again today.

dengert added 2 commits March 1, 2023 06:45
Needed for private keys that do not require a PIN

 Changes to be committed:
	modified:   minidriver/minidriver.c
SC_ALGORITHM_RSA_HASH_SHA* are also SC_ALGORITHM_ECDSA_HASH_SHA*
Turn off any RSA PAD flags if key is EC

 On branch minidriver-EC-2
 Changes to be committed:
	modified:   minidriver/minidriver.c
@llogar
Copy link
Contributor

llogar commented Mar 1, 2023

Thanks. It works now.

@dengert
Copy link
Member Author

dengert commented Mar 1, 2023

With updated (forced pushed) if statement: md_log shows:

returning info on PIN ROLE_EVERYONE [0]
returns 'PIN Information' --- 00000265ED666550:36
 0000  06000000 00000000 00000000 00000000  00000000 06000000 02000000 00000000
 0020  00000000 
MD_Function:CardGetProperty:6511 returning with: 0x00000000

certutil -scinfo shows all 4 certificates, where Certificate 3 does not require a PIN
Note: "Private key verifies"

================ Certificate 3 ================
--- Reader: Identiv SCR3500 A Contact Reader 0
---   Card: NIST DEMO Oberthur test cards
Provider = Microsoft Smart Card Key Storage Provider
Key Container = Certificate for Card Authentication

Serial Number: 01f6
Issuer: CN=Test ECC P-384 CA for Test PIV Cards, OU=Test CA, O=Test Certificates 2010, C=US
 NotBefore: 10/1/2010 2:30 AM
 NotAfter: 10/1/2030 2:30 AM
Subject: SERIALNUMBER=D650185A13422C2267829D916CD89080501E649C86501843E2, OU=Test Agency, OU=Test Department, O=Test Government, C=US
Non-root Certificate
Cert Hash(sha1): 59618196f7713320e2c758117903141d415282f0

Performing  public key matching test...
Public key matching test succeeded
  Key Container = Certificate for Card Authentication
  Provider = Microsoft Smart Card Key Storage Provider
  ProviderType = 0
  Flags = 1
    0x1 (1)
  KeySpec = 0 -- XCN_AT_NONE
Private key verifies
Microsoft Smart Card Key Storage Provider: KeySpec=0
AES256+RSAES_OAEP(ECC:CNG) test skipped

@Jakuje
Copy link
Member

Jakuje commented Sep 19, 2023

Whats the status of this PR? Should we merge the ECC part and drop/postpone the discussed/disputed role_everyone as it is already discussed in #2721?

@dengert
Copy link
Member Author

dengert commented Sep 20, 2023

This PR replaces #2721 and adds some additional changes. It was tested using PIV card and certutil.exe as noted in #2722 (comment)

@llogar author of #2712 says it worked in #2722 (comment)

I think it is ready to to be committed.

@Jakuje
Copy link
Member

Jakuje commented Sep 20, 2023

ok, I am not much happy how the flags are handled in the minidriver, but I do not know anything about the minidriver so if you believe it is working, please merge this. Its in the limbo for long enough already.

@dengert dengert merged commit 9d059e0 into OpenSC:master Sep 20, 2023
@dengert
Copy link
Member Author

dengert commented Sep 20, 2023

Main problem with old code is:#define ROLE_EVERYONE 0 which means there is no PIN_ID. This PR takes this into account.

@dengert dengert deleted the minidriver-EC-2 branch September 20, 2023 10:24
@xhanulik xhanulik mentioned this pull request Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants