490238 - Create new NSS samples

Status: NEW

(NSS :: Documentation, defect, P3)

Description

[Elio Maldonado]

NSS needs a completely new set of sample applications written from scratch. 

This request comes as a result of an attempt to clean up the existing samples and the ensuing discussion in Bug 463848. They should be good examples on how to properly do cryptographic tasks with NSS, should illustrate good security practices, and proceed from the simplest task to more complex ones.  The existing samples do not meet these goals. The problem goes beyond coding and into their own design.

Comment 1

[Elio Maldonado]

Below is a copy of some of Nelson Bolyard's comments and suggestions to that effect written originally for Bug 463848

I think we need to completely redo the NSS sample code from scratch,
starting with design, and worry about coding style later.
<< clipped >>
So, I wrote some discussion about why these examples are bad, and what
good examples should do differently.  I set out to write something about what
good examples should and should not do. 

Good examples will have these properties:

1. They will start out very simple, with more complex examples coming
after the simple ones.  Start with the easy stuff, then get more complex
as you go.

2. They will exemplify good crypto practices.  They will not emphasize
the long term storage and reuse of symmetric keys for communications
between parties.  They will not exemplify the use of unauthenticated keys
sent in the clear between parties.  They will not exemplify the design of
insecure encrypted communications protocols.

3. They will exemplify good practices in the management of NSS DBs.
They won't reset the PIN every time the application is run.
They won't insist on using empty passwords.

4. They will actually work.

5. They will appear to be relevant to purposes for which people actually
use cryptography, rather than being extremely contrived.

So, I really think we need to discard the existing applications (at least some
of them) and start by designing a small set of demonstration applications.

I have written a draft set of ideas for new example programs.  I will 
add them in a forthcoming comment to this bug.

Further Comment From Nelson Bolyard

Here is a draft with some ideas for a good (?) set of sample programs.
These start simple and get more complex.
They are intended to not demonstrate insecure key management.

1. A program like MD5hash, reads file on stdin, outputs hex hash to stdout.
   - uses no cert DB and no keys.
   - command line option chooses the hash algorithm
   - loop, reading input buffer, feed into hash, repeat till EOF
   - finalize hash, output it, convert to hex, write to stdout.

2. Create a new empty DB and give it a user-supplied password. (that's it!)

3. A basic encryption/decryption and MACing program.
   Used to encrypt files for one's own use later.
   Uses token for storing data encryption keys. Key storage grows with use.
   It has two options,
   a) keygen and encryption/MACing
   b) decryption and MAC checking
   Option a does these steps:
   - Open DB for read/write, authenticate to it.
   - generate a symmetric AES key as a token object.
   - generate a second key to use for MACing, also a token object.
   - get their  CKA_IDs
   - genreate a random value to use as IV for AES CBC
   - open an input file and an output file,
   - write a header to the output that identifies the two keys by
     their CKA_IDs, May include original file name and length.
   - loop until EOF(input)
       - read a buffer of plaintext from input file,
       - MAC it, append the MAC to the plaintext
       - encrypt it using CBC, using previously created IV,
       - store the last block of ciphertext as the new IV,
       - write the cipher text to file file
    - close files
    - report success

    Option B does these steps:
    - Open DB read only, authenticate to it
    - open input file
    - read in header, get IV and CKA_IDs of two keys from it
    - find those keys in the DB token
    - Open output file
    - loop until EOF(input):
       - read a buffer of ciphertext from input file,
       - Save last block of ciphertext
       - decrypt ciphertext buffer using CBC and IV,
       - compute and check MAC, then remove MAC from plaintext
       - replace IV with saved last block of ciphertext
       - write the plain text to file file
    - close files
    - report success

4. Embellished encryption program, same purpose as program 3.
   Key storage does not grow with use.  3 options
   a) RSA key gen
   b) AES key gen, RSA encryption and AES data encryption and MACing
   c) RSA decryption and data decryption and MAC checking

   Option A generates an RSA key pair as a token object, gives it a
       user supplied label (friendly key name) so it can be found again.
   Option B finds the RSA public key by user-supplied label, then
     - opens input and output file
     - generates an AES encryption key, session object,
     - generates a MAC key, session object
     - wraps each of those keys with RSA public key, and outputs
       wrapped keys and RSA key label into output file header.
       (extra credit: signs header)
     - encryption and MACing loop, as in previous program
     - Destroy session keys
     - Close files
     - report success
   Option C opens input file, then
     - reads header including RSA key label and wrapped keys
     - finds RSA private key by label
     - unwraps two keys, creating session key objects
     - Open output file
     - decryption and MAC checking loop, as in previous program
     - Destroy session keys
     - Close files
     - report success

