Need help with a rsaenh bug

[Qian Hong]

Hello,

I was investigating Bug 33898 [1] hardly and get a partial result, I
have a special test case demonstrate the behavior of Aliwangwang [2],
however, I failed to expand the special case to a common test case. My
attempting is shown in [3]. The hack in [3] works for Aliwangwang, but
the test case in [3] doesn't fully pass on Windows as I expected [4].

I'm not sure if there is an APP bug rather than Wine bug here, is it
valid to call multiple CryptDecrypt with only one CryptDeriveKey
called? If true, is it correct to assume the second CryptDecrypt call
should behavior identical as the first one?
If there is a Wine bug here, what is the right way to write a common
test case for it?

Any hints on this bug is great appreciated!

Thanks very much!

[1] http://bugs.winehq.org/show_bug.cgi?id=33898
[2] http://bugs.winehq.org/attachment.cgi?id=44996
[3] http://bugs.winehq.org/attachment.cgi?id=44997&action=diff
[4] https://testbot.winehq.org/JobDetails.pl?Key=26138

--
Regards,
Qian Hong

-
http://www.winehq.org

[Daniel Jeliński]

It is definitely valid to call CryptDecrypt multiple times with the same
key. Calls with Final = FALSE change the internal state of the key, calls
with Final = TRUE restore the initial state. Subsequent calls with Final =
TRUE should return the same result.

Your testcase fails because CryptDecrypt changes the value of dwLen, which
you do not restore before calling the function again.

Regards,
Daniel

2013/6/27 Qian Hong
> Hello,
>
> I was investigating Bug 33898 [1] hardly and get a partial result, I
> have a special test case demonstrate the behavior of Aliwangwang [2],
> however, I failed to expand the special case to a common test case. My
> attempting is shown in [3]. The hack in [3] works for Aliwangwang, but
> the test case in [3] doesn't fully pass on Windows as I expected [4].
>
> I'm not sure if there is an APP bug rather than Wine bug here, is it
> valid to call multiple CryptDecrypt with only one CryptDeriveKey
> called? If true, is it correct to assume the second CryptDecrypt call
> should behavior identical as the first one?
> If there is a Wine bug here, what is the right way to write a common
> test case for it?
>
> Any hints on this bug is great appreciated!
>
> Thanks very much!
>
> [1] http://bugs.winehq.org/show_bug.cgi?id=33898
> [2] http://bugs.winehq.org/attachment.cgi?id=44996
> [3] http://bugs.winehq.org/attachment.cgi?id=44997&action=diff
> [4] https://testbot.winehq.org/JobDetails.pl?Key=26138
>
> --
> Regards,
> Qian Hong
> -
>
> http://www.winehq.org

[Qian Hong]

Hi Daniel,

On Fri, Jun 28, 2013 at 3:43 AM, Daniel Jeliński  wrote:
> It is definitely valid to call CryptDecrypt multiple times with the same
> key. Calls with Final = FALSE change the internal state of the key, calls
> with Final = TRUE restore the initial state. Subsequent calls with Final =
> TRUE should return the same result.
>
> Your testcase fails because CryptDecrypt changes the value of dwLen, which
> you do not restore before calling the function again.

Thanks a lot of the hint, with your help I finally resolve it! I've
improved my test and submit two rsaenh patches, would you mind help to
review them?

Thanks again!

--
Regards,
Qian Hong

-
http://www.winehq.org

[Daniel Jeliński]

Hi Qian Hong,
I'm not convinced that a failed call to CryptDecrypt actually resets
the key state. It's also possible that CryptDecrypt returns FALSE
before even trying to decrypt if data length is invalid. To check it,
you would need to change the key state by (successfully) calling
CryptDecrypt with Final=FALSE before your test. Could you add such
test?
Regards,
Daniel

2013/6/28, Qian Hong :
> Hi Daniel,
>
> On Fri, Jun 28, 2013 at 3:43 AM, Daniel Jeliński 
> wrote:
> > It is definitely valid to call CryptDecrypt multiple times with the same
> > key. Calls with Final = FALSE change the internal state of the key, calls
> > with Final = TRUE restore the initial state. Subsequent calls with Final
> > =
> > TRUE should return the same result.
> >
> > Your testcase fails because CryptDecrypt changes the value of dwLen,
> > which
> > you do not restore before calling the function again.
>
> Thanks a lot of the hint, with your help I finally resolve it! I've
> improved my test and submit two rsaenh patches, would you mind help to
> review them?
>
> Thanks again!
>
> --
> Regards,
> Qian Hong
> -
>
> http://www.winehq.org

