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

Set correct output length in PKCS#1 v1.5 depadding #3077

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

xhanulik
Copy link
Contributor

Fixes #3076

The PKCS#1 v1.5 depadding function returned correctly the length of resulting message, but the out_len used in minidriver was not set to the length of the depadded message.

Checklist
  • Documentation is added or updated
  • New files have a LGPL 2.1 license statement
  • PKCS#11 module is tested

Copy link
Member

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

looks good. Lets see if it solves the originally reported issue.

@RufusJWB
Copy link

Sorry, but not working. Here are the logs: https://gist.github.com/RufusJWB/9d287f887f16c0e4732ab59a4f82bb0c

@xhanulik
Copy link
Contributor Author

@RufusJWB thanks for report and log, I will investigate it along with the recent changes to minidriver.

@xhanulik xhanulik force-pushed the pkcs1-outlen branch 3 times, most recently from 98ed5d8 to de25718 Compare March 21, 2024 10:03
@xhanulik
Copy link
Contributor Author

I have found some additional issues

  • the return value of depadding function should be non-negative in good case
  • the inversion after depadding is refactored, it should be touching only bytes from decrypted message now.

@RufusJWB
Copy link

I can confirm, the bug is fixed now.

@xhanulik
Copy link
Contributor Author

@RufusJWB Thanks for testing!

Copy link
Member

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

two minor comments.

As we are not Windows developers, is there somebody from you reporting this issue, able to put together some (ideally automated) tests of the minidriver to avoid such issues in the future? Or at least be able to run some manual tests before the release?

src/libopensc/padding.c Outdated Show resolved Hide resolved
src/minidriver/minidriver.c Outdated Show resolved Hide resolved
@RufusJWB
Copy link

As we are not Windows developers, is there somebody from you reporting this issue, able to put together some (ideally automated) tests of the minidriver to avoid such issues in the future?

Sorry, but not me.

Or at least be able to run some manual tests before the release?

Yes, I'd be happy to support. Ideally, you would provide a WinGet package with the pre-releases. I once opened an issue #2699 about this, which we might want to re-visit.

@Jakuje
Copy link
Member

Jakuje commented Mar 25, 2024

It looks like there is (outdated) opensc 0.23.0 as for today:

https://github.com/microsoft/winget-pkgs/blob/8f8f8792cd97da59cbf5da25d86f756cf00eebb2/manifests/o/OpenSC/OpenSC/0.23.0.0/OpenSC.OpenSC.yaml#L4

created after your request in microsoft/winget-pkgs#123939 so maybe just opening another to update would get it updated (or just opening a PR to update it) -- they just point out to our installers in github releases which do not look that complicated.

@RufusJWB
Copy link

It looks like there is (outdated) opensc 0.23.0 as for today:

https://github.com/microsoft/winget-pkgs/blob/8f8f8792cd97da59cbf5da25d86f756cf00eebb2/manifests/o/OpenSC/OpenSC/0.23.0.0/OpenSC.OpenSC.yaml#L4

created after your request in microsoft/winget-pkgs#123939 so maybe just opening another to update would get it updated (or just opening a PR to update it) -- they just point out to our installers in github releases which do not look that complicated.

I wasn't aware, that this topic was picked up by someone at Microsoft. Great news. I'll follow up with them.

src/libopensc/padding.c Outdated Show resolved Hide resolved
@Jakuje Jakuje merged commit 7471dd2 into OpenSC:master Apr 4, 2024
42 of 44 checks passed
@Jakuje
Copy link
Member

Jakuje commented Apr 4, 2024

Thank you!

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.

Outlook not able to decrypt email - part 2
3 participants