microsoft / SymCrypt

Cryptographic library
MIT License
660 stars 68 forks source link

Test suite failures in RSA when using Cng #5

Closed woachk closed 5 years ago

woachk commented 5 years ago

image

According to the documentation, "The key size must be greater than or equal to 512 bits, less than or equal to 16384 bits, and must be a multiple of 64" from https://docs.microsoft.com/en-us/windows/desktop/api/bcrypt/nf-bcrypt-bcryptgeneratekeypair. In this case it isn't, and it makes Cng stop early.

Using g_BitSizeEntries[iSize].nBitsOfModulus = g_BitSizeEntries[iSize].nBitsOfModulus & ~63; makes the test suite fail much later than that point.

Any reason why?

NielsFerguson commented 5 years ago

What version of Windows are you running on? I don’t think we’ve ever run this test code on anything older than Windows 10 RS1; it might very well be that CNG dropped the ‘multiple of 64’ requirements at some point, and our test code just uses that.

We have a Win7 test in the repo, but that was created a long time ago and we have no regular process for running it on Win7. I’d have to run it manually, which involves setting up a Win7 VM, and then spending a day or two fixing all the bugs, and why exactly would I do that?

Cheers!

Niels

From: woachk notifications@github.com Sent: Friday, May 10, 2019 11:45 AM To: microsoft/SymCrypt SymCrypt@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [microsoft/SymCrypt] Test suite failures in RSA when using Cng (#5)

[image]https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuser-images.githubusercontent.com%2F24752637%2F57549476-f02dca00-7363-11e9-8ec9-5065b1b94878.png&data=01%7C01%7Cniels%40microsoft.com%7Cae4fc0d6fd2d4020bcab08d6d57798f9%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=UP%2FmyzBJzbviznbW%2BZEfeJAO4NLTt6MoxS1ZqJ4ueFs%3D&reserved=0

According to the documentation, "The key size must be greater than or equal to 512 bits, less than or equal to 16384 bits, and must be a multiple of 64". In this case it isn't, and it makes Cng stop early.

Using g_BitSizeEntries[iSize].nBitsOfModulus = g_BitSizeEntries[iSize].nBitsOfModulus & ~63; makes the test suite fail much later than that point.

Any reason why?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FSymCrypt%2Fissues%2F5&data=01%7C01%7Cniels%40microsoft.com%7Cae4fc0d6fd2d4020bcab08d6d57798f9%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=yu7ZNUJB%2BiCVuj5JGcQE6LjldUCrIwzb2FN3XOLyUGc%3D&reserved=0, or mute the threadhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJJHWVSOEI372HTTLSY5KNDPUW7CNANCNFSM4HMFMZJA&data=01%7C01%7Cniels%40microsoft.com%7Cae4fc0d6fd2d4020bcab08d6d57798f9%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=l5%2FYyVF0Vs3j4JzJ3h%2FZMizaEE7NOTZbEZEgZ4hkGQM%3D&reserved=0.

woachk commented 5 years ago

@NielsFerguson This is on Windows 10 version 1903 (build 18362.53/19H1)...

NielsFerguson commented 5 years ago

@woachk: Strange; we've been running the full unit test (including the CNG tests) on the latest Win10 builds multiple times each day for many years. No idea what this could be.

woachk commented 5 years ago

@NielsFerguson does https://gist.github.com/woachk/71cb7692b2f328dc7331f21161a37283 as a minimal testcase for this run successfully for you? It'll help me a bit to pinpoint the failure.

Results of testing here: 1024: works 2048: works 1388: doesn't work 2047: doesn't work 960: works

It seems to enforce the multiple of 64 rule here...

edit 2: Fails under 17134/Windows 10 version 1803 too.

NielsFerguson commented 5 years ago

I checked the Windows source code; CNG default provider limits RSA key generation to keys where the modulus is a multiple of 8 bits. We reduced that from the 64 bits it was before to provide more flexibility, but don't allow odder sizes to avoid interoperability issues we've seen.

woachk commented 5 years ago

Removing those sizes makes the testsuite pass: image

889 isn't a multiple of 8 either...