485127 - bltest crashes when attempting rc5_cbc or rc5_ecb

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

I was trying to do some quick performance testing of all algorithms. I kept getting core files with RC5.

I discovered that RC5 is not in softoken and seems not to have been there for a while. The code is surrounded with #ifdef NSS_SOFTOKEN_DOES_RC5 .

But bltest still accepts commands like :

./bltest -E -m rc5_cbc -g 16 -p 100 -4 2 -i ./test -o /dev/null
./bltest -E -m rc5_ecb -g 16 -p 100 -4 2 -i ./test -o /dev/null

And they will crash with an obscure stack :

  [1] 0x0(0x0, 0x0, 0xfffffd7ffebcef08, 0x0, 0x7379a0, 0x20000), at 0x0
  [2] cipherDoOp(cipherInfo = 0x716540), line 2338 in "blapitest.c"
  [3] ThreadExecTest(data = 0x716540), line 3129 in "blapitest.c"
=>[4] _pt_root(arg = 0x717970), line 228 in "ptthread.c"
  [5] _thr_setup(0x0, 0x0, 0x0, 0x0, 0x0, 0x0), at 0xfffffd7fff006deb
  [6] _lwp_start(0x0, 0x0, 0x0, 0x0, 0x0, 0x0), at 0xfffffd7fff007020

The crash is because the content of the cipherInfo is actually invalid.

The fix should be to refuse to do those mechanisms that are not implemented and give a usage page.

Incidently, the usage page should probably mention the list of the supported mechanisms. That would make things much easier. bltest -H has a list of ECC curves, but not mechanisms.

Comment 1

[Julien Pierre]

Attachment: Fix crash (checked in)

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369221 [details] [diff] [review]
Fix crash (checked in)

Two issues:

1,
>+#if NSS_SOFTOKEN_DOES_RC5
>+#ifdef NSS_SOFTOKEN_DOES_RC5

These tests should all be consistent, either #if or #ifdef.

>@@ -2918,7 +2925,6 @@ blapi_selftest(bltestCipherMode *modes, 
>     finished = PR_FALSE;
>     nummodes = (numModes == 0) ? NUMMODES : numModes;
>     for (i=0; i < nummodes && !finished; i++) {
>-	if (i == bltestRC5_ECB || i == bltestRC5_CBC) continue;

That should be ifdef'ed, not deleted, I believe.

Comment 3

[Julien Pierre]

Nelson,

1. That's a cosmetic issue, I can fix it at checkin

2. No, actually it is fine to delete it. The RC5 options themselves are ifdef'ed above in the source file. Therefore, by deleting this statement, if NSS_SOFTOKEN_DOES_RC5 is not defined, there is no need to skip any test, since the RC5 option won't be encountered. And if it is defined, it will be encountered, and with the statement deletion, it will now be run where it wasn't before, which I think is correct. So, please reconsider your review on this patch.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369221 [details] [diff] [review]
Fix crash (checked in)

I agree that the first of the two issues is something that, by itself, didn't require r-.  I gave r- for the second.  But you've convinced me that the second is OK, so r+, provided you fix the if/ifdef issue.

Comment 5

[Julien Pierre]

Comment on attachment 369221 [details] [diff] [review]
Fix crash (checked in)

Thanks, Nelson.

I checked in the patch with the #if changed to #ifdef.

Checking in blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.57; previous revision: 1.56
done