5. Further embellishment, useful for file transfer to others.
   Same 3 options as in program 4
   Option A generates same keys as program 4, outputs public key to file.
     (Maybe a cert request?  self signed)?
   Option B
     Like program 4, except that it reads in public key from file instead
     of looking it up by label name, and writes public key into header
     instead of a label name.  Rest is the same.
   Option C
     Like program 4, except that it reads in RSA public key, and then
     finds matching private key (by key ID).  Rest is the same.

6. Further embellishment:  use certs.  4 options
   Option A: as in program 5, output is a PKCS#10 CSR
   Option B: input and store a cert in cert DB (done by multiple users)
        Option B also used to input, store and trust CA cert.
   Option C: like program 5, but puts cert in header
   Option D: like program 5, but find key matching cert in header

The demo for program 6 runs it 4-5 times
   As Alice, make CSR
        Then
   As Alice, input Alice's cert (optional)
   As Bob,   input Alice's cert
   As Bob,   generate encrypted message for alice
   As Alice, read and decrypt Bob's message.

Additional embellishments:  Bob signs message encrypted message.
Alice verifies Bob's signature.

I invite refinements to these ideas, or an alternative proposal.

Comment 2

[Shailen]

Attachment: sample1

Comment 3

[Shailen]

Attachment: sample2

Comment 4

[Elio Maldonado]

(In reply to comment #1)
> 
> Good examples will have these properties:
> 
> 1. They will start out very simple, with more complex examples coming
> after the simple ones.  Start with the easy stuff, then get more complex
> as you go.
I agree and they should not get overly complex all of a sudden.
That is the problem with sample 3.

> 
> 2. Create a new empty DB and give it a user-supplied password. (that's it!)
This is an opportunity to also illustrate techniques for handling passwords at the price of some extra complexity.

> 3. A basic encryption/decryption and MACing program.
>    Used to encrypt files for one's own use later.
>    Uses token for storing data encryption keys. Key storage grows with use.
>    It has two options,
>    a) keygen and encryption/MACing
>    b) decryption and MAC checking
>    Option a does these steps:
>    - Open DB for read/write, authenticate to it.
>    - generate a symmetric AES key as a token object.
>    - generate a second key to use for MACing, also a token object.
>    - get their  CKA_IDs
>    - genreate a random value to use as IV for AES CBC
>    - open an input file and an output file,
>    - write a header to the output that identifies the two keys by
>      their CKA_IDs, May include original file name and length.
>    - loop until EOF(input)
>        - read a buffer of plaintext from input file,
>        - MAC it, append the MAC to the plaintext
>        - encrypt it using CBC, using previously created IV,
>        - store the last block of ciphertext as the new IV,
>        - write the cipher text to file file
>     - close files
>     - report success
> 
>     Option B does these steps:
>     - Open DB read only, authenticate to it
>     - open input file
>     - read in header, get IV and CKA_IDs of two keys from it
>     - find those keys in the DB token
>     - Open output file
>     - loop until EOF(input):
>        - read a buffer of ciphertext from input file,
>        - Save last block of ciphertext
>        - decrypt ciphertext buffer using CBC and IV,
>        - compute and check MAC, then remove MAC from plaintext
>        - replace IV with saved last block of ciphertext
>        - write the plain text to file file
>     - close files
>     - report success
> 
> 4. Embellished encryption program, same purpose as program 3.
>    Key storage does not grow with use.  3 options
>    a) RSA key gen
>    b) AES key gen, RSA encryption and AES data encryption and MACing
>    c) RSA decryption and data decryption and MAC checking
> 
>    Option A generates an RSA key pair as a token object, gives it a
>        user supplied label (friendly key name) so it can be found again.
>    Option B finds the RSA public key by user-supplied label, then
>      - opens input and output file
>      - generates an AES encryption key, session object,
>      - generates a MAC key, session object
>      - wraps each of those keys with RSA public key, and outputs
>        wrapped keys and RSA key label into output file header.
>        (extra credit: signs header)
>      - encryption and MACing loop, as in previous program
>      - Destroy session keys
>      - Close files
>      - report success
>    Option C opens input file, then
>      - reads header including RSA key label and wrapped keys
>      - finds RSA private key by label
>      - unwraps two keys, creating session key objects
>      - Open output file
>      - decryption and MAC checking loop, as in previous program
>      - Destroy session keys
>      - Close files
>      - report success
> 
> 5. Further embellishment, useful for file transfer to others.
>    Same 3 options as in program 4
>    Option A generates same keys as program 4, outputs public key to file.
>      (Maybe a cert request?  self signed)?
>    Option B
>      Like program 4, except that it reads in public key from file instead
>      of looking it up by label name, and writes public key into header
>      instead of a label name.  Rest is the same.
>    Option C
>      Like program 4, except that it reads in RSA public key, and then
>      finds matching private key (by key ID).  Rest is the same.
................
> 
> I invite refinements to these ideas, or an alternative proposal.
This is a big jump in complexity. 
Consider splitting sample3 in two or three samples. The first one will illustrate encryption/decryption only, a second one for MAC, and the third one with both. 

