1236720 - Provide sym key derive mechanism as result of encryption of message

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Elio Maldonado]

Originally reported downstream in Fedora (and RHEL-7) by Jack Magne

Description of problem:
In Certificate Development we are trying to implement a later secure channel protocol. One of the things we need to do , is to be able to derive a symmetric key based in the encryption of some data with the original symmetric key.

In order to maintain FIPS compliance we want to not have to do this operation in the clear and have it done by NSS in the given token. Here is an example of what we want to do:

PK11_Derive( master , CKM_DES_ECB_ENCRYPT_DATA , &param , CKM_CONCATENATE_BASE_AND_KEY  , CKA_DERIVE, 0);

Ideally we need to be able to do some sort of CBC encrytion on this message data. If there is a mechanism that gives us that as well, that would be great to have also.

Version-Release number of selected component (if applicable):
Latest Fedora, 21,20 if possible.

How reproducible: Always.

Steps to Reproduce:
1. Try to use the given mechanism in code with NSS.

Actual results:
An error will be returned saying something about mechanism not supported.

Expected results:
The mechanism should ideally work in the soft token.

Additional info:

Comment 1

[Elio Maldonado]

Attachment: Test case for issue

Comment 2

[Elio Maldonado]

Attachment: makefile for test case

Comment 3

[Elio Maldonado]

Attachment: Fix from Bob Relyea

Bob's patch applied on Fedora and RHEL-7.1, slightly adapted for the upstream sources.

Comment 4

[Elio Maldonado]

Bob, Could you please choose the reviewer for your patch?

Comment 5

[Robert Relyea]

Or Martin. It's not a complicated patch.

Comment 6

[Elio Maldonado]

Comment on attachment 8703881 [details] [diff] [review]
Fix from Bob Relyea

Review of attachment 8703881 [details] [diff] [review]:
-----------------------------------------------------------------

The patch has rotten as there has been a lot of changes since it was originally submitted. Will submit an updates version soon.

::: lib/softoken/pkcs11.c
@@ +450,2 @@
>       {CKM_EXTRACT_KEY_FROM_KEY,		{1, 32, CKF_DERIVE},   PR_FALSE}, 
> +     {CKM_EXTRACT_KEY_FROM_KEY,		{1, 32, CKF_DERIVE},   PR_FALSE}, 

Bob, this and the previous entry are identical. Is that waht you intended?

Comment 7

[Elio Maldonado]

Bob, see question in Comment 6.

Comment 8

[Elio Maldonado]

Attachment: Bob's patch updated - V2

Bob's patch updated for the state of the current sources. Removed a duplicated entry I noticed from looking at previous patch. See previous comment. Also changed the indentation and use of white space to follow nss style guidelines as much as possible. Requesting Martin's review as Bob had suggested earlier.

Comment 9

[Elio Maldonado]

Comment on attachment 8892978 [details] [diff] [review]
Bob's patch updated - V2

Adding Bob as co-reviewer to ensure I didn't do violence to his original patch.

Comment 10

[Elio Maldonado]

Attachment: makefile for running test case against patched and unpatched nss

Slight improvement to the "makefile for test case" where I add targets to easy testing. Assumes NSS is installed in the system which doesn't have the fix. 

This is how I tested:

$ make all
cc -o encrypt_derive_test -g `pkg-config --cflags nss` `pkg-config --libs nss` encrypt_derive_test.c

$ make install
[ ! -d ~/bin ] && mkdir ~/bin || echo "directory exists"
directory exists
cp encrypt_derive_test ~/bin

$ make test-should-fail
(./encrypt_derive_test)
Testing DES ECB..FAILED
PK11_Derive-DES ECB: Success
make: *** [Makefile:24: test-should-fail] Error 1

$ make test-should-pass 
(LD_LIBRARY_PATH=/home/emaldonado/code/dist/Linux4.11_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib ./encrypt_derive_test)
Testing DES ECB..PASSED
Testing DES CBC..PASSED
Testing DES3 ECB..PASSED
Testing DES3 CBC..PASSED
Testing AES ECB..PASSED
Testing AES CBC..PASSED
Testing CAMELLIA ECB..PASSED
Testing CAMELLIA CBC..PASSED
Testing SEED ECB..PASSED
Testing SEED CBC..PASSED
tests passed

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8892978 [details] [diff] [review]
Bob's patch updated - V2

Review of attachment 8892978 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see some more concrete testing of this before I review it.  If that command-line tool could be ported into some gtests, that would be ideal.

Comment 12

[Elio Maldonado]

Attachment: Bob's patch updated - V3, patch 1 of 2

test cases as gtest coming next

Comment 13

[Elio Maldonado]

Attachment: Test case for issue as gtest added to pk11_gets - patch 2 of 2

Converted to a gtest as Martin requested.

Comment 14

[Elio Maldonado]

Attachment: runs the pk11_gtests

Martin, I adapted a script one that you shared with me a while back for ssl_getsts with small modifications I can now use it this issue. To quote you:
"Run the attached script, which is very convenient for debugging. I results in seconds, unlike the regular scripts, which take a lot longer just to >get through setup steps. All I need to do is pass part of the name of the test as >an argument (such as "EncryptDerive*") and it runs all the matching tests.
Pass it a -d option and it runs gdb (-r for rr if you have that)."

Comment 15

[Martin Thomson [:mt:]]

Comment on attachment 8894600 [details] [diff] [review]
Test case for issue as gtest added to pk11_gets - patch 2 of 2

Review of attachment 8894600 [details] [diff] [review]:
-----------------------------------------------------------------

See other comments about try.

