http://bugs.winehq.org/show_bug.cgi?id=20358
Summary: chromium's base_unittests.exe fails on RSAPrivateKeyUnitTest.* Product: Wine Version: 1.1.31 Platform: PC OS/Version: Linux Status: NEW Keywords: download, source, testcase Severity: normal Priority: P2 Component: crypt32 AssignedTo: wine-bugs@winehq.org ReportedBy: dank@kegel.com
The three tests, RSAPrivateKeyUnitTest.InitRandomTest, RSAPrivateKeyUnitTest.PublicKeyTest, and RSAPrivateKeyUnitTest.ShortIntegers all fail.
Source involved is http://src.chromium.org/viewvc/chrome/trunk/src/base/crypto/rsa_private_key_... http://src.chromium.org/viewvc/chrome/trunk/src/base/crypto/rsa_private_key.... http://src.chromium.org/viewvc/chrome/trunk/src/base/crypto/rsa_private_key_... etc.
I'll attach +relay,+crypt logs of the three tests failing.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #1 from Dan Kegel dank@kegel.com 2009-10-13 20:21:40 --- Created an attachment (id=24110) --> (http://bugs.winehq.org/attachment.cgi?id=24110) +relay,+crypt log of RSAPrivateKeyUnitTest.InitRandomTest, compressed with rzip
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #2 from Dan Kegel dank@kegel.com 2009-10-13 20:22:30 --- Created an attachment (id=24111) --> (http://bugs.winehq.org/attachment.cgi?id=24111) +relay,+crypt log of RSAPrivateKeyUnitTest.PublicKeyTest, compressed with rzip
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #3 from Dan Kegel dank@kegel.com 2009-10-13 20:23:08 --- Created an attachment (id=24112) --> (http://bugs.winehq.org/attachment.cgi?id=24112) +relay,+crypt log of RSAPrivateKeyUnitTest.ShortIntegers, compressed with rzip
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #4 from Dan Kegel dank@kegel.com 2009-10-14 06:47:03 --- Created an attachment (id=24121) --> (http://bugs.winehq.org/attachment.cgi?id=24121) valgrind errors from ShortIntegers test
Bonus deal: the ShortIntegers test also has valgrind errors! See attached log. The stack is symbolic all the way from the user app through wine; source file names with .cc on the end are the user app. (Yay valgrind!)
(The same test probably doesn't have purify errors on windows, but it's possible purify missed something and/or that we ignored the warning.)
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #5 from Juan Lang juan_lang@yahoo.com 2009-10-19 23:01:37 --- Is it possible to attach a compiled test program?
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #6 from Dan Kegel dank@kegel.com 2009-10-19 23:04:15 --- Yes. (I posted this originally in bug 20390.) wget http://kegel.com/wine/chromium/chromium-tests.tar.bz2 tar -xjvf chromium-tests.tar.bz2 will get you base_unittests.exe and a whole bunch of other stuff. To run all the unit tests in that file in small batches, do more chromium-runtests.sh sh chromium-runtests.sh Or just run base_unittests.exe with no arguments to run all tests in one go.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #7 from Juan Lang juan_lang@yahoo.com 2009-10-20 13:42:48 --- It seems you can run the failing tests with: wine base_unittests.exe --gtest_filter=RSAPrivateKeyUnitTest.*
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #8 from Dan Kegel dank@kegel.com 2009-10-20 13:49:35 --- Yeah, that's what that script does, more or less.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #9 from Juan Lang juan_lang@yahoo.com 2009-10-20 13:59:22 --- Okay. I'm trying to boil it down to instructions for how to reproduce the error, Dan ;-) I think I finally succeeded, though the download size is a bit painful.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #10 from Juan Lang juan_lang@yahoo.com 2009-10-20 14:15:32 --- Created an attachment (id=24250) --> (http://bugs.winehq.org/attachment.cgi?id=24250) Patch: Store key when algid is a CALG_RSA_* algid, too
This fixes one of the failures for me. CryptExportPublicKeyInfoEx was failing, because CryptGetUserKey was failing. The CryptGenKey call was generating a key, but not storing it in the provider. Frankly, I have no idea why it wasn't doing so before, or whether this is even the correct fix. I don't claim any more understanding of rsaenh than your average bloke.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #11 from Juan Lang juan_lang@yahoo.com 2009-10-20 14:28:50 --- (In reply to comment #10)
This fixes one of the failures for me. CryptExportPublicKeyInfoEx was failing, because CryptGetUserKey was failing. The CryptGenKey call was generating a key, but not storing it in the provider. Frankly, I have no idea why it wasn't doing so before, or whether this is even the correct fix.
The initial checkin of rsaenh was storing the key, but it was removed to doing so only conditionally in the fourth commit: http://source.winehq.org/git/wine.git/?a=commit;h=95c3d9b2a561e67c2a9460fc0f...
Tests would probably help.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #12 from Juan Lang juan_lang@yahoo.com 2009-10-20 15:08:57 --- The second failing test is an overly restrictive one on chromium's part. The failing test expects the following public key:
const uint8 expected_public_key_info[] = { 0x30, 0x81, 0x9f, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x81, 0x8d, 0x00, 0x30, 0x81, 0x89, 0x02, 0x81, 0x81, 0x00, 0xb8, 0x7f, 0x2b, 0x20, 0xdc, 0x7c, 0x9b, 0x0c, 0xdc, 0x51, 0x61, 0x99, 0x0d, 0x36, 0x0f, 0xd4, 0x66, 0x88, 0x08, 0x55, 0x84, 0xd5, 0x3a, 0xbf, 0x2b, 0xa4, 0x64, 0x85, 0x7b, 0x0c, 0x04, 0x13, 0x3f, 0x8d, 0xf4, 0xbc, 0x38, 0x0d, 0x49, 0xfe, 0x6b, 0xc4, 0x5a, 0xb0, 0x40, 0x53, 0x3a, 0xd7, 0x66, 0x09, 0x0f, 0x9e, 0x36, 0x74, 0x30, 0xda, 0x8a, 0x31, 0x4f, 0x1f, 0x14, 0x50, 0xd7, 0xc7, 0x20, 0x94, 0x17, 0xde, 0x4e, 0xb9, 0x57, 0x5e, 0x7e, 0x0a, 0xe5, 0xb2, 0x65, 0x7a, 0x89, 0x4e, 0xb6, 0x47, 0xff, 0x1c, 0xbd, 0xb7, 0x38, 0x13, 0xaf, 0x47, 0x85, 0x84, 0x32, 0x33, 0xf3, 0x17, 0x49, 0xbf, 0xe9, 0x96, 0xd0, 0xd6, 0x14, 0x6f, 0x13, 0x8d, 0xc5, 0xfc, 0x2c, 0x72, 0xba, 0xac, 0xea, 0x7e, 0x18, 0x53, 0x56, 0xa6, 0x83, 0xa2, 0xce, 0x93, 0x93, 0xe7, 0x1f, 0x0f, 0xe6, 0x0f, 0x02, 0x03, 0x01, 0x00, 0x01 };
The public key we export instead is: static const BYTE pbEncoded[] = { 0x30, 0x81, 0x9d, 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x03, 0x81, 0x8d, 0x00, 0x30, 0x81, 0x89, 0x02, 0x81, 0x81, 0x00, 0xb8, 0x7f, 0x2b, 0x20, 0xdc, 0x7c, 0x9b, 0x0c, 0xdc, 0x51, 0x61, 0x99, 0x0d, 0x36, 0x0f, 0xd4, 0x66, 0x88, 0x08, 0x55, 0x84, 0xd5, 0x3a, 0xbf, 0x2b, 0xa4, 0x64, 0x85, 0x7b, 0x0c, 0x04, 0x13, 0x3f, 0x8d, 0xf4, 0xbc, 0x38, 0x0d, 0x49, 0xfe, 0x6b, 0xc4, 0x5a, 0xb0, 0x40, 0x53, 0x3a, 0xd7, 0x66, 0x09, 0x0f, 0x9e, 0x36, 0x74, 0x30, 0xda, 0x8a, 0x31, 0x4f, 0x1f, 0x14, 0x50, 0xd7, 0xc7, 0x20, 0x94, 0x17, 0xde, 0x4e, 0xb9, 0x57, 0x5e, 0x7e, 0x0a, 0xe5, 0xb2, 0x65, 0x7a, 0x89, 0x4e, 0xb6, 0x47, 0xff, 0x1c, 0xbd, 0xb7, 0x38, 0x13, 0xaf, 0x47, 0x85, 0x84, 0x32, 0x33, 0xf3, 0x17, 0x49, 0xbf, 0xe9, 0x96, 0xd0, 0xd6, 0x14, 0x6f, 0x13, 0x8d, 0xc5, 0xfc, 0x2c, 0x72, 0xba, 0xac, 0xea, 0x7e, 0x18, 0x53, 0x56, 0xa6, 0x83, 0xa2, 0xce, 0x93, 0x93, 0xe7, 0x1f, 0x0f, 0xe6, 0x0f, 0x02, 0x03, 0x01, 0x00, 0x01 };
You can see the main difference on line 3 of the expected input: there appear two bytes, 0x05,0x00. This is the NULL encoding of asn.1, and represents the algorithm id's parameters, which are empty. We omit them instead. Either encoding is legal. (The remaining differences reflect the different lengths of the outputs.)
Changing our implementation to include the NULL causes our CryptHashPublicKeyInfo implementation to fail, as the tests expect the hashed public key not to contain the NULL. It's possible to fix this by introducing a private function just for CryptHashPublicKeyInfo.. but I think the Chromium test is overly restrictive, and it'd be simpler to fix it to accept either form.
http://bugs.winehq.org/show_bug.cgi?id=20358
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |juan_lang@yahoo.com
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #13 from Juan Lang juan_lang@yahoo.com 2009-10-20 15:15:59 --- Hmm, my tests already show that native does embed the NULL, so perhaps my tests were too permissive by accepting both forms.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #14 from Juan Lang juan_lang@yahoo.com 2009-10-20 15:40:32 --- I sent patches for the second error: http://www.winehq.org/pipermail/wine-patches/2009-October/080297.html http://www.winehq.org/pipermail/wine-patches/2009-October/080298.html http://www.winehq.org/pipermail/wine-patches/2009-October/080299.html http://www.winehq.org/pipermail/wine-patches/2009-October/080300.html
I misspoke when I sent the last patch, saying it fixed this bug. It doesn't, it just fixes the second failure.
http://bugs.winehq.org/show_bug.cgi?id=20358
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #24250|0 |1 is obsolete| |
--- Comment #15 from Juan Lang juan_lang@yahoo.com 2009-10-20 17:27:26 --- (From update of attachment 24250) The fix is correct. I sent a patch that includes the fix, and tests: http://www.winehq.org/pipermail/wine-patches/2009-October/080310.html
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #16 from Juan Lang juan_lang@yahoo.com 2009-10-20 17:28:28 --- (In reply to comment #15)
The fix is correct. I sent a patch that includes the fix, and tests:
I mean, the fix for the first issue. I've now sent patches for the first and second failing tests, but I haven't really looked into the third one yet.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #17 from Juan Lang juan_lang@yahoo.com 2009-11-02 20:03:39 --- Figuring out the ShortIntegers test is a challenge, because the chromium tests hand build their own asn.1-encoded private key, and only produce a single bit of information for the tests: it equals the expected value, or it doesn't. It doesn't say where the difference lies.
The expected output is easy enough to see, short_integer_with_high_bit in rsa_private_key_unittest.cc. The value it's compared to isn't produced directly by Wine, however. Instead, a private key is exported in a different format, and the unit tests copy bits of pieces of this into a memory blob, along with magic asn.1 bytes. Aside from the CryptExportKey, whose value is not the final output value, none of the remaining code to produce the expected output value uses the crypto API, so a trace isn't helpful.
The relevant code is in rsa_private_key.cc's PrivateKeyInfoCode::Export function.
A simpler to understand bit of code would go a long way toward getting this fixed. Ideally it'd be a Wine regression test. I tried compiling this myself under Linux with wineg++, but I couldn't coerce it.
http://bugs.winehq.org/show_bug.cgi?id=20358
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Component|crypt32 |rsaenh
--- Comment #18 from Juan Lang juan_lang@yahoo.com 2009-11-03 12:00:55 --- Now that there's an rsaenh component, changing component.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #19 from Juan Lang juan_lang@yahoo.com 2009-11-03 13:42:00 --- I sent a patch that demonstrates the problem a little more clearly: http://www.winehq.org/pipermail/wine-patches/2009-November/080871.html
It's still a little hard to tell the exact problem, but it appears to be a bug in how integers with the high bit are exported. They should have a 0 byte prepended to them to prevent them from being interpreted as signed. I believe the bug is in mp_to_unsigned in rsaenh's mpi.c, but that's as far as I've gotten.
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #20 from Juan Lang juan_lang@yahoo.com 2009-11-05 12:21:30 --- I was mistaken in saying that the exported private key should have a leading 0 byte prepended. On the contrary, it should have such bytes omitted. I was just misreading the exported private key.
I sent a patch which corrects the issue here: http://www.winehq.org/pipermail/wine-patches/2009-November/080947.html
It's part of a series, and it depends on patch 1 of the series: http://www.winehq.org/pipermail/wine-patches/2009-November/080949.html
(The remaining patches in the series are handy, but aren't strictly needed to fix this bug.)
http://bugs.winehq.org/show_bug.cgi?id=20358
--- Comment #21 from Dan Kegel dank@kegel.com 2009-11-05 21:33:15 --- Confirmed! Looking forward to tomorrow's cvs with bated breath. (I hope Alexandre is still planning to do commits tomorrow...)
http://bugs.winehq.org/show_bug.cgi?id=20358
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #22 from Juan Lang juan_lang@yahoo.com 2009-11-09 15:44:34 --- Fixed by commit 2d05074fbaf8edb4c765ad65aabdcb0929ed60a6.
http://bugs.winehq.org/show_bug.cgi?id=20358
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #23 from Alexandre Julliard julliard@winehq.org 2009-11-13 12:44:35 --- Closing bugs fixed in 1.1.33.
-
wine-bugs＠winehq.org