77983 - Add support for keygen tag.

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[David P. Drinan]

Add support for the <KEYGEN> tag in PSM 2.0.

Comment 1

[David P. Drinan]

Attachment: Javi, please review my patch.

Comment 2

[Javier Delgadillo]

2 comments:

1) Use NS_IMPL_THREADSAFE_ISUPPORTS to implement the base methods instead of
IMPL_ADDREF
2) When possible, avoiding using PORT_* functions (specifically for strcmp)

Comment 3

[David P. Drinan]

Attachment: Second revision.

Comment 4

[David P. Drinan]

Attachment: Third revision. This imports user cert and fixes bug in certdownloader when default buffer size is too small.

Comment 5

[David P. Drinan]

Adding thayes to cc-list.

Javi or Terry, could one of you review the lastest patch.

Thanks.

Comment 6

[Javier Delgadillo]

A couple of notes:

1) We may want to consider using SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION instead 
of SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION.
2) -u diffs are easier to read.  Please use it instead next time.

None of these are stop ships for this check-in, so r=javi on the latest patch.

Comment 7

[David P. Drinan]

Adding [e-mail] to cc list.

Scott,

Please super-review the latest patch.

Thanks.

Comment 8

[Scott Collins]

Diff:
  should be unified; please use unified from now on

Licenses:
  choosetoken.js -- wrong date
  chosetoken.xul -- missing 3rd and 4th paragraph of license
  nsKeygenHandler.cpp, .h -- wrong date, wrong license, should be MPL

  the MPL is the license we use for all new (even Netscape) files
    the specific rules and exceptions are at the top of
     http://www.mozilla.org/MPL/boilerplate-1.1/
  always update the date to the current year for new files
  use the templates at the above URL

Source conventions:
  use spaces not tabs

