bltest crashes when attempting rc5_cbc or rc5_ecb

RESOLVED FIXED in 3.12.3

Status

NSS
Tools
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

2.50 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
OS: Solaris → All
Priority: -- → P2
Hardware: x86 → All
(Assignee)

Comment 1

9 years ago
Created attachment 369221 [details] [diff] [review]
Fix crash (checked in)
Assignee: nobody → julien.pierre.boogz
Attachment #369221 - Flags: review?(nelson)
Attachment #369221 - Flags: review?(nelson) → review-
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.
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #369221 - Flags: review- → review?(nelson)
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.
Attachment #369221 - Flags: review?(nelson) → review+
(Assignee)

Comment 5

9 years ago
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
Attachment #369221 - Attachment description: Fix crash → Fix crash (checked in)
(Assignee)

Updated

9 years ago
Target Milestone: --- → 3.12.3
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.