When doing encryption plus mac, is is necessary to append the MAC at the end of every block? This complicates the work in Option B where one has decrypt the keep track of where the MAC starts and remove it thus distracting the reader from learning the API. How abut changing Step A to keep updating the MAC at evry block and calling DigestFinal aat the the end and save the MAC after the cipher part. 

3. A simpler basic encryption/decryption and MACing program
   Used to encrypt files for one's own use later.
   Uses token for storing data encryption keys. Key storage grows with use.
   It has two options,
   a) keygen and encryption/MACing
   b) decryption and MAC checking
   Option a does these steps:
   - Open DB for read/write, authenticate to it.
   - generate a symmetric AES key as a token object.
   - generate a second key to use for MACing, also a token object.
   - get their  CKA_IDs
   - generate a random value to use as IV for AES CBC
   - open an input file and an output file,
   - write a header to the output that identifies the two keys by
     their CKA_IDs, May include original file name and length.
   - loop until EOF(input)
       - read a buffer of plaintext from input file,
       - MAC update it
       - encrypt it using CBC, using previously created IV,
       - store the last block of ciphertext as the new IV,
       - write the cipher text to the intermediate file
   - compute the final MAC final (DigestFinal)
   - write the MAC to the intermediate file as trailer
    - close files
    - report success

    Option B does these steps:
    - Open DB read only, authenticate to it
    - open input file
    - read in header, get IV and CKA_IDs of two keys from it
    - find those keys in the DB token
    - extract the MAC, it's at the end
    - set the file pointer to the start of ciphertext
    - Open output file
    - loop until EOF(input):
       - read a buffer of ciphertext from input file,
       - Save last block of ciphertext
       - decrypt ciphertext buffer using CBC and IV,
       - update MAC context with decrypted plaintext
       - replace IV with saved last block of ciphertext
       - write the plain text to file file
    - finish MACing and save resultant MAC
    - check that the computed MAC matches the extracted MAC
    - close files
    - report success (or failure if didn't macs didn't match)

I think it would simplify Option B considerably and be easier on the persons learning, implementing, and on on the reviewer. We may not need the other two simpler samples after all. 

Conflict of interest disclosure: I confess to having a vested interest in simplifying the implementor and reviewer's tasks :-)

Comments?

Comment 5

[Elio Maldonado]

Attachment: sample 3 version 1

Attaching sample3 on Shailendra's behalf. I'll will soon have many comments on it.

Comment 6

[Shailen]

Attachment: sample3 revision 2

Alternative encryption/mac sample

Comment 7

[Elio Maldonado]