[Qian Hong]

Hi Daniel,

On Fri, Jun 28, 2013 at 4:47 PM, Daniel Jeliński  wrote:
> I'm not convinced that a failed call to CryptDecrypt actually resets
> the key state. It's also possible that CryptDecrypt returns FALSE
> before even trying to decrypt if data length is invalid. To check it,
> you would need to change the key state by (successfully) calling
> CryptDecrypt with Final=FALSE before your test. Could you add such
> test?

Thanks for the important hints! I'll try to improve the test case.

--
Regards,
Qian Hong

-
http://www.winehq.org

[Qian Hong]

Hi Daniel, new patches sent with improving from your hints, would you
mind have a look? Thanks in advance!

[Juan Lang]

Hi Qian,

On Fri, Jun 28, 2013 at 3:44 AM, Qian Hong  wrote:
> Hi Daniel, new patches sent with improving from your hints, would you
> mind have a look? Thanks in advance!

nice work! These look fine to me, but a stylistic nit:
+
 ok(memcmp(pbData,cTestData[i].decstr,cTestData[1].enclen)==0,"decryption
incorrect %d\n",i);

It's more in line with most C code to use !memcmp(...) instead of
memcmp(...)==0. I find it easier to scan, anyway, as I've gotten used to !
comparisons to check equality in memcmp, strcmp, and variants.

Another minor point: it's customary to set last error prior to testing it
when you expect it to have a certain value, e.g.:
+      bad_data[cTestData[i].buflen - 1] = ~bad_data[cTestData[i].buflen -
1];
+      result = CryptDecrypt(hKey, 0, TRUE, 0, bad_data, &dwLen);
+      ok(!result, "CryptDecrypt should failed!\n");
+      ok(GetLastError() == NTE_BAD_DATA, "%08x\n", GetLastError());

Prior to the result = CryptDecrypt(hKey, ...) line, please add a
SetLastError(0xdeadbeef); that will ensure that the following comparison of
GetLastError() to NTE_BAD_DATA isn't succeeding due to an earlier failure.
--Juan

[Qian Hong]

Dear Juan,

Thanks for reviewing!

On Fri, Jun 28, 2013 at 11:31 PM, Juan Lang  wrote:
> It's more in line with most C code to use !memcmp(...) instead of
> memcmp(...)==0. I find it easier to scan, anyway, as I've gotten used to !
> comparisons to check equality in memcmp, strcmp, and variants.

I'm glad to change, but the surrounding code use memcmp(...)==0
already, should I change that as well?
> Another minor point: it's customary to set last error prior to testing it
> when you expect it to have a certain value, e.g.:
> -
>  bad_data[cTestData[i].buflen - 1] = ~bad_data[cTestData[i].buflen -
>
> 1];
> -
>  result = CryptDecrypt(hKey, 0, TRUE, 0, bad_data, &dwLen);
> -
>  ok(!result, "CryptDecrypt should failed!\n");
> -
>  ok(GetLastError() == NTE_BAD_DATA, "%08x\n", GetLastError());
>
> Prior to the result = CryptDecrypt(hKey, ...) line, please add a
> SetLastError(0xdeadbeef); that will ensure that the following comparison of
> GetLastError() to NTE_BAD_DATA isn't succeeding due to an earlier failure.

Good point, thanks, will do that.

--
Regards,
Qian Hong

-
http://www.winehq.org

[Juan Lang]

On Fri, Jun 28, 2013 at 9:16 AM, Qian Hong  wrote:
> Dear Juan,
>
> Thanks for reviewing!
>
> On Fri, Jun 28, 2013 at 11:31 PM, Juan Lang  wrote:
> > It's more in line with most C code to use !memcmp(...) instead of
> > memcmp(...)==0. I find it easier to scan, anyway, as I've gotten used to
>
> !
> > comparisons to check equality in memcmp, strcmp, and variants.
>
> I'm glad to change, but the surrounding code use memcmp(...)==0
> already, should I change that as well?

Ah. Thanks for following the existing style then :-/ No, don't bother
changing the existing code. I leave it up to you whether to follow my
suggestion or ignore it, either is fine in this case.
--Juan

[Qian Hong]

On Sat, Jun 29, 2013 at 12:34 AM, Juan Lang  wrote:
> Ah. Thanks for following the existing style then :-/ No, don't bother
> changing the existing code. I leave it up to you whether to follow my
> suggestion or ignore it, either is fine in this case.

Patch sent, thanks Juan Lang and Daniel Jeliński for so many helps!

--
Regards,
Qian Hong

-
http://www.winehq.org