Naming conventions:
  argument names start with `a', e.g., |aCtx|, |aTokenList|, |aCount|,
  |aTokenChosen|, as opposed to, e.g., (sic) |tokenChoosen|

Coding conventions:
  prefer |0| or else *cough* |nsnull|.  Don't use |NULL|.
  prefer, e.g., |if (!arena)| over |if (arena == NULL)|
  _never_ use old-style casts, e.g., prefer
    |params = NS_STATIC_CAST(PQGParams*, PORT_ArenaZAlloc(...))|
  declare variables where they are needed and can be immediately initialized,
     not at the top of the function
  avoid |goto|

  for example, consider this re-write of |decode_pqg_params| (excuse poor line
  breaks please)

    static
    PQGParams*
    decode_pqg_params( char* aStr )
      {
        PQGParams* params = 0;

        PRArenaPool* arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
        if ( arena && (params = NS_STATIC_CAST(PQGParams*,
PORT_ArenaZAlloc(arena, sizeof(PQGParams)))) )
          {
            params->arena = arena;

            unsigned int len = 0;
            char* buf = NS_STATIC_CAST(char*, ATOB_AsciiToData(aStr, &len));

            if ( !buf || !len || SEC_ASN1Decode(arena, params,
SECKEY_PQGParamsTemplate, buf, len) != SECSuccess )
              params = 0;
          }

        if ( arena && !params )
          PORT_FreeArena(arena, PR_FALSE);

        return params;
      }


  When stabilizing refcounts around a query for a new object, consider an
  |nsCOMPtr|, e.g.

    nsCOMPtr<nsISupports> stabilize = formProc;
    rv = formProc->Init();
    if ( NS_SUCCEEDED(rv) )
      rv = formProc->QueryInterface(aIID, aResult);
    return rv;

  In fact, if the type |nsKeygenFormProcessor| meets the right criteria, you
  could directly make an |nsCOMPtr| at the point of the |new|, see
    news://news.mozilla.org/[e-mail]

  prefer pre-increment over post-increment except where idiom rules
  avoid |do|...|while| loops where possible, they are traps waiting to catch
    the unwary
  when you have a lot of resource allocation to clean up,
    consider using simple C++ guard classes to automatically handle it

  where a routine accepts a |const nsAReadableString&| or |const nsAString&|
    you can say, e.g.,

      res = selectElement->GetAttribute(NS_LITERAL_STRING("keytype"), keyTypeValue);

    and in this case, you would probably want to declare |keyTypeValue| as a
|nsAutoString|

  prefer
    |Compare(aStr, bStr, nsCaseInsensitiveStringComparator()) == 0| over
    |aStr.EqualsIgnoreCase(bStr)|

  this allows you to pass a more abstract type for your parameters, e.g.,

    NS_METHOD
    nsKeygenFormProcessor::ProvideContent( const nsAString& aFormType,
                                               nsVoidArray& aContent,
                                                 nsAString& aAttribute )
      {
        if ( Compare(aFormType, NS_LITERAL_STRING("SELECT"),
nsCaseInsensitiveStringComparator()) == 0 )
          {
            for ( SECKeySizeChoiceInfo* choice = SECKeySizeChoiceList; choice &&
choice->name; ++choice )
              {
                nsString *str = new nsString(choice->name);
                aContent.AppendElement(str);
              }
            aAttribute = NS_ConvertASCIItoUCS2(mozKeyGen);
          }

        return NS_OK;
      }

  The above example fixes the parameters to be more permissive,
    puts the declarations and initializations at the right place,
    converts the loop to the idiomatic form,
    exploits pre-increment,
    and makes better use of strings.

  you mustn't |#include| files inside an |extern "C"| declaration,
  either they are written to have C calling conventions or they aren't
  if you wrap their declarations in this way, on platforms where the calling
    conventions differ there will be link problems
  if they really are C functions, then their header should be written like this

  #ifdef __cplusplus
  extern "C" {
  #endif

  // stuff...

  #ifdef __cplusplus
  }
  #endif

  for example, see
http://lxr.mozilla.org/seamonkey/source/directory/c-sdk/ldap/include/lber.h#26


  and the comment

+ //For some weird reason, nsProxiedService has to be the first file
+ //included.  Don't ask me, I'm just the messenger.

  doesn't make any sense given that "nsProxiedService.h" is the 9th file
  included

I think this patch is going to require some effort before it's ready,
particularly in the area of getting rid of |goto|s and appropriate resource
protection in the face of more robust control flow mechanisms.  You are probably
going to want a second opinion on this.  I recommend brendan, shaver, or blizzard.

Comment 9

[Scott Collins]

In discussion with blizzard, his position is that, at least for
|decode_pqg_params|, he finds your version more readable than mine.  So there is
a reasonable point of view that your use of |goto|s may be acceptable.  You are,
after all, using them in the standard C pattern.

Comment 10

[Scott Collins]

There are definitely issues to consider in this code.  I don't want you to be
discouraged by my review.  I don't want to keep this code out of the tree from
spite or anything like that.  I think there are things that can be improved, and
more input from another super-reviewer could be very beneficial.  I don't
necessarily think that everything I mentioned needs to be fixed all at once
before this code can go in.

Comment 11

[David P. Drinan]

Yes, I think our use of goto's is useful because we are calling alot of NSS 
fuctions that have C interfaces and we need a clean and efficient way to do 
clean up. We could write a C++ layer that sits on top of the NSS interfaces that 
would do the correct clean up, but this would be quite an effort and could not 
be done by the 0.9.1 freeze.

I will post a new patch shortly incorporating your other comments.

Comment 12

[David P. Drinan]

Attachment: 4th revision of patch.

Comment 13

[David P. Drinan]

This patch addresses most of Scotts comments *except*:

Use of goto's:
This is a much larger issue and we should consider for a future release. What's 
needed are wrapper classes around NSS that take care of the allocation and 
freeing of resources.

|#include| files inside an |extern "C"|
This is a bug in NSS. I'll open a bug against NSS and when that bug is fixed and 
PSM moves to the fixed version, I'll fix this in PSM.

Comment 14

[David P. Drinan]

Adding [e-mail] to cc-list.

Scott,

I've updated the patch. Please read the previous comments.

Thanks,
David.

Comment 15

[Scott Collins]

To give your code the fairest possible shake, I've asked blizzard to be the
super-reviewer in this case.  I know this patch is important, and I don't want
my particular language demons to unfairly prevent you from getting this stuff
in, in case my viewpoint is over the line.  Hope this helps :-)

Comment 16

[Christopher Blizzard (:blizzard)]

I see an allocator mismatch - a few even.

+  username = CERT_GetCommonName(&cert->subject);
+  if ( username == NULL ) 
+    username = strdup("");
...
+  if ( username )  {
+    PR_Free(username);
+  }

I suspect that it won't hurt anything but I can't be sure.

I'm kind of bothered by the use of c-named-like functions in a .cpp file when
you could make them static member functions but we can let it slide for now.

+  //Do a check to see if we need to allocate more memory.
+  if ((mBufferOffset + (PRInt32)aLength) > mContentLength) {
+      size_t newSize = mContentLength + kDefaultCertAllocLength;
+      char *newBuffer;
+      newBuffer = (char*)nsMemory::Realloc(mByteData, newSize);
+      if (newBuffer == nsnull) {
+        return NS_ERROR_OUT_OF_MEMORY;
+      }
+      mByteData = newBuffer;
+      mContentLength = newSize;
+  }

is this something that we could move to another data structure without so much
pointer magic?  We have at least nsStrings which can be used as poor man's
buffers.  If not then don't worry about it.  It may be data from some other source.

I'm not particularly concerned with the goto: style deallocation - in fact I've
used it from time to time when it's appropriate.  And here it seems to be since
you've got a lot of C-style allocations.

Assuming that you open a bug against the extern "C" and fix the mismatched
allocators you should be all set to go with an sr=blizzard

Thanks!

Comment 17

[David P. Drinan]

Fix checked in.

Comment 18

[John Unruh]

Will verify when bug 81443 is fixed. I cannot get any certs from a Netscape 4.1 
CMS right now.

Comment 19

[John Unruh]

Verified.