::: gtests/pk11_gtest/pk11_encrypt_derive_unittest.cc
@@ +26,5 @@
> +  } TestCase;
> +
> +  const TestCase table[] = {
> +    { "DES ECB",     TYPE_STR, CKM_DES_ECB_ENCRYPT_DATA,  CKM_DES_ECB,  8, 16, 1 },
> +    { "DES CBC",     TYPE_DES, CKM_DES_CBC_ENCRYPT_DATA,  CKM_DES_CBC,  8, 16, 1 },

I don't think that we have to continue to take DES seriously.  The same goes for 3DES.  The block size of DES makes it vulnerable to a range of attacks.

Is there a strong requirement for these new functions with 3DES?

@@ +60,5 @@
> +        unsigned int test_out_len;
> +        SECItem param, e_param;
> +        SECItem *data;
> +        SECStatus rv;
> +        unsigned int blocksize = table.blocksize;

This isn't C, so can you please move the declaration of the variable to its first use.

@@ +62,5 @@
> +        SECItem *data;
> +        SECStatus rv;
> +        unsigned int blocksize = table.blocksize;
> +
> +        slot = PK11_GetBestSlot(table.derive_mech, NULL);

Use ScopedPK11SlotInfo (see cpputil/scoped_ptrs.h)

@@ +68,5 @@
> +
> +        baseKey = PK11_TokenKeyGenWithFlags(
> +                slot, table.encrypt_mech, 
> +                NULL, 16, NULL, CKF_ENCRYPT|CKF_DERIVE, 0, NULL);
> +        ASSERT_NE(nullptr, baseKey);

Use ScopedPK11SymKey (as above).

@@ +70,5 @@
> +                slot, table.encrypt_mech, 
> +                NULL, 16, NULL, CKF_ENCRYPT|CKF_DERIVE, 0, NULL);
> +        ASSERT_NE(nullptr, baseKey);
> +
> +        switch (table.param_type) {

Can you factor the creation of the parameter into a separate function?

@@ +82,5 @@
> +                break;
> +            case TYPE_AES:
> +                aes.pData = test_string;
> +                aes.length = KEYSIZE;
> +                PORT_Memcpy(IV, aes.iv, blocksize);

You shouldn't need to copy the IV anywhere.

@@ +98,5 @@
> +                e_param.data = IV;
> +                e_param.len = blocksize;
> +                break;
> +
> +            newKey = PK11_Derive(baseKey, table.derive_mech, &param,

The remainder of this function is inside the switch statement.  That means it never gets run.

@@ +124,5 @@
> +        }
> +    }
> +};
> +
> +TEST_F(EncryptDerive, Test_DES_ECB) {

You can use TEST_P and have the table parameters be the arguments.  See the very end of gtests/ssl_gtest/ssl_ciphersuite_unittest.cc for how to do that.

But I wouldn't use the table at all.  I would pass the parameters directly and make multiple tests:

```
TEST_F(EncryptDeriveTests, Test_AES_ECB) {
  SECItem derive_param;
  MakeAesDeriveParam(&derive_param);
  DataBuffer derived_key;
  Derive(&derive_param, &derived_key);

  SECItem encrypt_param;
  MakeAesEncryptParam(&encrypt_param);
  DataBuffer encrypted_key;
  Encrypt(&encrypt_param, &encrypted_key);

  EXPECT_EQ(encrypted_key, derived_key);
}
```

All the parameters (such as IV, key, etc...) can be static members of the class.

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8894598 [details] [diff] [review]
Bob's patch updated - V3,  patch 1 of 2

Review of attachment 8894598 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/pkcs11c.c
@@ +6268,5 @@
>  }
>  #endif /* NSS_DISABLE_ECC */
>  
>  /*
> + *  Handle The derive from a block encryption cipher

This comment doesn't parse.

@@ +6271,5 @@
>  /*
> + *  Handle The derive from a block encryption cipher
> + */
> +CK_RV
> +sftk_DeriveEncrypt(SFTKObject *key, CK_ULONG keySize, void *cipherInfo, 

Formatting is a bit off.  Can you run this on nss-try?  <https://wiki.mozilla.org/NSS:TryServer>.  That should pick up things like this.

@@ +6987,5 @@
> +                crv = sftk_DeriveEncrypt(key, keySize, cipherInfo, 8,
> +                                         desEncryptPtr->pData, desEncryptPtr->length,
> +                                         (SFTKCipher) DES_Encrypt);
> +            DES_DestroyContext(cipherInfo, PR_TRUE);
> +            break;

I really don't think we want to be adding these features for DES.  No one should be using DES.  Probably the same for 3DES too, though that might have a stronger use case (does it?).  I'm ambivalent regarding SEED and CAMELLIA; I wouldn't have.

Comment 17

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #15)
> Comment on attachment 8894600 [details] [diff] [review]
> Test case for issue as gtest added to pk11_gets - patch 2 of 2
> 
> Review of attachment 8894600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See other comments about try.
> 
> ::: gtests/pk11_gtest/pk11_encrypt_derive_unittest.cc
> @@ +26,5 @@
> > +  } TestCase;
> > +
> > +  const TestCase table[] = {
> > +    { "DES ECB",     TYPE_STR, CKM_DES_ECB_ENCRYPT_DATA,  CKM_DES_ECB,  8, 16, 1 },
> > +    { "DES CBC",     TYPE_DES, CKM_DES_CBC_ENCRYPT_DATA,  CKM_DES_CBC,  8, 16, 1 },
> 
> I don't think that we have to continue to take DES seriously.  The same goes
> for 3DES.  The block size of DES makes it vulnerable to a range of attacks.
> 
> Is there a strong requirement for these new functions with 3DES?

Jack, Could you answer that question?

> 
> @@ +60,5 @@
> > +        unsigned int test_out_len;
> > +        SECItem param, e_param;
> > +        SECItem *data;
> > +        SECStatus rv;
> > +        unsigned int blocksize = table.blocksize;
> 
> This isn't C, so can you please move the declaration of the variable to its
> first use.

Will do.
> 
> @@ +62,5 @@
> > +        SECItem *data;
> > +        SECStatus rv;
> > +        unsigned int blocksize = table.blocksize;
> > +
> > +        slot = PK11_GetBestSlot(table.derive_mech, NULL);
> 
> Use ScopedPK11SlotInfo (see cpputil/scoped_ptrs.h)
> 
> @@ +68,5 @@
> > +
> > +        baseKey = PK11_TokenKeyGenWithFlags(
> > +                slot, table.encrypt_mech, 
> > +                NULL, 16, NULL, CKF_ENCRYPT|CKF_DERIVE, 0, NULL);
> > +        ASSERT_NE(nullptr, baseKey);
> 
> Use ScopedPK11SymKey (as above).

Will do.
> 
> @@ +70,5 @@
> > +                slot, table.encrypt_mech, 
> > +                NULL, 16, NULL, CKF_ENCRYPT|CKF_DERIVE, 0, NULL);
> > +        ASSERT_NE(nullptr, baseKey);
> > +
> > +        switch (table.param_type) {
> 
> Can you factor the creation of the parameter into a separate function?

Will do.
> 
> @@ +82,5 @@
> > +                break;
> > +            case TYPE_AES:
> > +                aes.pData = test_string;
> > +                aes.length = KEYSIZE;
> > +                PORT_Memcpy(IV, aes.iv, blocksize);
> 
> You shouldn't need to copy the IV anywhere.

Yes.

> 
> @@ +98,5 @@
> > +                e_param.data = IV;
> > +                e_param.len = blocksize;
> > +                break;
> > +
> > +            newKey = PK11_Derive(baseKey, table.derive_mech, &param,
> 
> The remainder of this function is inside the switch statement.  That means
> it never gets run.

Oh yes, my bad. Likely a copy and paste error as I was transforming the original to be gtest-like.
Once I fixed that the test no longer pass. Still quite a bit of work me do on the test.

> 
> @@ +124,5 @@
> > +        }
> > +    }
> > +};
> > +
> > +TEST_F(EncryptDerive, Test_DES_ECB) {
> 
> You can use TEST_P and have the table parameters be the arguments.  See the
> very end of gtests/ssl_gtest/ssl_ciphersuite_unittest.cc for how to do that.
> 
> But I wouldn't use the table at all.  I would pass the parameters directly
> and make multiple tests:
> 
> ```
> TEST_F(EncryptDeriveTests, Test_AES_ECB) {
>   SECItem derive_param;
>   MakeAesDeriveParam(&derive_param);
>   DataBuffer derived_key;
>   Derive(&derive_param, &derived_key);
> 
>   SECItem encrypt_param;
>   MakeAesEncryptParam(&encrypt_param);
>   DataBuffer encrypted_key;
>   Encrypt(&encrypt_param, &encrypted_key);
> 
>   EXPECT_EQ(encrypted_key, derived_key);
> }
> ```
> 
> All the parameters (such as IV, key, etc...) can be static members of the
> class.

Thank you for the suggestion. It will likely make the test code easier to read and maintain. I'll give it a try. Given my very limited experience with the Google test framework and Mozilla's additions, I'll likely need some help. Will ask via email or IRC not to pollute this bug.

Comment 18

[Elio Maldonado]

Attachment: add encrypt derive support - V4

Only changes are fixes for formatting that was off as Martin noticed. Not ready for review until the tests get fixed.

Comment 19

[Elio Maldonado]

Attachment: add encrypt derive gtest

Not ready yet for review as some test are failing, just for discussion with Marting off-line. Implemented several of the suggestions Martin provided in review though not all. Making incremental changes so it should not be hard to compare with the original. 

The AES, CAMELLIA, and 3DES tests {ECB & CBC} are passing but the DES and SEED ones are failing. In all cases the failure occurs when trying to create the new key where PK11_Derive return null.
+        ScopedPK11SymKey newKey(
+            PK11_Derive(
+                     baseKey.get(),
+                     derive_mech, &derive_param,
+                     has_check_sum ? CKM_DES3_CBC : CKM_AES_CBC,
+                     CKA_DECRYPT, keysize));
+
+         ASSERT_NE(nullptr, newKey.get());

With the original stand-alone encrypt_derive_test.c test from Bob they all passing.  It's hard me to debug gtests so I'll need some help doing this.

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8894607 [details]
runs the pk11_gtests

You don't need my review of this :)

Comment 21

[Robert Relyea]

Re: comment 6. Yes that was a cut and paste error The duplicate should be removed (which to did in the current patch).

Re: comment 15. There are still some legacy smart card usage of 2DES (of all things).The cards are finally moving to AES, but there's still a lot of 2 key DES cards (supported by 3DES with the proper key).

Comment 22

[Elio Maldonado]

(In reply to Elio Maldonado from comment #19)
> Created attachment 8897618 [details] [diff] [review]
> add encrypt derive gtest
> 
> Not ready yet for review as some test are failing, just for discussion with
> Marting off-line. Implemented several of the suggestions Martin provided in
> review though not all. Making incremental changes so it should not be hard
> to compare with the original. 
> 
> The AES, CAMELLIA, and 3DES tests {ECB & CBC} are passing but the DES and
> SEED ones are failing. In all cases the failure occurs when trying to create
> the new key where PK11_Derive return null.
> +        ScopedPK11SymKey newKey(
> +            PK11_Derive(
> +                     baseKey.get(),
> +                     derive_mech, &derive_param,
> +                     has_check_sum ? CKM_DES3_CBC : CKM_AES_CBC,
> +                     CKA_DECRYPT, keysize));
> +
> +         ASSERT_NE(nullptr, newKey.get());
> 
> With the original stand-alone encrypt_derive_test.c test from Bob they all
> passing.  It's hard me to debug gtests so I'll need some help doing this.

Did some debugging and this is what I found. When I call PK11_Derive this is where it fails.

Breakpoint 3, sftk_DeriveEncrypt (key=0x6c1b10, keySize=24, cipherInfo=0x6c31b0, blockSize=8,
    data=0x68f910 <nss_test::EncryptDeriveTest::test_string> "FEDCBA0987654321", len=0,
    encrypt=0x7f9dc8b48d62 <DES_Encrypt>) at pkcs11c.c:6286
6286        if (keySize && (len < keySize)) {
(gdb) print keySize
$1 = 24
(gdb) print len
$2 = 0
(gdb) n
6287            return CKR_MECHANISM_PARAM_INVALID;

This is because I have
TEST_F(EncryptDeriveTest, Test_DES_ECB) {
    Derive("DES ECB", TYPE_STR, CKM_DES_ECB_ENCRYPT_DATA, CKM_DES_ECB, 8, 0, 1);
}
If I change it to be
TEST_F(EncryptDeriveTest, Test_DES_ECB) {
    Derive("DES ECB", TYPE_STR, CKM_DES_ECB_ENCRYPT_DATA, CKM_DES_ECB, 8, 16, 1);
}
then the test passes. Similarly with "DES ECB", "SEED ECB" and "SEED CBC"

I don't is it's a flaw in the main library patch, where some additional logic may be needed, or in the test. 

I should mention that in order to run through the debugger I resorted to stop using ScopedPK11SymKe otherwise I could not hit my breakpoints.

Comment 23

[Elio Maldonado]

Attachment: add encrypt derive gtest - the one where all tests pass

Comment 24

[Elio Maldonado]

These are the changes I made to the add gtest patch.

--- /home/emaldonado/Downloads/add_encrypt_derive_gtest.patch	2017-08-29 18:27:32.839860274 -0700
+++ add_encrypt_derive_gtest.patch	2017-08-29 19:02:51.146398989 -0700
@@ -157,11 +157,11 @@
 +
 +
 +TEST_F(EncryptDeriveTest, Test_DES_ECB) {
-+    Derive("DES ECB", TYPE_STR, CKM_DES_ECB_ENCRYPT_DATA, CKM_DES_ECB, 8, 0, 1);
++    Derive("DES ECB", TYPE_STR, CKM_DES_ECB_ENCRYPT_DATA, CKM_DES_ECB, 8, 16, 1);
 +}
 +
 +TEST_F(EncryptDeriveTest, Test_DES_CBC) {
-+    Derive("DES CBC", TYPE_DES, CKM_DES_CBC_ENCRYPT_DATA,  CKM_DES_CBC,  8, 0, 1);
++    Derive("DES CBC", TYPE_DES, CKM_DES_CBC_ENCRYPT_DATA,  CKM_DES_CBC,  8, 16, 1);
 +}
 +
 +TEST_F(EncryptDeriveTest, Test_DES3_ECB) {
@@ -190,11 +190,11 @@
 +
 +// TODO: Next two are failing
 +TEST_F(EncryptDeriveTest, Test_SEED_ECB) {
-+    Derive("SEED ECB", TYPE_STR, CKM_SEED_ECB_ENCRYPT_DATA, CKM_SEED_ECB, 16, 0, 0);
++    Derive("SEED ECB", TYPE_STR, CKM_SEED_ECB_ENCRYPT_DATA, CKM_SEED_ECB, 16, 16, 0);
 +}
 +
 +TEST_F(EncryptDeriveTest, Test_SEED_CBC) {
-+    Derive("SEED CBC", TYPE_AES, CKM_SEED_CBC_ENCRYPT_DATA, CKM_SEED_CBC, 16, 0, 0);
++    Derive("SEED CBC", TYPE_AES, CKM_SEED_CBC_ENCRYPT_DATA, CKM_SEED_CBC, 16, 16, 0);
 +}
 +
 
so the test would pass and which I am not sure are needed as they may be hiding some flaw on the library.

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8902508 [details] [diff] [review]
add encrypt derive gtest - the one where all tests pass

Review of attachment 8902508 [details] [diff] [review]:
-----------------------------------------------------------------

I assume that my other comments will be addressed once these tests pass more reliably.  Here's a few more things to consider.

::: gtests/pk11_gtest/pk11_encrypt_derive_unittest.cc
@@ +11,5 @@
> +#include "scoped_ptrs.h"
> +#include "util.h"
> +
> +#define NSS_PERROR(string) \
> +    fprintf(stderr,  "%s: %s\n", string, PORT_ErrorToString(PORT_GetError()))

Please use std::cerr << string << ": " << PORT_ErrorToString(PORT_GetError()) << std::endl;

@@ +15,5 @@
> +    fprintf(stderr,  "%s: %s\n", string, PORT_ErrorToString(PORT_GetError()))
> +
> +#define NSS_PERROR2(string, string2) \
> +    fprintf(stderr,  "%s-%s: %s\n", string, string2, \
> +            PORT_ErrorToString(PORT_GetError()))

As above.

@@ +19,5 @@
> +            PORT_ErrorToString(PORT_GetError()))
> +
> +namespace nss_test {
> +
> +  typedef enum { TYPE_STR, TYPE_AES, TYPE_DES } TEST_TYPE;

No indent inside of namespace blocks.  This suggests that you need to format your code.  You can do that by running ./mach clang-format from the root directory (which requires docker and sudo) or by pushing to nss-try with 'try: -t clang-format' in the commit message.

@@ +35,5 @@
> +        TEST_TYPE param_type,
> +        CK_MECHANISM_TYPE derive_mech,
> +        CK_MECHANISM_TYPE encrypt_mech,
> +        unsigned int blocksize,
> +        unsigned int keysize,

All your changes were to make keysize non-zero.  That seems like the right thing to do, but I haven't investigated why.

@@ +36,5 @@
> +        CK_MECHANISM_TYPE derive_mech,
> +        CK_MECHANISM_TYPE encrypt_mech,
> +        unsigned int blocksize,
> +        unsigned int keysize,
> +        int has_check_sum

use `bool`

@@ +88,5 @@
> +            derive_param.data = (unsigned char *) &des;
> +            derive_param.len = sizeof(des);
> +            encrypt_param.data = iv;
> +            encrypt_param.len = blocksize;
> +            break;

default:
  ADD_FAILURE() << "Unexpected param_type " << param_type;
  return;

@@ +95,5 @@
> +        ScopedPK11SymKey newKey(
> +            PK11_Derive(
> +                     baseKey.get(),
> +                     derive_mech, &derive_param,
> +                     has_check_sum ? CKM_DES3_CBC : CKM_AES_CBC,

Seems like you could pass CKM_DES3_CBC in rather than has_check_sum.

@@ +112,5 @@
> +                 keysize, test_string, keysize);
> +         ASSERT_EQ(SECSuccess, rv);
> +         ASSERT_EQ(keysize, test_out_len);
> +         if (has_check_sum) {
> +             // Des keys have a checksum, mask them out first

If the checksum is calculated in the same way, why would the check bit be different?

Comment 26

[Elio Maldonado]

Attachment: add encrypt derive gtest V3

Addresses some, not all, the requests from Martin's last code review. Factored out the encrypt derive parameter determination into it's own function for the sake or clarity. Made some variables protected fields of the class.  This may help pave the to using subclasses in the future though I am not sure doing this will buy us much. Still using has_check_sum. Haven't found out why using key size 0 causes failure on DES and SEED though it it's okay in the stand alone test from Bob. This is still a work in progress.

Comment 27

[Elio Maldonado]

(In reply to Elio Maldonado from comment #26)
> Created attachment 8903332 [details] [diff] [review]
> add encrypt derive gtest V3
> 
> Haven't found out why using key size 0 causes failure on DES and SEED though
> it it's okay in the stand alone test from Bob. This is still a work in
> progress.

Mystery solved. While tracing using Bob's original test with "SEED CBC" I see
-------------------------------------------------------------
sftk_DeriveEncrypt (key=0x61bbc0, keySize=16, cipherInfo=0x61d260, blockSize=16, 
    data=0x7fffffffdac0 "FEDCBA09876543211234567890abcdef \333\377\377\377\177", len=16, <------------
    encrypt=0x7f719c534070 <SEED_Encrypt>) at pkcs11c.c:6277

and with "DES ECB"
Breakpoint 3, sftk_DeriveEncrypt (key=0x61bbc0, keySize=16, cipherInfo=0x61d260, blockSize=8, 
    data=0x7fffffffda60 "FEDCBA09876543211234567890abcdef`\332\377\377\377\177", len=16, <--------------
    encrypt=0x7fd9c2c96ea0 <DES_Encrypt>) at pkcs11c.c:6283
---------------------------------------------------------------

6283	    if ((len % blockSize) != 0) {
6284		return CKR_MECHANISM_PARAM_INVALID;
6285	    }
6286	    if (keySize && (len < keySize)) {
6287		return CKR_MECHANISM_PARAM_INVALID;

So the checks don't fail. Compare with what I told you when using the gtest where I had
Breakpoint 3, sftk_DeriveEncrypt (key=0x6c1b10, keySize=24, cipherInfo=0x6c31b0, blockSize=8,
    data=0x68f910 <nss_test::EncryptDeriveTest::test_string> "FEDCBA0987654321", len=0, <---------------
    encrypt=0x7f9dc8b48d62 <DES_Encrypt>) at pkcs11c.c:6286
Here len=0.

The reason is that in Bob's original test, while he has
------------------------------------------------------------------ keysize field in the struct = 0
const TEST_TABLES table[] = {
    {"DES ECB",     TYPE_STR,CKM_DES_ECB_ENCRYPT_DATA,  CKM_DES_ECB,     8,0,1},
    {"DES CBC",     TYPE_DES,CKM_DES_CBC_ENCRYPT_DATA,  CKM_DES_CBC,     8,0,1},
    {"DES3 ECB",    TYPE_STR,CKM_DES3_ECB_ENCRYPT_DATA, CKM_DES3_ECB,    8,16,1},
....
    {"SEED ECB",    TYPE_STR,CKM_SEED_ECB_ENCRYPT_DATA, CKM_SEED_ECB,   16, 0}, <-- missing entry is 0 by default
    {"SEED CBC",    TYPE_AES,CKM_SEED_CBC_ENCRYPT_DATA, CKM_SEED_CBC,   16, 0}     idem
};
The call is
	newKey = PK11_Derive(baseKey, table[i].derive_mech, &param,
		table[i].has_check_sum ? CKM_DES3_CBC : CKM_AES_CBC, 
		CKA_DECRYPT, KEYSIZE);
Near the top there is
#define KEYSIZE 16

grep "table\[i\]\." ~/Downloads/encrypt_derive_test.c | grep keysize
shows me that table[i].keysize is not used at all test.
The latest version of the gtest, with a 16, is correct. It could also be simplyfied.
Mystery solved.

Comment 28

[Elio Maldonado]

Attachment: add encrypt derive gtest - V4

Simplified the test a bit as I had mentioned in previous comment.

Comment 29

[Martin Thomson [:mt:]]

Comment on attachment 8897612 [details] [diff] [review]
add encrypt derive support - V4

Review of attachment 8897612 [details] [diff] [review]:
-----------------------------------------------------------------

HI Elio, could you take a go at using Phabricator for this change?  See my recent email to the nss-dev list for pointers.  Having tests and code in the same changeset would be ideal.

Comment 30

[Martin Thomson [:mt:]]

Hi Elio, I see that you have some build issues on Windows:

c:\Users\task_1504457088\gyp\test-env\Scripts\python.exe gyp-win-tool link-with-manifests environment.x86 True c:/Users/task_1504457088/dist/Release/bin\pk11_gtest.exe "c:\Users\task_1504457088\gyp\test-env\Scripts\python.exe gyp-win-tool link-wrapper environment.x86 False link.exe /nologo /OUT:c:/Users/task_1504457088/dist/Release/bin\pk11_gtest.exe @c:/Users/task_1504457088/dist/Release/bin\pk11_gtest.exe.rsp" 1 mt.exe rc.exe "obj\gtests\pk11_gtest\pk11_gtest.pk11_gtest.exe.intermediate.manifest" obj\gtests\pk11_gtest\pk11_gtest.pk11_gtest.exe.generated.manifest
pk11_gtest.pk11_encrypt_derive_unittest.obj : error LNK2005: "class std::vector<unsigned char,class std::allocator<unsigned char> > __cdecl hex_string_to_bytes(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" (?hex_string_to_bytes@@YA?AV?$vector@EV?$allocator@E@std@@@std@@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@2@@Z) already defined in pk11_gtest.pk11_aes_gcm_unittest.obj
c:/Users/task_1504457088/dist/Release/bin\pk11_gtest.exe : fatal error LNK1169: one or more multiply defined symbols found

I suspect that this isn't in fact your problem, but the application of the "inline" keyword to the declaration in gtests/common/util.h might fix it.

Comment 31

[Elio Maldonado]

Comment on attachment 8903681 [details] [diff] [review]
add encrypt derive gtest - V4

Review of attachment 8903681 [details] [diff] [review]:
-----------------------------------------------------------------

I will submit a new patch with the library and tests merge into one as Martin requested.

::: gtests/pk11_gtest/pk11_encrypt_derive_unittest.cc
@@ +176,5 @@
> +unsigned char EncryptDeriveTest::IV[] = "1234567890abcdef";
> +unsigned char EncryptDeriveTest::TEST_STRING[] = "FEDCBA0987654321";
> +
> +
> +int main(int argc, char **argv) {

The main is actually not needed as the test is part of the pk11-gtest suite. Looking at 
https://dxr.mozilla.org/mozilla-central/source/security/nss/gtests/common/gtests.cc
it does what we need as it initializes NSS via NSS_NoDBInit the way we want it.

@@ +180,5 @@
> +int main(int argc, char **argv) {
> +
> +  SECStatus rv;
> +
> +  rv = NSS_InitializePRErrorTable();

This is not needed as the PRErrorTable gets automatically intialized by the NSS_NoDBInt call or by ant NSS_Init call.

Comment 32

[Elio Maldonado]

Attachment: encrypt derive support - V5

library and gtest patches merged into one as requested

Comment 33

[Elio Maldonado]

Attachment: encrypt derive support - V6

Applies cleanly.

Comment 34

[Martin Thomson [:mt:]]

Elio, is there a phabricator diff for this?  I don't see one.  This is big enough that splinter is difficult to use.

Can you need-info Jack for the question about DES?  I would *really* rather not add new features for 

Bob can answer this part probably: are these new NSS-proprietary mechanisms?  I thought that we didn't add new mechanisms without talking to the PKCS#11 group first.

Comment 35

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #34)
> Elio, is there a phabricator diff for this?  I don't see one.  This is big
> enough that splinter is difficult to use.

It's now here https://phabricator.services.mozilla.com/D84

> 
> Can you need-info Jack for the question about DES?  I would *really* rather
> not add new features for 

See Bob's answer in Comment 21 and either Bob or Jack can elaborate if needed.

> 
> Bob can answer this part probably: are these new NSS-proprietary mechanisms?
> I thought that we didn't add new mechanisms without talking to the PKCS#11
> group first.

Camellia and Seed, which we have supported for quite sometime, aren't NSS-proprietary mechanisms.
See Camellia: https://tools.ietf.org/html/rfc5932 and Seed: https://tools.ietf.org/html/rfc5669
The first comes from NTT (Nippon Telegraph and Telephone) and the second from the Korean government if I reacall correctly..

Comment 36

[Eric Rescorla (:ekr)]

I think by proprietary, MT means "are there PKCS#11 code points for these".

Comment 37

[Elio Maldonado]

Not sure I understand what you mean by "are the PKCS#11 code points for these"

What I can say is I see Camellia and SEED at pkcs11t.h from
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/pkcs11t.h#890 through #907

Camellia and SEED support was added on NSS 3.12 and NSS 3.12.3, respectively.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.12_release_notes.html
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/release_notes.html/NSS_3.12.3_release_notes.html

Does that answer the question?

Comment 38

[Elio Maldonado]

Attachment: encrypt derive support - V7

Address a subset of the review request made by Martin on https://phabricator.services.mozilla.com/D87
Not addressed yet are the requests to split up the Derive function and to parameterize the test as this requires a drastic redesign of the test and for which a lot of help is needed as well as some feedback from Bob. Posting it here as I am currently having problems using Phabricator.

Comment 39

[Robert Relyea]

Comment on attachment 8914545 [details] [diff] [review]
encrypt derive support - V7

Review of attachment 8914545 [details] [diff] [review]:
-----------------------------------------------------------------

There is a missing sanity test. The MAX_KEY_SIZE is probably too small.

There are also some style issues. you can reduce the number of code blocks in NSC_Derive by half with a small change suggested in my review. That's a win, because NSC_Derive() need to be broken up even more than it currently is.

I'd also like a reorder on the parameters for your new function.

Some other style issues.

::: lib/softoken/pkcs11c.c
@@ +6245,5 @@
> + *  Handle the derive from a block encryption cipher
> + */
> +CK_RV
> +sftk_DeriveEncrypt(SFTKObject *key, CK_ULONG keySize, void *cipherInfo,
> +                   int blockSize, unsigned char *data, CK_ULONG len, SFTKCipher encrypt)

NIT, I would prefer the order to be:
encrypt, cipherInfo, blocksize, key, keySize, data, len

First three are three are related to the cipher, the next two related to the key, the last 2 the actual data.

@@ +6248,5 @@
> +sftk_DeriveEncrypt(SFTKObject *key, CK_ULONG keySize, void *cipherInfo,
> +                   int blockSize, unsigned char *data, CK_ULONG len, SFTKCipher encrypt)
> +{
> +/* large enough for a 512-bit key */
> +#define kMAX_KEY_SIZE 16

16*8 = 128, not big enough for the 512-bit key (or even a 256 bit key).
kMAX_KEY_SIZE isn't normal NSS constant style, use MAX_KEY_SIZE or MAX_DERIVE_KEY_SIZE or if you want to put it in a header SFTK_MAX_DERIVE_KEY_SIZE.

@@ +6257,5 @@
> +    CK_RV crv;
> +
> +    if ((len % blockSize) != 0) {
> +        return CKR_MECHANISM_PARAM_INVALID;
> +    }

Check for len <= kMAX_KEY_SIZE so you don't overflow your constrainted tmpdata buffer and stomp on your stack.

@@ +6332,5 @@
>      int i;
>      unsigned int outLen;
>      unsigned char sha_out[SHA1_LENGTH];
>      unsigned char key_block[NUM_MIXERS * SFTK_MAX_MAC_LENGTH];
> +    unsigned char des3key[24];

NIT: 24 is a bit magic. I'm sure we have a define for DES3 Key Size.

@@ +6938,5 @@
>              break;
>          }
>  
> +/* TODO: to be removed */
> +#if (0)

OK, I was worried we were removed DES_ECB, but we've never implemented it, so it's OK not to implement it.

@@ +7003,5 @@
> +            } else {
> +               crv = CKR_KEY_SIZE_RANGE; break;
> +            }
> +            cipherInfo =  DES_CreateContext(des3key, desEncryptPtr->iv,
> +                                            NSS_DES_EDE3_CBC, PR_TRUE);

The only difference between CKM_DES3_CBC_ENCRYPT_DATA and CKM_DES3_ECB_ENCRYPT_DATA is the setting of the mode (NSS_DES_EDE3_CBC versus NSS_DES_EDE3 and the setting of the iv (desEncryptPtr->iv versus NULL). Make them the same common block and either do:

case CKM_DES3_ECB_ENCRYPT_DATA:
      mode = NSS_DES_EDE3;
      iv = NULL;
      goto do_des3_encrypt;
case CKM_DES3_CBC_ENCRYPT_DATA:
      mode = NSS_DES_EDE3_CBC;
      iv = desEncryptPtr->iv
do_des3_encrypt:
        ....


or 

case CKM_DES3_ECB_ENCRYPT_DATA:
case CKM_DES3_CBC_ENCRYPT_DATA:
       if (mechanism == CKM_DES_ECB_ENCRYPT_DATA) {
             mode = NSS_DES_EDE3;
             iv = NULL;
       } else {
             mode = NSS_DES_EDE3_CBC;
             iv = desEncryptPtr->iv;
       }
        ......

@@ +7029,5 @@
> +            stringPtr->pData, stringPtr->ulLen, (SFTKCipher) AES_Encrypt);
> +            AES_DestroyContext(cipherInfo, PR_TRUE);
> +            break;
> +
> +        case CKM_AES_CBC_ENCRYPT_DATA:

Same issue as DES3

@@ +7060,5 @@
> +                                    (SFTKCipher) Camellia_Encrypt);
> +            Camellia_DestroyContext(cipherInfo, PR_TRUE);
> +            break;
> +
> +        case CKM_CAMELLIA_CBC_ENCRYPT_DATA:

Same issue as DES3

@@ +7090,5 @@
> +                                     (SFTKCipher) SEED_Encrypt);
> +            SEED_DestroyContext(cipherInfo, PR_TRUE);
> +            break;
> +
> +        case CKM_SEED_CBC_ENCRYPT_DATA:

Same issue as DES3

Comment 40

[Robert Relyea]

Elio, what kind of failures are you seeing. Unless you are testing with a key >128 bits, the code should work.

Comment 41

[Elio Maldonado]

(In reply to Robert Relyea from comment #40)
> Elio, what kind of failures are you seeing. Unless you are testing with a
> key >128 bits, the code should work.

No more failures, I had made mistake which I fixed a while back, See Comment 27.

Comment 42

[Elio Maldonado]

Comment on attachment 8914545 [details] [diff] [review]
encrypt derive support - V7

Review of attachment 8914545 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the feedback and suggestions.

::: lib/softoken/pkcs11.c
@@ +429,5 @@
> +/* NOTE: to be removed */
> +#if (0)
> +    { CKM_DES_ECB_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },
> +    { CKM_DES_CBC_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },
> +#endif

Will remove.

::: lib/softoken/pkcs11c.c
@@ +6245,5 @@
> + *  Handle the derive from a block encryption cipher
> + */
> +CK_RV
> +sftk_DeriveEncrypt(SFTKObject *key, CK_ULONG keySize, void *cipherInfo,
> +                   int blockSize, unsigned char *data, CK_ULONG len, SFTKCipher encrypt)

I like the suggestion, it reads better.

@@ +6248,5 @@
> +sftk_DeriveEncrypt(SFTKObject *key, CK_ULONG keySize, void *cipherInfo,
> +                   int blockSize, unsigned char *data, CK_ULONG len, SFTKCipher encrypt)
> +{
> +/* large enough for a 512-bit key */
> +#define kMAX_KEY_SIZE 16

Don't know how I got 16, since 512 / 8 = 64 
In softoknt.h I can add
+#define SFTK_MAX_DERIVE_KEY_SIZE 64

@@ +6257,5 @@
> +    CK_RV crv;
> +
> +    if ((len % blockSize) != 0) {
> +        return CKR_MECHANISM_PARAM_INVALID;
> +    }

Will do.

@@ +6332,5 @@
>      int i;
>      unsigned int outLen;
>      unsigned char sha_out[SHA1_LENGTH];
>      unsigned char key_block[NUM_MIXERS * SFTK_MAX_MAC_LENGTH];
> +    unsigned char des3key[24];

No we don't have DES key size defined anywhere
we do have {AES|SEED}_BLOCK_SIZE define in blapit.h
Other heaaer could be softokn.h where we alreday have
#define NSS_SOFTOKEN_DEFAULT_CHUNKSIZE 2048
There I could add:
#define DES_BLOCK_SIZE 8            /* bytes */
#define MAX_DES3_KEY_SIZE 24        /* DES_BLOCK_SIZE * 3 */
#define SFTK_MAX_DERIVE_KEY_SIZE 64

@@ +7003,5 @@
> +            } else {
> +               crv = CKR_KEY_SIZE_RANGE; break;
> +            }
> +            cipherInfo =  DES_CreateContext(des3key, desEncryptPtr->iv,
> +                                            NSS_DES_EDE3_CBC, PR_TRUE);

I like the second style better. Something like this:

 case CKM_DES3_ECB_ENCRYPT_DATA:
 case CKM_DES3_CBC_ENCRYPT_DATA: {
    int mode;
    unsigned char *iv;
    if (mechanism == CKM_DES3_ECB_ENCRYPT_DATA) {
       stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)
               pMechanism->pParameter;
       mode = NSS_DES_EDE3;
    } else {
       mode = NSS_DES_EDE3_CBC;
       desEncryptPtr =
            (CK_DES_CBC_ENCRYPT_DATA_PARAMS *)
              pMechanism->pParameter;
       iv = desEncryptPtr->iv;
    }
    if (att->attrib.ulValueLen == 16) {
       PORT_Memcpy(des3key, att->attrib.pValue, 16);
       PORT_Memcpy(des3key + 16, des3key, 8);
    } else if (att->attrib.ulValueLen == 24) {
       PORT_Memcpy(des3key, att->attrib.pValue, 24);
    } else {
       crv = CKR_KEY_SIZE_RANGE;
       break;
    }
    cipherInfo = 
          DES_CreateContext(des3key, iv, mode, PR_TRUE);
    PORT_Memset(des3key, 0, 24);
    if (cipherInfo == NULL) {
      crv = CKR_HOST_MEMORY;
      break;
    }
    crv = sftk_DeriveEncrypt((SFTKCipher) DES_Encrypt,
        cipherInfo, 8, key, keySize,
        (mode == NSS_DES_EDE3)
           ? stringPtr->pData : desEncryptPtr->pData,
        (mode == NSS_DES_EDE3)
           ? stringPtr->ulLen : desEncryptPtr->length);
    DES_DestroyContext(cipherInfo, PR_TRUE);
    break;
   }

   case CKM_AES_ECB_ENCRYPT_DATA:
   case CKM_AES_CBC_ENCRYPT_DATA: {
   ....

Comment 43

[Elio Maldonado]

Attachment: encrypt derive support - V8

Address Bob's requests in Comment 39.

Comment 44

[Elio Maldonado]

Comment on attachment 8915194 [details] [diff] [review]
encrypt derive support - V8

Review of attachment 8915194 [details] [diff] [review]:
-----------------------------------------------------------------

Got some arguments backwards. New one coming once I retest.

::: gtests/pk11_gtest/pk11_encrypt_derive_unittest.cc
@@ +132,5 @@
> +    CK_KEY_DERIVATION_STRING_DATA string_;
> +};
> +
> +TEST_F(EncryptDeriveTest, Test_DES3_ECB) {
> +    Derive("DES3 ECB", TYPE_STR, CKM_DES3_ECB_ENCRYPT_DATA, CKM_DES3_ECB, 8, false);

Should be
Derive("DES3 ECB", TYPE_STR, CKM_DES3_ECB_ENCRYPT_DATA, CKM_DES3_ECB, 8, true);

@@ +136,5 @@
> +    Derive("DES3 ECB", TYPE_STR, CKM_DES3_ECB_ENCRYPT_DATA, CKM_DES3_ECB, 8, false);
> +}
> +
> +TEST_F(EncryptDeriveTest, Test_DES3_CBC) {
> +    Derive("DES3 CBC", TYPE_DES, CKM_DES3_CBC_ENCRYPT_DATA, CKM_DES3_CBC, 8, false);

Derive("DES3 CBC", TYPE_DES, CKM_DES3_CBC_ENCRYPT_DATA, CKM_DES3_CBC, 8, true); 
It's the DES / DES3 that need has_check_sum true for the masking.
The ones follwoing (AES, CAMELLIA, SEED) should have it as false.

Comment 45

[Robert Relyea]

Comment on attachment 8915194 [details] [diff] [review]
encrypt derive support - V8

Review of attachment 8915194 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating. I won't r+ since elio r- the test cases, but the code itself looks good my only comment is on a comment;).

::: lib/softoken/softoknt.h
@@ +9,5 @@
>  #define _SOFTOKNT_H_
>  
>  #define NSS_SOFTOKEN_DEFAULT_CHUNKSIZE 2048
> +#define DES_BLOCK_SIZE 8            /* bytes */
> +#define MAX_DES3_KEY_SIZE 24        /* DES_BLOCK_SIZE * 3 */

The comment is wrong it should be DES_KEY_SIZE * 3. NOTE: DES_KEY_SIZE is different than DES_KEY_STRENGTH. A DES key is 64 bits while the key strength is 56. 8 bits are parity (or ignored in the case of a derived key like this).

Comment 46

[Robert Relyea]

clearing need info since I believe I supplied it with the review.

Comment 47

[Elio Maldonado]

Attachment: encrypt derive support - V9

Fixed the incorrect comment in the header that Bob pointed out.
Removed from Derive the unused test name parameter which is not needed. 
Removed also the block size and has_check_sum parameters as they can be computed of the fly from the other parameters.

Martin's request in https://phabricator.services.mozilla.com/D87#530ce3ee
is not addressed in this version of the patch. I'll elaborate on the problems I encountered in a subsequent comments.

Comment 48

[Elio Maldonado]

Comment on attachment 8916152 [details] [diff] [review]
encrypt derive support - V9

Not ready to be checked in but it's worth reviewing.

Comment 49

[Martin Thomson [:mt:]]

Comment on attachment 8916152 [details] [diff] [review]
encrypt derive support - V9

Review of attachment 8916152 [details] [diff] [review]:
-----------------------------------------------------------------

See D119

Comment 50

[Phabricator Automation]

Comment on attachment 8919354 [details]
Bug 1236720 - Provide sym key derive mechanism as result of encryption of message

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D133#3159

Comment 51

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/nss/rev/6c4aced0b56bc89a6783837079dd19d910c4ef18

Comment 52

[Elio Maldonado]

Backed out the changes: https://hg.mozilla.org/projects/nss/rev/7f7ab101aebc9c5c5d3936ec8fec1d33865d48e0
due to build bustage. Sorry, I should have used nss-try first.

Comment 53

[Elio Maldonado]

Attachment: approved patch that caused problems

Attaching the patch here for the benefits of those without phabricator access. This will allow me to explain what I think caused the build bustage.

Comment 54

[Elio Maldonado]

Comment on attachment 8920651 [details] [diff] [review]
approved patch that caused problems

Review of attachment 8920651 [details] [diff] [review]:
-----------------------------------------------------------------

This patch caused this 
I../../../dist/private/nss  pkcs11c.c
pkcs11c.c: In function ‘NSC_DeriveKey’:
pkcs11c.c:7091:24: error: ‘iv’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
             cipherInfo = SEED_CreateContext((unsigned char *)att->attrib.pValue,
             ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                             iv, mode, PR_TRUE);
                                             ~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Indeed, iv is not being set to NULL when needed. See comment below.

::: lib/softoken/pkcs11c.c
@@ +7077,5 @@
> +                stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)
> +                                pMechanism->pParameter;
> +                aesEncryptPtr = NULL;
> +                data = stringPtr->pData;
> +                len = stringPtr->ulLen;

Must add iv = NULL; here like is done for the other ones. Required for SEED_CreateContext below.

Comment 55

[Elio Maldonado]

Still have to do a new push to nss-try and make sure the bustage is gone. Being a novice with taskcluster tools I'll likely need to ping someone on IRC for help.

Comment 56

[Elio Maldonado]

Adding iv = NULL; solved the problem but found two other problems on Windows which I think I have solved. 
Last nss-try push was https://hg.mozilla.org/projects/nss-try/rev/e1fd5a602c91d041b1b16fbc14058ca57e1ff32f and 
https://treeherder.mozilla.org/#/jobs?repo=nss-try&author=[e-mail] is showing all green.
I won't land this in mercurial just yet, I prefer to wait until Martin's gets back and get his feedback.

Comment 57

[Phabricator Automation]

Comment on attachment 8919354 [details]
Bug 1236720 - Provide sym key derive mechanism as result of encryption of message

Martin Thomson [:mt:] has been removed from the revision.

https://phabricator.services.mozilla.com/D133#3449

Comment 58

[Elio Maldonado]

The latest revision of the patch is at https://phabricator.services.mozilla.com/D151 
Bob, as Martin stated there it's best to wait for your feedback and ensure we haven't done violence to your code.

Comment 59

[Elio Maldonado]

Attachment: encrypt derive support

patch from https://phabricator.services.mozilla.com/D151 
for the benefit of those who may be phabricator-challenged :-)

Comment 60

[Robert Relyea]

Comment on attachment 8921646 [details] [diff] [review]
encrypt derive support

Review of attachment 8921646 [details] [diff] [review]:
-----------------------------------------------------------------

I have some comments, but nothing I'd insist on.

bob

::: lib/softoken/pkcs11.c
@@ +432,5 @@
> +    { CKM_AES_CBC_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },
> +    { CKM_CAMELLIA_ECB_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },
> +    { CKM_CAMELLIA_CBC_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },
> +    { CKM_SEED_ECB_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },
> +    { CKM_SEED_CBC_ENCRYPT_DATA, { 1, 32, CKF_DERIVE }, PR_FALSE },

You set SFTK_MAX_DERIVE_KEY_SIZE to 64, you should probably set the max key here to 64 as well.

::: lib/softoken/pkcs11c.c
@@ +6956,5 @@
> +            } else {
> +                mode = NSS_DES_EDE3_CBC;
> +                desEncryptPtr =
> +                    (CK_DES_CBC_ENCRYPT_DATA_PARAMS *)
> +                        pMechanism->pParameter;

I'd have made this a local pointer to this context since it's not used outside this block.

@@ +7000,5 @@
> +                data = stringPtr->pData;
> +                len = stringPtr->ulLen;
> +            } else {
> +                aesEncryptPtr =
> +                   (CK_AES_CBC_ENCRYPT_DATA_PARAMS *) pMechanism->pParameter;

Same comment as the DES comment.

@@ +7041,5 @@
> +                iv = NULL;
> +            } else {
> +                stringPtr = NULL;
> +                aesEncryptPtr = (CK_AES_CBC_ENCRYPT_DATA_PARAMS *)
> +                                    pMechanism->pParameter;

CAMELLIA uses CK_AES_CBC_ENCRYPT_DATA_PARAMS ? 

It's possible, just a little surprising.

@@ +7082,5 @@
> +                iv = NULL;
> +            } else {
> +                mode = NSS_SEED_CBC;
> +                aesEncryptPtr = (CK_AES_CBC_ENCRYPT_DATA_PARAMS *)
> +                                    pMechanism->pParameter;

If camellia uses it, it's not surprising SEED does as well.

Comment 61

[Robert Relyea]

clearing need info.

Comment 62

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/nss/rev/7345a09561bb4290f7ffeba55329671cb524b9c5

Comment 63

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/33f5160584ccdf9cdcb50793b9b8919eee05afd3 for clang-format problems.

Comment 64

[Phabricator Automation]

Attachment: Bug 1236720 - Provide sym key derive mechanism as result of encryption of message, under review

Comment 65

[Phabricator Automation]

Comment on attachment 8917626 [details]
Bug 1236720 - Provide sym key derive mechanism as result of encryption of message

Elio Maldonado has approved the revision.

https://phabricator.services.mozilla.com/D119

Comment 66

[Phabricator Automation]

Attachment: Bug 1236720 - Provide sym key derive mechanism as result of encryption of message, r=mt