463848 - nss samples could use some some cleanup

Status: RESOLVED WONTFIX

(NSS :: Documentation, defect)

Description

[Elio Maldonado]

Attachment: patch to sample code html pages

The nss sample applications as they stand don't compile out of the box. They also don't follow the nss coding style and guidelines, e.g. use NSPR rather than the standard libraries, clean-up after themselves on exit. Attached is an attempt to clean them up a bit.

Comment 1

[Elio Maldonado]

Attachment: archive with actual sources and simple makefile to build them

Comment 2

[Elio Maldonado]

Attachment: cleaner code and html fixes

Comment 3

[Elio Maldonado]

Attachment: html pages with the patch applied, sources and makefile

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 347296 [details] [diff] [review]
cleaner code and html fixes

Among the problems with the old samples that were listed in comment 0 are:
> They also don't follow the nss coding style and guidelines
I infer from that that a goal of this patch is to follow NSS coding style.

The new samples are C++, not c, and use an indentation quantum of 2, not 4.
They use at least 3 different styles with respect to braces, e.g.
  if (expression) { statement; }
and
  if (expression)
  {
    statement;
  }
and
  if (expression) {
    statement;
  }

So, I would say that they do not meet the goal (if indeed it was a goal) 
of following the NSS coding style.  (Or perhaps this is meant to reflect
NSS's own lack of consistency in coding style? :)

So, either this patch gets an r- for not achieving its own criteria, or 
else the goals for this patch need to be restated in a way that is not 
inconsistent with the new samples.  I'll leave it up to my esteemed 
RedHat colleagues to decide which.  You tell me against what criteria 
this patch should be reviewed, and I will review it that way.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 347298 [details]
html pages with the patch applied, sources and makefile

I was unable to use this attachment, but I was able to use the other one, and apply it to my copy of the www.mozilla.org tree.

Comment 6

[Elio Maldonado]

Attachment: samples cleanup second iteration

Using 4 stops indentation throughout, using do { .. } while (0); to replace  numerous unsightly goto's with breaks. Simplified the flow logic where I could, better names here and there, using NSPR calls instead standard library calls where I could. The purpose is improved readability, resemblance to nss utilities where it matters, and encouragement of the use of nspr. Not aiming for 100% adherence to the mozilla coding standard by any stretch of the imagination. Kept sample1 a c++ application as I found it, it does deviate somewhat from the style of the other samples. sample 3 should had been a c application as there is hardly anything c++-like about it expcept the printing of the key.

Comment 7

[Elio Maldonado]

These relate to sample3.
1. Missing the log implementation. Add it near the top as
static void log(const char *msg) { std::cout << msg << std::endl; }
2. Uses unnecessary "\n" in the log invocation string arguments. e.g.
log("something failed\n")  --->  log("something failed");
3. mixing C and C++ ways of output which is bad style.
Having a dumpBufferAxHex(unsigned char *buffer, unsigned int len) function would be nice. We could then get rid of "cout << (void)key << endl;" and such and make it a plain C application.

Comment 8

[Elio Maldonado]

Actually, the way to fix sample3 is as follows:
remove the #include <iostream> -- it won't be needed
implement the convenience logging function as
static void log(const char *msg) { printf("%s\n", msg); }
fix the length printing line in printDigest to be printf("length: %d\n", len); 
don't bother to print the key in main as there is no value on doing this anyway
Now sample3 is a plain old C application.

Comment 9

[Elio Maldonado]

Attachment: samples cleanup third iteration

Summary of changes:
1. sample1 - rsaParams now an argument to Init per original author's suggestion
2. sample1 - further cleanup of control logic for readability
3. sample1 - added a simple Log function to make conversion to C easier
4. sample2 - fixed duplicate context destruction bug
5. sample3 - fully converted sample to a C application
6. samples 4,5,6 - improved comments and reordered functions
Still to do: Find out the answer to and remove the comment: "Does this need to be "WithFlags"??" in the Unwrap the MAC key code block in sample1

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I think we need to completely redo the NSS sample code from scratch,
starting with design, and worry about coding style later.  Here's why:

When I first got a review request for this bug, I wasn't sure what I
was supposed to look for.  So I read comment 0 which said that the
existing sample code has certain problems that should be fixed.  The
problems cited were:

1. Doesn't compile
2. Doesn't follow NSS coding style guidelines
3. Doesn't use NSPR
4. Doesn't clean up (e.g. free allocated resources) before exit.

So, when I read the first patch, I judged it strictly on those issues.

But when I received the second version of the patch for review, I stepped 
back and read the intro sample code page, and read the examples over to try to understand what they are trying to demonstrate.  The intro page says:

"The collection of sample code here demonstrates how NSS can be used for
cryptographic operations, certificate handling, SSL, etc. It also
demonstrates some best practices in the application of cryptography."

After I read the examples, I concluded that these examples are HORRIBLE in
their design.  They are examples of how NOT to do cryptography.  They do
the very things that we routinely advise programmers NOT to do, because
they're bad security practices.  People who try to follow these examples
will  definitely start in the wrong direction.  The very first example is
hopelessly complicated and very poor crypto design.  Getting the designs
right is much more important than getting the coding style right.

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.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 348700 [details] [diff] [review]
samples cleanup second iteration

I am cancelling this review request, rather than giving it either r+ or r-.
In previous comments in this bug, I have commented that I believe that all
of the NSS sample programs need to be replaced entirely with new programs
that are much better designed.  I believe that strongly enough that I don't
think it's worthwhile to try to "fix up" the bad old sample programs.

Having said that, if you believe that one of the patches attached to this 
bug is a real improvement, then you could go ahead and check it in.  
But I recommend that little or no more time be spent trying to make 
further improvements on these bad old sample programs, and that if more 
effort is to be spent, it should be spent on writing better designed 
sample programs.

Comment 13

[Elio Maldonado]

I agree, there is no point on cleaning up this code. Instead I have entered Bug 490238 to create new NSS samples and I am closing this one.