746842 - Improve blapitest testing of the CBC mode of block ciphers

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch, and fix aes_cbc test input file

The block cipher CBC mode self test performed by blapitest
uses an input file of only one block of data and calls the
encrypt function only once.  So it does not test whether the
encrypt function properly sets the last ciphertext block as
the new IV.

The attached patch changes the self test input file for
aes_cbc to contain two blocks of data, and changes blapitest.c
to call the encrypt function twice, first on the first 16
bytes of input and then on the remaining input.  This assumes
16 is a multiple of the block size.

The self test input files for other block ciphers in CBC mode
needs to be changed in the same way.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch, and fix aes_cbc test input file (v2)

I fixed compiler warnings (some were in unrelated code), and
eliminated another local variable also named "input" to avoid
confusion.  That other "input" variable was added to keep the
lines shorter than 80 characters.  I solved that problem by
reformatting.

Comment 2

[Wan-Teh Chang]

Patch checked in on the NSS trunk (NSS 3.14).

Checking in blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.67; previous revision: 1.66
done
Checking in tests/aes_cbc/ciphertext0;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/aes_cbc/ciphertext0,v  <--  ciphertext0
new revision: 1.2; previous revision: 1.1
done
Checking in tests/aes_cbc/plaintext0;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/aes_cbc/plaintext0,v  <--  plaintext0
new revision: 1.2; previous revision: 1.1
done

Comment 3

[Kai Engert (:KaiE:)]

Did this break tinderbox?

gcc -o Linux2.6_x86_glibc_PTH_DBG.OBJ/blapitest.o -c -g -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -fPIC -Di386 -DLINUX2_1 -m32 -Wall -Werror-implicit-function-declaration -Wno-switch -pipe -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_tinderbox -D_REENTRANT -DNSS_ENABLE_ECC -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -DNSS_USE_STATIC_LIBS -I../../../nss/lib/softoken -I../../../../dist/Linux2.6_x86_glibc_PTH_DBG.OBJ/include -I../../../../dist/public/nss -I../../../../dist/private/nss -I../../../../dist/public/seccmd -I../../../../dist/public/dbm -I../../../../dist/public/softoken  blapitest.c
blapitest.c:513: error: expected =, ,, ;, asm or __attribute__ before * token
blapitest.c: In function pubkeyInitKey:
blapitest.c:1941: error: implicit declaration of function getECParams
blapitest.c:1941: warning: assignment makes pointer from integer without a cast

Comment 4

[Kai Engert (:KaiE:)]

I notice this was checked in yesterday. Sorry, bonsai is broken. It seems bug 744651 is more likely to have caused the breakge.

Comment 5

[Elio Maldonado]

Is was not just  the changes in sectutil/basicutil but a change I had made earlier to blapitest itself that I think caused the breakage. As part of 744651 I made a change a that though it built on my system it breaks Tinderbox. This is the change 
diff -u -p -r1.65 -r1.67
--- mozilla/security/nss/cmd/bltest/blapitest.c        20 Mar 2012 14:46:52 -0000        1.65
+++ mozilla/security/nss/cmd/bltest/blapitest.c        8 May 2012 00:20:18 -0000        1.67
@@ -13,7 +13,7 @@
 #include "prsystem.h"
 #include "plstr.h"
 #include "nssb64.h"
-#include "secutil.h"
+#include "basicutil.h"
 #include "plgetopt.h"
 #include "softoken.h"
 #include "nspr.h"
....
A grep -r SECKEYECParams mozilla/security/nss shows me
mozilla/security/nss/lib/cryptohi/keythi.h:typedef SECItem SECKEYECParams;
...
so SECKEYECParams, the return type of getECParameters, is defined in a higher-level header that is no longer included.
It was included before indirectly. Previously, blapitest.h included secutil.h, secutil.h included key.h which in turn includes keythi.h (that's all it is) and it is this one that has the needed typedef. 

If we could move that typedef to a lower level header that the others include that would solve the problem. Right now I don't know were to move it. Any ideas?

If you don't mind as temporary solution I could replicate 'typedef SECItem SECKEYECParams;' in blapitest.c of blapitest.h. Is not what we like doing but it would restore Tinderbox to green until I find the proper fix. 

As I write this I think I saw you that have backed out the latest changes for Bug 746842. Let's see if if helps, at least temporarily.

Comment 6

[Elio Maldonado]

We don't need to replicate any typedefs, just change getECParams to ruetur SECItem * which is what SECKEYECParams * is after all.

Comment 7

[Elio Maldonado]

Attachment: Use SECItem directly

A simpler and cleaner fix. I hope you don't mind if I check it in and fix Tinderbox before you have had a chance to review it.

Comment 8

[Kai Engert (:KaiE:)]

Your proposed change probably works and makes compilers happy, but it should be preferred to use the more detailed type.

I believe the use of the type with the more detailed name is a "kind of documentation".

I'm fine with your change for now, but I propose to find a way to keep using the more detailed type.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 622272 [details] [diff] [review]
Use SECItem directly

r=kaie but please undo this change later.

Comment 10

[Kai Engert (:KaiE:)]

reopening until we get the proper patch

Comment 11

[Wan-Teh Chang]

Comment on attachment 622272 [details] [diff] [review]
Use SECItem directly

Elio: please move this patch to bug 744651.  Thanks.

Comment 12

[Wan-Teh Chang]

Please use bug 744651 for the blapitest.c compilation error reported
in comment 3.