(In reply to comment #5) The problem I see with sample3 version 1 is that in EncryptBlock there is a the 
rv = PK11_CipherOp(ctx, out, &outLen1, maxout, in, inLen);
is followed by 
rv = PK11_DigestFinal(ctx, out+outLen1, &outLen2, sizeof(out) - outLen1);
That's the same ctx! Isn't it wiping out the Mac that was appended earlier in MacThenEncrypt. In OpetionB  DecryptThenReoveMac calls Decrypt where the line
rv = PK11_CipherOp(ctx, out, &outLen1, maxOut, in, inLen);
is followed with a
rv = PK11_DigestFinal(ctx, out+outLen1, &outLen2, inLen-outLen1);
Again the encrypt/decrypt ctx is being used for MAcing!

I discussed this with Shailendra and our attempts get rid of the extraneous Mac operations didn't work. I haven't figured this one out. 

For that the reason I suggested the alternative (simpler) approach outlined in comment 4 and implemented in the alternate sample 3 in attachment (id=431039).  At the price of having to create a separate .header file the cryptographic operations become cleaner and easier to read and debug.

Comment 8

[Robert Relyea]

NOTE: if you need to get an intermediate result, you can use PK11_CloneContext().


rv = PK11_CipherOp (ctx, out .....);

new_ctx = PK11_CloneContext(ctx);

rv = PK11_DigestFinal(new_ctx .... );

{rest is the same}

Before to PK11_DestroyContext(new_ctx) at some point as well.



 and

Comment 9

[Robert Relyea]

Comment on attachment 425670 [details]
sample1 

r+

Some comments:

1. It's not necessary to look up the oid value. hashoid->offset is the same as the OidTag you used to look it up.

2. There is a higher level interface for doing hashing. We probably should have examples for both. (see sechash.h).

This example points out that we really need a string to oidtag conversion mechanism for hashes and ciphers. That way this sample would just work if we added new hashing algorithms. (not an issue with this sample).

bob

Comment 10

[Elio Maldonado]

Attachment: sample2 review comments in patch form

My review comments attached in the form of a patch. These are already in the version of sample2 inside of additional tiny samples that you plan to submit later on.

Comment 11

[Elio Maldonado]

Comment on attachment 425671 [details]
sample2

see suggestions in patch attachement

Comment 12

[Shailen]

Attachment: sample2 version2

Elio :

  Thanks for reviewing sample2. I have applied the patches and let me know if any further changes required.

Thanks,
Shailendra

Comment 13

[Shailen]

Attachment: sample1 version2

Hi Bob,

  In addition to the attached tar file (sample1_version2.tar), I will follow Elio's approach to attach the patch file containing the changes I made for sample1 version 2 to take care of your review comments.

Thanks and Regards,
Shailendra

Comment 14

[Shailen]

Attachment: sample1 version2 review changes in patch form

Bob :

> 1. It's not necessary to look up the oid value. hashoid->offset is the 
>      same as the OidTag you used to look it up.
 
  Changes made. Please review and let me know if any concerns.

>2. There is a higher level interface for doing hashing. We probably should have
> examples for both. (see sechash.h).

    I will try to write sample using higher level interface, but I might need your/Elio's help :)

>This example points out that we really need a string to oidtag conversion
>mechanism for hashes and ciphers. That way this sample would just work if we
>added new hashing algorithms. (not an issue with this sample).

  Is there any alternative way of writing sample without hard coding string to oidtag conversion mechanism ? Please let me.

Also I have a question to you/Elio.

1. In the patch, I changed the data type from 'PRUint64' to 'unsigned it' for variable nb. Will this be an issue? Initially the change was made from 'PRUint32' to 'PRUint64' because with 64 bit version of NSS lib, it was causing problem.

Thanks and Regards,
Shailendra

Comment 15

[Shailen]

Attachment: sample1 using sechash

I modified only the function 'DigestFile' to call sechash interfaces instead of pk11pub interfaces.

Please review and let me know if any changes required.

Note: I will capture the changes required for 'using sechash' in a separate attachment in patch form.

Comment 16

[Shailen]

Attachment: sample1 using sechash in patch form

Comment 17

[Robert Relyea]

>  Is there any alternative way of writing sample without hard coding string to
> oidtag conversion mechanism ? Please let me.

It's more a deficiency in NSS that there isn't a good way to do this. There is a way to get a PKCS #11 mechanism from a string. See cmd/pk12util/pk12util.c, The function is PKCS12U_MapCipherFromString(). A general form of the function doesn't need the check for being a PKCS 5 algorithm.

One thing it's missing is a way to determine what kind of algorithm has been selected (hashing, encryption, pbe, derive, etc).

bob

Comment 18

[Robert Relyea]

Comment on attachment 431590 [details]
sample1 version2

r+ rrelyea

Comment 19

[Robert Relyea]

> In the patch, I changed the data type from 'PRUint64' to 'unsigned it' for
> variable nb. Will this be an issue? Initially the change was made from
> 'PRUint32' to 'PRUint64' because with 64 bit version of NSS lib, it was causing
> problem.

If the interface takes unsigned int, you should pass it an unsigned int.

unsigned int length varies depending on platform.
PRUint32 is always 32 bits no matter what platform you are on.
PRUin64 is always 64 bits not matter what platform you are on.

lengths are usually unsigned int since they can, in theory, vary depending on platform and pointer size. They should have been size_t's probably.

bob

Comment 20

[Shailen]

Bob :

Thanks for the useful information.

Regards,
Shailendra

Comment 21

[Robert Relyea]

Comment on attachment 431605 [details]
sample1 using sechash

r+ rrelyea

Comment 22

[Elio Maldonado]

Comment on attachment 426511 [details]
sample 3 version 1

Giving version 1 of sample 3 an r- for the reasons stated on Comment 7; namely, the strange need to perform that mac that overrides the action of the first.  

My question then is: Shall we stick with the original requirements as outline by Nelson and try to fix sample 3 version1 or switch to the alternative one as implemented in sample3 v2?

Comment 23

[Elio Maldonado]

Comment on attachment 431039 [details]
sample3 revision 2

I will do a first review. Having a second reviewer is desirable given that I collaborated in it which may bias me a bit.

Comment 24

[Elio Maldonado]

I'm giving it a conditional r+. I few things are worth investigating.

1. The command line option for input file and and output file are potentially confusing to the user that tries Option B. It seems that the input file is not the encrypted file (output of Option A) as one would expect but the original unencrypted file. Could you clarify this? Either change the semantics or document that for the user.
2. Sample 3 works with text files. I wonder if it would work with binary files also. Could you find out. Could you try it?

3. You added a header, sample3.h, but only sample3.c uses those functions. Is it really necessary?

Otherwise, this version of sample3 is looking good.

Comment 25

[Shailen]

Attachment: Sample3 Version 3

Comment 26

[Shailen]

Hi Elio,

>1. The command line option for input file and and output file are potentially
>confusing to the user that tries Option B. It seems that the input file is not
>the encrypted file (output of Option A) as one would expect but the original
>unencrypted file. Could you clarify this? Either change the semantics or
>document that for the user.

Modified the usage for the sample3 as below

Usage:  sample3 -c <a|b> -d <dbdirpath> [-z <noisefilename>] [-p <dbpwd> | -f <dbpwdfile>] -i <ipfilename> -o <opfilename>

-c <a|b>              Specify 'a' for encrypt operation

                      Specify 'b' for decrypt operation

-d <dbdirpath>        Specify db directory path

-p <dbpwd>            Specify db password {optional]

-f <dbpwdfile>        Specify db password file {optional]

-z <noisefilename>    Specify noise file name {optional]

-i <ipfilename>       Specify an input file name

-o <opfilename>       Specify an output file name

Note :  For encrypt, it takes <ipfilename> as an input file and produces
        <ipfilename>.inc and <ipfilename>.header as intermediate output files.

        For decrypt, it takes <ipfilename>.inc and <ipfilename>.header
        as input files and produces <opfilename> as a final output file.

>2. Sample 3 works with text files. I wonder if it would work with binary files
>also. Could you find out. Could you try it?

Yes it works fine.

>3. You added a header, sample3.h, but only sample3.c uses those functions. Is
>it really necessary?

Removed the header sample3.h

Please review and let me know your comments.

Thanks,
Shailendra

Comment 27

[Elio Maldonado]

r+, but please add two small corrections. The encrypted file name suffix is actually .enc. On encrypt both the encrypted and the header files should be considered output. The note should actually say
Note :  For encrypt, it takes <ipfilename> as an input file and produces
        <ipfilename>.enc and <ipfilename>.header as output files.

        For decrypt, it takes <ipfilename>.inc and <ipfilename>.header
        as input files and produces <opfilename> as a final output file.

Nitpick: In the other two print lines you have {optional]. Make the closing bracket style match the opening one, either {optional} or [optional]

Comment 28

[Elio Maldonado]

Comment on attachment 437283 [details]
Sample3 Version 3

r+, with the suggestions noted earlier.

Comment 29

[Shailen]

Attachment: Sample3 Version 4

Hi Elio,

  With this patch, I have taken care of the suggestions you made.

Thanks,
Shailendra

Comment 30

[Shailen]

Attachment: Sample4 Version 1

Hi Elio,

   Please find attached the tar file containing sample4 and let me know your review comments.

Thanks and regards,
Shailendra

Comment 31

[Elio Maldonado]

Comment on attachment 438073 [details]
Sample4 Version 1

Generally good but it could se a bit of work regarding generility on some functions and more obvious naming of some high level functions. Details and a patch next.

Comment 32

[Elio Maldonado]

I suggest renaming some of the constants and variables to be more generic. For example, wrappedEncKeyItem is preferable to wrappedAesKeyItem given that the code is quite generic. The CryptInit has a CK_MECHANISM_TYPE type parameter. Please use it instead of CKM_AES_CBC, something Bob caught this one while reviewing sample3.  For high level functions, on teh other hand, I suggest being more specific. PeformStep{A|B|C} doesn't make it obvious to our readers what's intended. Attaching a patch with my suggestions.

Comment 33

[Elio Maldonado]

Attachment: Suggested changes for sample 4 in patch form.

Comment 34

[Shailen]

Attachment: sample4 version 2

Hi Elio,

   Thanks for reviewing sample4 and I have applied the patch containing your suggestions with this attachment.

Thanks as always,
Shailen.

Comment 35

[Robert Relyea]

Comment on attachment 437494 [details]
Sample3 Version 4

r- for mostly nits. The general code and NSS usage itself is ok.

there are a few bugs in corner cases (like padding).

Here are my comments:


Makefile..

Probably should set the -I and -l flags based on pkgconfig if it exists and user settable environment variables if it does't.


sample3.c..

WriteToHeaderFile:
   change:
        char header[40];
        char trailer[40]; 
   to:
        const char *header;
        const char *trailer;

   change each:
        strcpy(header, *_HEADER);
	strcpy(trailer, *_TRAILER);
   to:
        header= *_HEADER;
        trailer=*_TRAILER;

(example: change strcpy(header, AESKEY_HEADER); to header=AESKEY_HEADER);

You don't need to modify the string, so there is no reason to give each of these strings their own buffer. This will be less error prone if you add strings longer than 40 bytes.

  in the switch:
        add a default case the fails .

CryptInit:
    1. You pass in the mechanism type, but it's still hardcoded to CKM_AES_CBC internally.
   

ReadFromHeaderFile:
   Same comment as WriteToHeaderFile.
   FileToItem is not returning a NULL terminated string, so strstr could go off into the weeds (or even crash!).
   You don't check to see if the header didn't exist? Since you use don't check the return code from the HexToBuf function, you don't know if the header is missing or not.   

EncryptAndMac:
  You don't check the return code from MacUpdate.
  Small bug. If inFile has exactly BLOCKSIZE bytes, then the final padding will be wrong (which should be a block full of bytes labeld BLOCKSIZE). You also write the wrong padding length to the pad file in this case (paddingLenght will be uninitialized.

DecryptAndVerifyMac:
   Takes file names, not already open file descriptors like EncryptAndMac does. Is there a reason?
   Why are you saving the last block of cipher text in the iv? the context takes care of that.



Util.c

Loose the password length check. The appropriate length and strength depends on the token used.

Comment 36

[Elio Maldonado]

Comment on attachment 446155 [details]
sample4 version 2

Bob's review comments for sample3 are applicable to sample4. Conversely, some of the naming changes you did here apply also to sample3.

Comment 37

[David Woodhouse]

Hm, these examples don't seem to touch on how to decide which dbdir should be opened by well-behaved applications. As far as I know, this is the current best practice... although it sucks for apps to have to do this for themselves.

Perhaps NSS_InitReadWrite(NULL) should do this automatically?

char *find_dbdir(void)
{
	int found = FALSE;
	FILE *f;
	char buf[80];
	char *tmp;

	f = fopen(NSS_SYSTEM_DB "/pkcs11.txt", "r");
	if (f) {
		/* Check whether the system NSS db is actually enabled */
		while (fgets(buf, 80, f) && !found) {
			if (!strcmp(buf, "library=libnsssysinit.so\n"))
				found = TRUE;
		}
		fclose(f);
	}
	if (found)
		return strdup("sql:/etc/pki/nssdb");

	tmp = getenv("HOME");
	if (!tmp || asprintf(&tmp, "sql:%s/.pki/nssdb", tmp) == -1)
		return NULL;
	return tmp;

}

Comment 38

[Elio Maldonado]

Attachment: NSS_InitReadWrite(NULL) automatically selects database to open

This is inspired on David's code in comment #37. Not for review as it doesn't properly belong here. I'm playing with this as part of another task.

Comment 39

[David Woodhouse]

 SECStatus
 NSS_InitReadWrite(const char *configdir)
 {
+	if (!configdir)
+		configdir = find_dbdir();
+

That bit wants to go in nss_Init(), doesn't it? With some care to ensure we're doing the right thing for NSS_Init_NoDB(), but I think the noCertDB flag covers that.

Comment 40

[Elio Maldonado]

(In reply to comment #39)

> That bit wants to go in nss_Init(), doesn't it? 

It does.  Let's look cloesely.
NSSS_InitReadWrite(const char *configdir) is 
return nss_Init(configdir, "", "", SECMOD_DB, "", "", "", "", "", NULL,
		NULL, PR_FALSE, PR_FALSE, PR_FALSE, PR_FALSE, PR_FALSE, 
		PR_TRUE, PR_FALSE, PR_FALSE, PR_FALSE);
 
A breakdown of the parameters shows:
 1 configdir     = find_dbdir() when we pass NUL, 
 2 certPrefix    = "", 
 3 keyPrefix     = "",
 4 secmodName    = SECMOD_DB, 
 5 updateDir     = "", 
 6 updCertPrefix = "", 
 7 updKeyPrefix  = "",
 8 updateID      = "", 
 9 updateName    = "",
10 initContextPtr= NULL,
11 initParams    = NULL,

12 readOnly  = PR_FALSE, <- as expected, we want write access
13 noCertDB  = PR_FALSE, <- as expected, we do want the certDB

14 noModDB       = PR_FALSE, 
15 forceOpen     = PR_FALSE, 
16 noRootInit    = PR_FALSE,
17 optimizeSpace = PR_TRUE, 
18 noSingleThreadedModules = PR_FALSE,
19 allowAlreadyInitializedModules = PR_FALSE,
20 dontFinalizeModules = PR_FALSE

Looks okay to me.

> ............................................... With some care to ensure we're
>doing the right thing for NSS_Init_NoDB(), but I think the noCertDB flag covers
>that.

I'm glad to ask, noCertDB indeed takes care of this one. Looking at the implementation of NSS_NoDB_Init(const char * configdir) one finds
return nss_Init("","","","", "", "", "", "", "", NULL, NULL,
		PR_TRUE,PR_TRUE,PR_TRUE,PR_TRUE,PR_TRUE,PR_TRUE,
		PR_FALSE,PR_FALSE,PR_FALSE);

parameter 12 readOnly=PR_TRUE <- cert/key/{semdmod) db won't be modified
parameter 13 noCertDB=PR_TRUE <- and don't even bother to open thm as only
                                 in-memory db (tables) will be accessed

Comment 41

[Elio Maldonado]

Comment on attachment 454123 [details] [diff] [review]
NSS_InitReadWrite(NULL) automatically selects database to open

Not a good idea to do it in NSS. There a better propsals.

Comment 42

[Elio Maldonado]

Reviewing patches in the form of tar achives is very cumbersome. Shailendra and I collaborate on them on an on-and-off basis when there is time left after other committments. To facilitate collaboration, ease the review chores, and attract collaborators we have an nss samples git repo. Get them via: git clone git://fedorapeople.org/~emaldonado/samples.git

Comment 43

[Elio Maldonado]

Comment on attachment 431577 [details]
sample2 version2

r+ from me with some suggestions. REQUIRE_ARG(opt,value) macro is no longer needed. As part of another bug I fixed the the argument parsing code in the secutil library to watch for sutuations like that. Subsequent submissions for review should be made as patches to the code in our temporary git repository.

Comment 44

[Shailen]

All the samples (sample1 to sample6) are ready for review and you can get them via : git clone git://fedorapeople.org/~emaldonado/samples.git

Please review and provide your review comments.

Note:

One small effort related to sample6 on sign/verify functionality is pending which I am discussing with Elio. I will update once I have more information.

Comment 45

[Elio Maldonado]

In order to get a review Bugzilla requires an attachment. I'll attach the files for sample 5 and sample 6 for Shailendra. Will request review from Bob on 5 and leave 6 waiting until Shalilendra gets feedback on 5. samples 3 though 6 are related, each one is a variant or refinement of the prior one. Review volunteers are welcome.

Comment 46

[Elio Maldonado]

Attachment: sample5 - variant of sample 4 that uses certificates

Comment 47

[Elio Maldonado]

Attachment: sample5 - utilities source

Comment 48

[Elio Maldonado]

Attachment: sample5 - utilities header

Comment 49

[Elio Maldonado]

Attachment: sample5 - makefile

Comment 50

[Elio Maldonado]

Attachment: sample6 - makefile

Comment 51

[Elio Maldonado]

Attachment: sample6 - refinement of 5

Comment 52

[Elio Maldonado]

Attachment: sample6 - untilites source

Comment 53

[Elio Maldonado]

Attachment: sample5 - utilities header

Comment 54

[Robert Relyea]

Comment on attachment 640283 [details]
sample5 - variant of sample 4 that uses certificates


Some bugs and some style issues in the following code in main:


   if (PL_strlen(command)==0)
    	Usage(progName);
    cmd = command[0] == 'a' ? GEN_CSR : command[0] == 'b' ? ENCRYPT : command[0] == 'c' ? DECRYPT : UNKNOWN;

    if (!command || !dbdir) {
        Usage(progName);
    }

    if (command[0] == 'a') {
        if (!certReqFileName) {
            Usage(progName);
        }
    }
    if (command[0] == 'b') {
        if (!certReqFileName && !inFileName) {
            Usage(progName);
        }
    }
    if (command[0] == 'c') {
        if (!inFileName && !outFileName) {
            Usage(progName);
        }
    }


First, the 'if (!command || !dbdir)' should go before the 'PL_strlen' and the 'command[0] ==' statements. (you want to check for NULL before you use the function. (bug)

Second, The whole cmd = command[0] == 'a' ? .... along with the 'if (command[0] ==a )' code all cry out to be come a big switch statement:

switch (command[0]) {
case 'a' : ....
case 'b' : ....
default:
}
--------------------------------------------------------------------------

- You should check for subject name to be set.
-Just looks the phone number, you do nothing but print it out (you don't include it in the cert request.).

- In CertReq: No need to check if the file exists and delete it, PR_Open with PR_CREATE_FILE and PR_TRUNCATE will effectively erase the previous file anyway. (NOTE Same with main's deleting encryptedFileName).
- In CertReq you free the private key and public key. It is better style to free it in the function you generated it in (CreateCertificateRequest). Same comment for freeing slot in CreateCertificateRequest, EncryptFile, DecryptFile
- In CertReq you leak: spki, cr, arena (which encoding and result are allocated from), and obuf

--- in ExtractPublicKeyFromCertRequest, ReadDERFromFile appears to be called but not defined.
--- In ExtractPublicKeyFromCertRequest: reqDER is leaked in most error paths. Also on success arena is leaked. It's safe to free the arena on success, SECKEY_ExtractPublicKey will give you your own copy of a new publicKey with the relevant fields copied.
--- In ExtractPublicKeyFromCert: If your CERT_VerifSignedData fails, you will still return a publicKey, even though you print "bad certificate request". On the other hand, if SECKEY_ExtractPublicKey returns NULL, you will not print the error message. Better to use publicKey==NULL as your error check. Be sure to bounce to cleanup if (rv != SECSuccess) after CERT_VerfifySignedData.
NIT (all your if (rv) calls should be if (rv != SECSuccess). the latter is more clear and doesn't depend of the fact that SECSuccess happens to == 0).

-- In GenerateSYMKey and throughout. There are lots of calls to PK11_Authenticate if (PK11_NeedLogin() is set. You really only need one at the beginning, and even then it shouldn't be strictly necessary.

First:  PK11_Authenticate() will automatically check PK11_NeedLogin and return SECSuccess if PK11NeedLogin is false.
Second: All the low level PK11_ functions automatically check PK11_NeedLogin if you need to login to provide services. That is why they all take pwdata, so they can prompt for passwords on the fly.

The correct use of PK11_Authenticate in an application is calling it at the beginning of an application to authenticate to tokens while the application still has access to the user. 

--- SeedFromNoiseFile and GenerateRandom arm missing. Why GenerateRandom instead of PK11_GenerateRandom?
--- Just a note, the removal of headerFile in main when making a cert request should move the Encrypt (certrequest doesn't write into the header file). The reason it's necessary and the other removes aren't is because the header file is opened PR_Append not PR_TRUNCATE.


--- WritetoHeaderFile and ReadFromHeaderFile appear to use PEM format, but actually write hex rather than base64 (AtoB/BtoA).


I have not reviewed the Decrypt side... this should get things started, however..

Comment 55

[Robert Relyea]

>- You should check for subject name to be set.

This comment is about main, and the subjectname should be checked if you are trying to create a cert request. Otherwise you are likely to crash, (or create a NULL subject cert request).

> -Just looks the phone number, you do nothing but print it out (you don't include it in the cert request.).

This comment is related to main , CreateCertificateRequest, and CertReq.

Comment 56

[Kai Engert (:KaiE:)]

If you eventually publish the new samples, please consider to update
https://developer.mozilla.org/en-US/docs/Getting_Started_With_NSS#NSS_Sample_Code
to include a link to it. Thanks.

Comment 57

[Elio Maldonado]

(In reply to Kai Engert (:kaie) from comment #56)
Thanks for the reminder, that is the plan. In the meantime, I have updated the page with a link to this bug and with instructions on how to download the samples. I do mention that they are still a work in progress and under review.

Comment 58

[Manohar]

Hi All,
I would like to contribute from my end. I have written a program to satisfy this case
1. A program like MD5hash, reads file on stdin, outputs hex hash to stdout.

Please have a look at below output(MD5 on test file),if it satisfies, I will share code.


prompt # ./md5_evp test
MD5(test)= 33bc1afb8e975dbca2ec4f491940062a

However, if it is assumed that openssl is installed in the machine, then a ready command can be used as shown below.

prompt# openssl md5 test
MD5(test)= 33bc1afb8e975dbca2ec4f491940062a
prompt#

Thanks
Manohar

Comment 59

[Elio Maldonado]

Updated in https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Getting_started_with_NSS the instructions of how to download the samples as they have moved to a mercurial branch. To download them execute: hg clone https://hg.mozilla.org/projects/nss; cd nss; hg update SAMPLES_BRANCH

Comment 60

[Elio Maldonado]

Preview of the new samples at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/New_NSS_Samples

Comment 61

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.