Closed Bug 402712 Opened 17 years ago Closed 7 years ago

Add PKCS#11 PEM Reader

Categories

(NSS :: Libraries, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: rcritten, Assigned: elio.maldonado.batiz)

References

Details

Attachments

(6 files, 13 obsolete files)

127 bytes, text/plain
Details
134.44 KB, patch
Details | Diff | Splinter Review
213.83 KB, patch
rrelyea
: review-
Details | Diff | Splinter Review
59.46 KB, patch
Details | Diff | Splinter Review
26.43 KB, patch
Details | Diff | Splinter Review
62.14 KB, patch
Details | Diff | Splinter Review
Attached file The PKCS#11 module (obsolete) —
Red Hat requires a PKCS#11 module that can read PEM files in the OS NSS package it ships.

Code is attached.

Please let's get this done prior to the NSS 3.12 release.
This is really an enhancement.

Marking it as such.
Severity: blocker → enhancement
Rob, can you attack the original as a diff.

You can do this by doing CVS add in your tree, then doing a cvs diff -Nup on the directory.


Also, the patch to build the module should probably be limitted to UNIX platforms for now. (Look at ckfw/Makefile to see how capi and mkey were added).

bob
NSS has needed a PKCS#11 module that can read PKCS#12 files for a long time.
Unless we want NSS to shift its focus away from PKCS#12 and towards PEM,
we should not give preference to support of other packages' file formats 
over support of our own file formats.

Maybe this should be a Red Hat feature, and not an NSS feature?
(In reply to comment #4)
> NSS has needed a PKCS#11 module that can read PKCS#12 files for a long time.

I guess nobody needed that urgent enough to step up and do it?
But we urgently needed the PEM reader.


> Unless we want NSS to shift its focus away from PKCS#12 and towards PEM,
> we should not give preference to support of other packages' file formats 
> over support of our own file formats.

We are trying to get our job done.
I don't think contributing a new feature is equivalent to evangelizing into a certain direction?
We required it, we did it, we're donating it as a contribution, here it is.


> Maybe this should be a Red Hat feature, and not an NSS feature?

Are you saying the functionality to read a common cert file format does not make sense for a generic crypto library?
The PEM support is needed to support compatibility with old openssl apps (mostly as a bridge). It's akin to capi and mkey pkcs #11 modules and should only be compiled on those platforms that need it (currently Linux).
If you're trying to offer a migration path from PEM files to NSS, then 
there should be a one-time conversion program.  Offering a PKCS#11 module
that uses PEM files as its storage mechanism promotes PEM files to be a 
full peer of NSS's DBs and PKCS#12.  

The acceptance of major new contributions into NSS must be continent on the
NSS team's willingness to support them going forward.  I have no wish, and 
I think Sun has no wish, to support this PKCS#11 module.  

This was probably in development for a long time, yet no mention was ever made of it until it is "ready for checkin"?  What other surprises lie in store?
s/continent/contingent/
There is nothing nefarious going on. I simply didn't ask any questions about how to do this beyond Bob (who is on my speed-dial).

I'm not sure how else I would open a bug on this without completed code except perhaps as a place holder. If that is standard procedure you can chalk it up to a misunderstanding on my part.
Attached patch cvs -Nup diff (obsolete) — Splinter Review
Attachment #287539 - Attachment is obsolete: true
Attachment #287541 - Attachment is obsolete: true
Attachment #287590 - Flags: review?(rrelyea)
It would be convenient to include it in the NSS tree (and make it easier to to share changes etc should Sun want/need such a module in the future, or if sun wanted to enhance the module to support PKCS #12 files, for instance). 

If the rest of the NSS teams believes including such source code constitutes a commitment to support it in the future, we'll do something else.

bob

P2 not a beta blocker.
Priority: -- → P2
Version: 3.12 → trunk
Comment on attachment 335142 [details] [diff] [review]
small patch to fix memory allocation problem on 64-bit machines

> static const NSSItem pem_x509Item = {
>-    (void *) &ckc_x509, (PRUint32) sizeof(CKC_X_509)
>+    (void *) &ckc_x509, (PRUint32) sizeof(CK_ULONG)

Rob, the best way to avoid this kind of bug is to
use sizeof(ckc_x509).  To be consistent with the
rest of the file, you should continue to use
sizeof(type-name).  It's better to use
sizeof(CK_CERTIFICATE_TYPE) because ckc_x509 is
declared as a CK_CERTIFICATE_TYPE.  (I understand
CK_CERTIFICATE_TYPE is defined as CK_ULONG.)
Support for writing PEM files is also needed. The lack of this feature hampers the migration to NSS of several security products as part of the Fedora Crypto Consolidation effort.
Comment on attachment 366832 [details] [diff] [review]
patch to fix +/-1 problem in pem/pobject.c

who should review this?
Attachment #366832 - Flags: review?(rcritten)
Comment on attachment 366832 [details] [diff] [review]
patch to fix +/-1 problem in pem/pobject.c

r+ relyea for now.

Be sure Elio sees this, he's working on the PEM module now.

bob
Attachment #366832 - Flags: review?(rcritten) → review+
Support for pem file does raise some *interesting* question. Must each file be inserted or is there some location that is auto-discovered ? Is each file seen as a separate module ? If not, how do you handle the case where they don't use the same private key password ? All of this is not obvious and needs to be documented at a high level. Where is that doc ?

I haven't looked a lot at the patch, but it seems to me it should not get approval until open_log/close_log/plog have been replaced with the use of the standard NSPR log system.

Given Nelson's very calm and very moderate reaction, he hasn't yet seen that the code allows PEM private key that are not protected by a password. I'm starting the stopwatch now and waiting until I see him hit the roof :-)

Seriously, I'm wondering if it does not rise questions with regards to FIPS validation. OTOH OpenSSL did get validated whilst implementing it. But did you consider that aspect ? Do you have an answer about it ? (arguments that show doing that is compatible with FIPS, a way not to disable it in FIPS mode ?)
There is no auto-discovery. Each cert/key needs to be either loaded via the initialization string passed by secmod or programatically by creating objects on-the-fly. And yes, there is a dearth of documentation at this point, sorry about that.

Non-password protected keys are supported by NSS now, I don't know why it would bother Nelson. Doing away with this would defeat the purpose which is to make integration with an existing OpenSSL-based infrastructure possible.

I don't think this module will be FIPS validated, or at least that wasn't one of the driving factors in developing it. If someone wants FIPS certification they'd have to create an NSS database and import the certs and keys instead.
For servers that are locked in data centers with security
guards, it is fine to save private keys unencrypted on
disks.  If we force them to encrypt the private keys with
passwords, they will just save the passwords unencrypted
on disks.  It should be obvious that servers that need to
start up unattended must do this.

The storage of private keys on disks by software crypto
modules is outside the scope of the FIPS 140-2 section (4.7?)
on key management.  FIPS 140-2 is also relying on physical
security of the computer (at Level 1) or OS discretionary
access control (at Level 2) to protect the private keys
stored on disks.
(In reply to comment #19)

> Given Nelson's very calm and very moderate reaction, he hasn't yet seen that
> the code allows PEM private key that are not protected by a password. I'm
> starting the stopwatch now and waiting until I see him hit the roof :-)

LOL!  You're right that I haven't seen the code at all.  I just looked at
the patch, and immediately saw one BIG issue.  This code does NOT belong
under lib/ckfw.  It uses CKFW, but is not part of the CKFW.  It doesn't 
belong under the CKFW source any more than programs that use libc belong
under the libc source code in the Linux source tree.  (The same is really
true for nssckbi, but that mistake was made long ago.) I would give this 
patch an r- for that.

If this belongs in NSS at all (see my previous comments on that), it is a 
peer (in the source tree) of the softoken directory.  I think it should be
separately buildable.  That is, it must remain possible to pull and build
NSS without building this module (and ideally without pulling it).

> Seriously, I'm wondering if it does not rise questions with regards to FIPS
> validation. 

Well, this new PKCS#11 module is a peer of softoken, another PKCS#11 module
that can be loaded by NSS's PKS#11 module loader.  The FIPS 140 validation
only applies to softoken, so the presence of new code outside of softoken 
presents no problem to the validation of softoken.  But customers need to 
be clear that it's not part of Softoken's FIPS 140 solution.  

As for my "moderate reaction", :), it's clear to me that the goals of a PEM
module are not the same goals as we've historically had for the rest of NSS.
So, I'm OK with it as long as it is kept conceptually separate from the rest
of NSS, and is seen as a separate module that uses and/or works with NSS,
but is not part of NSS proper.  It should be a separately installed RPM,
for example, IMO.
RE: FIPS....

It's at least RH intention that PEM not be supported in FIPS mode. We will most likely make sure the PEM module would return errors when the OS itself is placed in FIPS mode to guarrentee this.
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
Kamil told me, that it is difficult to convert other tools to use NSS, because this package is missing in upstream. He says, it's important that the PEM reader is available as an upstream project either directlry in NSS, or maybe a separate upstream project.

Would it make sense to create a separate upstream project, only for the PEM reader module?
No, Elio needs to make this his #1 priority to get his stuff finished and get this in.
As an aside, OpenLDAP now has support for NSS but we have refrained from publishing that fact until this feature is commonly available. The majority of the world uses and understands how to use PEM files. We're not looking forward to releasing with NSS and then having to field all the questions about how to manage a certDB...
Attached patch PEM module rev1 (obsolete) — Splinter Review
Attached is the new version of the PEM module. Since Rob initially submitted it there has been quite bit attention given to the PEM module in Fedora with numerous patches contributed by Kamil Dudka and Rich Megginson. On Bob's request I have changed the module initialization parameters parser so that the PEM module can be loaded automatically by NSS and enable users to specify initialization parameters. This part is still under test. If desired I can submit this portion as a patch on its own for ease of review.
Assignee: rrelyea → emaldona
Attachment #287590 - Attachment is obsolete: true
Attachment #335142 - Attachment is obsolete: true
Attachment #366832 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #447110 - Flags: review?
Attachment #287590 - Flags: review?(rrelyea)
Attachment #447110 - Flags: review? → review?(rrelyea)
Comment on attachment 447110 [details] [diff] [review]
PEM module rev1


>+#define FREE_CLEAR(p) if (p) { PORT_Free(p); p = NULL; }

The condition is redundant here.  Please consider also the 'do { ... } while (0)' pattern for the compound statement within a macro definition.

>+    modparms = (unsigned char *) modArgs->LibraryParameters;
>+    plog("Initialized with %s\n", modparms);
>+
>+    if (pem_HasNewFormatParameters(modparms)) {

warning: pointer targets in passing argument 1 of ‘pem_HasNewFormatParameters’ differ in signedness

>+    rv = pem_ParseParameters(modparms, &numcerts, &certstrings);

warning: pointer targets in passing argument 1 of ‘pem_ParseParameters’ differ in signedness

>+void plog(const char *fmt, ...)
>+{
>+#ifdef DEBUG
>+    va_list ap;
>+
>+    va_start(ap, fmt);
>+    PR_LogPrint(fmt, ap);
>+    va_end(ap);
>+
>+    PR_LogFlush();
>+#endif
>+}

The above is _not_ going to work well.  You simply can't pass the variable arguments to PR_LogPrint() this way.  It would either crash or misbehave.

Please fix also the indentation madness in the newly added code.

I didn't look at the code parsing arguments since I am a bit out of context here.
Attachment #447110 - Attachment description: PEM module new submission → PEM module rev1
Attached patch PEM module rev2 (obsolete) — Splinter Review
Should address most suggestions in comment #29.
Attachment #447110 - Attachment is obsolete: true
Attachment #447380 - Flags: review?(kdudka)
Attachment #447110 - Flags: review?(rrelyea)
(In reply to comment #29)
> I didn't look at the code parsing arguments since I am a bit out of context
> here.

Let me provide the necessary context to understand the changes to code that parses the module initialization parameters. We want the PEM pkcs #11 module and its clients to work well will the nss-sysinit module. See its design at https://wiki.mozilla.org/NSS_Shared_DB_And_LINUX. There it says:
"The short summary: Except as noted below, all applications should initialize NSS with the following call: rv = NSS_InitReadWrite(“sql:/etc/pki/nssdb”);"

This advise holds true in particular for clients of the PEM module. With the new parsing code an application does not need to explicitly load the PEM module and handle initialization parameters. If nss-sysinit is present and activated in the system, the user (typically a system administrator) will be able to supply the initialization parameters via NSS's pkcs11.txt configuration file. The comments in pinst.c:pem_Initialize and pargs.c:pem_DoParseParameters gives further detail by enumerating some use cases. The parsing code handles the new format(s) and for backward compatibility continues to support the old format.
There is still an inconsistent use of spacing. My style is to use 4-space indentation, no tabs. There is still quite a bit of tab use, for example in pinst.c in the block starting PR_LogPrint("pem_Initialize: old format\n");

The new config code seems ok to me but I haven't tested it.

Elio, what are you testing this new pem code against? curl, openldap, both, more?
(In reply to comment #32) Need to clean up indentation some more. These are my vi settings.
[emaldona@localhost ~]$ cat .vimrc
set autoindent
set smartindent
set noexpandtab
set tabstop=8
set shiftwidth=4
set showmatch
set guioptions-=T
set ruler
set hls
set incsearch
set virtualedit=all

> 
> The new config code seems ok to me but I haven't tested it.
I'm still testing it and Bob will be doing a thorough review next. This code hasn't had the scrutiny, review-wise and testing-wise, that your patches already received in Fedora.

> Elio, what are you testing this new pem code against? curl, openldap, both,
> more?
I have been using a modified version of the crypto-utils tools for Apache. I need to spell out my test cases. I have adapted it so it looks like a regular nss tool. I'll attach it here. Mind you it is still a work in progress.

I was planning to use curl next, have some modifications in mind for libcurl so it loads the pem module only if nss hasn't do so already. I could use some advise on test cases.  I haven't looked at openldap yet. I will welcome any guidance or advise on both curl and openldap.
openldap initializes the PEM module like this:
	fullname = PR_GetLibraryName( NULL, "nsspem" );
	configstring = PR_smprintf( "library=%s name=PEM parameters=\"\"", fullname );
	pem_module = SECMOD_LoadUserModule( configstring, NULL, PR_FALSE );
Attached patch PEM module rev (obsolete) — Splinter Review
Integrate bug fix contributed by Kamil for a segfault with bad certificate https://bugzilla.redhat.com/show_bug.cgi?id=596674.
Attachment #447380 - Attachment is obsolete: true
Attachment #448091 - Flags: review?(rrelyea)
Attachment #447380 - Flags: review?(kdudka)
Attached patch WIP pem module test application (obsolete) — Splinter Review
Fedora's crypto-utils package has a tool to manage certificates for Apache which I enabled to use NSS. In NSS mode of operation it uses certutil as its back-end. In Openssl compatibility mode it has a back-end application that reads and creates PEM files and by calling the PEM module. I have adapted that application a sort of PEM module test tool. I stole code from certutil and made it do actions which are normally frowned-upon as you will notice. It's still in progress.
(In reply to comment #35)
> Created an attachment (id=448091) [details]
> PEM module rev

- Indentation of the newly added code is still _completely_ broken.  If you have some spaces followed by a tab, it's always an _error_, no matter which indentation style you use.  I suggest you to set your editor to highligting this flaw in sources.

- Why the whole util.c is re-indented this way for nothing?  The result does not conform neither the upstream NSS indentation style, nor Rob's downstream indentation style.

- What is the purpose of close_log()?  It isn't called from anywhere.

- The call of PR_NewLogModule() is omitted when DEBUG is 0.  However PR_LogPrint() is always called.  This can't be right.  What about using PR_LOG() for logging instead?

- If pem_ParseParameters() fails, pem_Initialize() does not seem to catch it soon enough and it will probably operate on invalid data.

- If I understand, pem_ParseParameters() reads parameters in the new format, while pem_ParseString() reads parameters in the old format.  What about giving them some more self-explaining identifiers? e.g. pem_ParseParametersNew/pem_ParseParametersOld?  They might also have the same prototype IMO.

I still didn't reach the pargs.c yet, will get back later to this one.
(In reply to comment #37)
> (In reply to comment #35)
> > Created an attachment (id=448091) [details] [details]
> > PEM module rev
> 
> - Indentation of the newly added code is still _completely_ broken.  If you
> have some spaces followed by a tab, it's always an _error_, no matter which
> indentation style you use.  I suggest you to set your editor to highligting
> this flaw in sources.

I did that in Eclipse and saw the spaces that I couldn't see on vi. The NSS coding style calls for tab stops at 8 and at the same time indentation by 4 which causes me headaches no matter how I configure the Eclipe plugin editor (based obn gedit) or switch to vi with which I haven't yet learned to use effectively. By the way, I see leading spaces all over the sources. I haven't figured out how to deal with this.
 
> - Why the whole util.c is re-indented this way for nothing?  The result does
> not conform neither the upstream NSS indentation style, nor Rob's downstream
> indentation style.
I'm going to keep Rob's indentation as it is and try to cleanup code only in the parts where I adding or modifying a lot of code.
> 
> - What is the purpose of close_log()?  It isn't called from anywhere.
This is a left-over from the old logging code. I'll drop it.
> 
> - The call of PR_NewLogModule() is omitted when DEBUG is 0.  However
> PR_LogPrint() is always called.  This can't be right.  What about using
> PR_LOG() for logging instead?

Yes, I'm switching from PR_LogPrint to PR_LOG which does the DEBUG check before calling PR_LogPrin. 
> 
> - If pem_ParseParameters() fails, pem_Initialize() does not seem to catch it
> soon enough and it will probably operate on invalid data.

Indeed and I am examining the parsing code and it troubles me. I could very well fail to detect parsing errors. We want to enable users to set parameters but the pkcs11.txt format for initialization parameters is hardly user-friendly. 

> 
> - If I understand, pem_ParseParameters() reads parameters in the new format,
> while pem_ParseString() reads parameters in the old format.  What about giving
> them some more self-explaining identifiers? e.g.
> pem_ParseParametersNew/pem_ParseParametersOld?  They might also have the same
> prototype IMO.
I'll rename them as you suggest.

> 
> I still didn't reach the pargs.c yet, will get back later to this one.
Take your time, pargs is not an easy one to write or to review. Aren't we glad we have you to share in the review burden with Bob?

I new version is coming next that hopefully will less of a pain to review. I still have to work on safer parsing.
Attached patch PEM pkcs11 module (obsolete) — Splinter Review
Attachment #448091 - Attachment is obsolete: true
Attachment #449139 - Flags: superreview?(rrelyea)
Attachment #449139 - Flags: review?(kdudka)
Attachment #448091 - Flags: review?(rrelyea)
(In reply to comment #38)
> I did that in Eclipse and saw the spaces that I couldn't see on vi.

It takes more to convince me ;-)

Rob's original submission:
$ curl -Ls 'https://bugzilla.mozilla.org/attachment.cgi?id=287590' \
    | grep -P ' \t' | wc -l
2

Your current submission:
$ curl -Ls 'https://bugzilla.mozilla.org/attachment.cgi?id=449139' \
    | grep -P ' \t' | wc -l
39

> I new version is coming next that hopefully will less of a pain to review. I
> still have to work on safer parsing.

I wonder how you tested the patch.  I am not able to compile it:

pargs.c: In function'pem_ParseParametersNew':
pargs.c:414: warning: 'tmp' may be used uninitialized in this function
pargs.c:414: note: 'tmp' was declared here
pfind.c:121:36: error: macro "PR_LOG" requires 3 arguments, but only 1 given
pfind.c: In function 'pem_mdFindObjects_Next':
pfind.c:121: error: 'PR_LOG' undeclared (first use in this function)
pfind.c:121: error: (Each undeclared identifier is reported only once
pfind.c:121: error: for each function it appears in.)
pfind.c:124:41: error: macro "PR_LOG" requires 3 arguments, but only 1 given

... and so on and so forth.

Please include also the following hunk:

https://bugzilla.redhat.com/show_bug.cgi?id=596674#c10
Attachment #449139 - Attachment is obsolete: true
Attachment #449139 - Flags: superreview?(rrelyea)
Attachment #449139 - Flags: review?(kdudka)
(In reply to comment #37)

> - Indentation of the newly added code is still _completely_ broken.  If you
> have some spaces followed by a tab, it's always an _error_, no matter which
> indentation style you use. 

Kamil, I appreciate the reviews you've been doing of these patches.  I fully
agree with your comments regarding the need for the code to compile, and 
regarding the correctness of the executable code.  But regarding the above
white space comment, the position you're taking there seems overly strict,
beyond the rules we commonly enforce for NSS style guidelines.  I would agree
that it is _preferred_ that lines begin with 0 or more tabs, followed by 
zero or more spaces, followed by the first non-whitespace character, and no tabs after the first whitespace character, but as long as the code "looks right" (appears to have an indentation quantum of 4) when tabs are set at 
every 8 columns, the NSS module owners won't typically reject a patch because it has tabs following spaces.  

Having said that, Elio, it's easy to fix code like that with the unix "pr" 
command.  If you take a passage of code with questionable use of spaces and
tabs, and pass it through a simple shell script that will make the leading
spaces and tabs conform to the guidelines Kamil wants, and those also would
be acceptable to NSS.  I'll attach a little shell script that should be quite suitable.

However, while running such a script is OK for files of brand new code, and 
for small blocks of code in files that you're modifying, I'd advise against 
it for large portions of existing files, because it's likely to lead to HUGE 
patches that change nothing but whitespace, which NSS module owners tend to 
dislike.
(In reply to comment #41)
> Kamil, I appreciate the reviews you've been doing of these patches.  I fully
> agree with your comments regarding the need for the code to compile, and 
> regarding the correctness of the executable code.  But regarding the above
> white space comment, the position you're taking there seems overly strict,
> beyond the rules we commonly enforce for NSS style guidelines.

The note about indentation errors really wasn't meant as something that should prevent the patch from being applied.  I just mentioned my observation here as I hadn't been sure if Elio is aware of it.  That's all.
Attached patch WIP pem module (obsolete) — Splinter Review
Sorry about the prior submission, his one compiles. I'm still running tests - regression as well as the 5 use cases. Wanted to show some progress. Cleaned up white-space very selectively and reverted to using the plog function to reduce changes relative to Rob's original submission. plog should be safer that what had earlier but it may go away eventually.
Attachment #449674 - Attachment is patch: true
Attachment #449674 - Attachment mime type: application/octet-stream → text/plain
Attachment #449674 - Flags: review?
> diff -N ./mozilla/security/nss/lib/ckfw/pem/pargs.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ ./mozilla/security/nss/lib/ckfw/pem/pargs.c	7 Jun 2010 15:26:32 -0000

...

> +	/* get the certificate/key file names */
> +	if (!secmod_argIsBlank(*filesIndex)) {
> +		char *args = secmod_argFetchValue(filesIndex,&next);
> +		filesIndex += next;
> +		if (!args)
> +			rv = CKR_ARGUMENTS_BAD;
> +		else {
> +			/* check that files are of the form
> +			 * {cer_pem_file} or {cert_pem_file;key_pem_file}
> +			 */
> +			status = pem_parseFilePairsParameters(args,&files[0]);
> +			rv = (status == PR_TRUE) ? CKR_OK : CKR_ARGUMENTS_BAD;
> +			PORT_Free(args);
> +		}
> +	}
> +	if (name)
> +		PORT_Free(name);
> +		
> +	filesIndex = secmod_argStrip(filesIndex);

The variable filesIndex is assigned, but then not anyhow used.  Is it correct?

> +static CK_RV
> +pem_parseTokenParameters(char *param, pemTokenParameters *parsed)
> +{
> +	int next;
> +	char *tmp;
> +	char *index;
> +
> +	index = secmod_argStrip(param);
> +
> +	while (*index) {
> +		SECMOD_HANDLE_STRING_ARG(index,tmp,"flags=",

Invalid free() at this point.  You want to initialize 'tmp' first.

> +static CK_RV
> +pem_DoParseParameters(const char *param, pemModuleParams *parsed,
> +		PRInt32 *numStrings, char ***returnedstrings)
> +{
> +	int next;
> +	char *tmp;
> +	char *index;
> +
> +	index = secmod_argStrip((char *) param);
> +	while (*index) {
> +		/* pem module tags */
> +		SECMOD_HANDLE_STRING_ARG(index,parsed->slotFiles,"slotFiles=",;)
> +		SECMOD_HANDLE_STRING_ARG(index,parsed->slotFlags,"slotFlags=",;)
> +
> +		/* nss like tags */
> +		SECMOD_HANDLE_STRING_ARG(index,tmp,"tokens=",

Again, invalid free() at this point.  You want to initialize 'tmp' first.

> +			if(tmp) { pem_parseTokens(tmp,parsed); PORT_Free(tmp); })
> +		SECMOD_HANDLE_FINAL_ARG(index)
> +	}
> +
> +	*numStrings = 0;
> +	if (parsed->slotFiles) {
> +		char **strings = NULL;
> +		int stringsCount = 0, i = 0;
> +		/* ignore return value, we know it is well-formed */
> +		(void) pem_ParseParametersOld(parsed->slotFiles, ' ', &stringsCount,
> +								&strings);
> +
> +		for (i = 0; i < stringsCount; i++) {
> +			addString(returnedstrings, strings[i], (*numStrings)++);
> +		}
> +	}
> +	if (parsed->tokens == NULL && parsed->tokenCount > 0) {
> +		int j;
> +		for (j=0; j < parsed->tokenCount; j++) {
> +			int n = parsed->tokens->filesCount;

SIGSEGV at this point as you know parsed->tokens == NULL ... perhaps the
condition above is wrong?  A comment somewhere around here would be also
helpful.
- The second parameter you pass to pem_parseFilePairsParameters() is not used at all, so that the dynamically allocated array 'parsed->files' of size 1 will always end up zeroed with no useful data inside.  Or am I missing anything?

- I don't know if NSS cares about ISO C90.  If so, the following warnings may be also fixed:

pargs.c: In function ‘pem_StrNdup’:
pargs.c:77: warning: ISO C90 forbids mixed declarations and code
pargs.c:81: warning: ISO C90 forbids mixed declarations and code
pargs.c: In function ‘pem_Strdup’:
pargs.c:96: warning: ISO C90 forbids mixed declarations and code
pargs.c: In function ‘pem_ParseParametersOld’:
pargs.c:142: warning: ISO C90 forbids mixed declarations and code
pargs.c: In function ‘pem_FreeParsedStrings’:
pargs.c:178: warning: ISO C90 forbids mixed declarations and code
(In reply to comment #45)
> > diff -N ./mozilla/security/nss/lib/ckfw/pem/pargs.c
> The variable filesIndex is assigned, but then not anyhow used.  Is it correct?
No, I'll remove this.
> 
> Invalid free() at this point.  You want to initialize 'tmp' first.
Yes, to both the one in parseTokenParameters and pem_DoParseParameters

......
> > +	if (parsed->tokens == NULL && parsed->tokenCount > 0) {
> > +		int j;
> > +		for (j=0; j < parsed->tokenCount; j++) {
> > +			int n = parsed->tokens->filesCount;
> 
> SIGSEGV at this point as you know parsed->tokens == NULL ... perhaps the
> condition above is wrong?  A comment somewhere around here would be also
> helpful.

Well, somewhere around here and somewhere around there and in many other places comments would be helpfull. My head spins as I revisit this. Time to re-examinine a lot of this code and clarify the design choices and clearly state the parsing requirements to myself and to you. Unstated assumptions are evil and there are quite a few.

(In reply to comment #46) Yes, in NSS we don't do C99. Mixed declarations and code will go away.
Attachment #448101 - Attachment is obsolete: true
Attached patch pem module (obsolete) — Splinter Review
Updated to the latest pem module sources in Fedora plus changes in initialization arguments as per Kamil's review. Among the problems this patch has is that enumeration of the certificates doesn't all of them. 
I tested with the following parameters: 
/etc/pki/tls/ca-bundle.crt /etc/pki/tls/certs/localhost.pem;/etc/pki/tls/private/localhost.key
It enumerated the ca-bundle but not the server.
Attachment #449674 - Attachment is obsolete: true
Attachment #449674 - Flags: review?
Attachment #497578 - Flags: review?(rrelyea)
Attachment #497571 - Flags: review?(rrelyea)
Attachment #497571 - Flags: review?(kdudka)
Attachment #497578 - Flags: review?(kdudka)
ckpem.h preliminary review:

line 69: "Allows us to data description objects to this PKCS #11 module"
should made to be sensible English (looks like a verbetum copy of the comment on capi/ckapi.h, so the mangled English is probably my fault).

Probably should read "Allows us to hold data description objects in this PKCS #11 module".

Line 82: 'struct pemKeyParamStr'
This structure holds 2 things, generic private/public key values (used by the cert and key objects), and RSA specific parameters. Ideally this should be split. The RSA specific parameters should be kept together in a single struct with the work RSA in it. That way you can easily see how to add support for other key types.

line 91 "unsigned char publicExponentData" not used, should be deleted.

lines 102, 103: provName and containerName are CAPI cruft from ckcapi.h. They are not used and should be deleted.

line 123: labelData is appearantly not used (it appears to be 'freed', but never allocated'. If this is true, it should be deleted.
line 124,5: derSerial is not used and should be deleted.

lines 193-201: there is a dittography problem here it appears 188 to 194 was somehow replicated at 198, with a partial correction to deal with a compiler issue. In addition it appears that nss_pem_data and nss_pem_nObjects is not used (instead the variables are pem_data and pem_nObjects below) this whole block.

Line 209: The following appears to to be used and should be deleted:

  pem_SlotDescription
  pem_TokenLabel

Line 271:  A short description of some of these functions would be helpful.



Other comments:
   There appears to be non-NSS standard indentation going on. As a last step of this review, this should be fixed, but not until everything else is dealt with (I don't want to deal with whitespace diffs until the end).
preliminary review continues:

No comments for manifest.mn (in ckfw), Makefile, anchor.c, ckpemver.c, config.mk, constants.c (for now), manifest.mn (in pem), nsspem.def


comments for parg.c:

1) memory management. 

pargs.c defines it's own memory management functions that are not used at all anywhere else in the pem module. pem_malloc, for instance is fine, but only pargs.c uses it.

The file has a funny mix of pem_Malloc, PR_Malloc, pem_Free, PR_Free, and PORT_Free. Please pick one and stay with it. The purpose of having multiples is so that one can change the basic allocator easily, but with the current code, that just isn't possible.

2) local only used global functions.

pem_Malloc, pem_StrNdup, pem_Strdup, pem_Free, and addString are all functions that are declared globally, but only declared and used in parg.c. They all should be static (assuming the survive the fix for 1) above.

3) variables allocated outside of the beginning of a block. Not all the NSS platforms support this extension of C syntax. please fix these I see them in pem_StrNdup, pem_Strdup, pem_ParseString, pem_FreeParsedStrings


pem_StrNdup - This function could reference uninitialized memory if instr is smaller then inlen. This isn't the case with any of the current callers, but it's a bit dangerous since the semantic is different than the normal strndup() function. If pem_StrNdup survives the fix to number 1, then we should fix it to have a more strndup like semantic, or at least put a large warning comment above the function.

addString - 
   1) Note the PR_Malloc/PR_Calloc(). comment 1 above may have an effect here.
   2) There is no santiy check on *returnedstrings. As coded, *returnedstrings should be NULL if stringcount == 0 and should not be NULL if stringcount != 0.
   3) The allocation of one entry at a time seems a bit inefficient (I'd rather see a block, but that would require keeping both a current_count and block_size_count.
   4) addStrings adopts newString, and never returns an error. That means if we fail to get space in *returnstrings, we should free newString rather than leak it.

pem_ParseString() -
   line 133 Why is 'inputstring' an unsigned char when inputstring is clearly a null terminated string (and thus a char *). It's callers in the file all pass char *.
    line 147 nextchar is an unused local. The while loop  would be clearer without the unnecessary assignment.
    line 148 len should probably size_t (minor).
    line 158 len >0 should be len == 0. len can't be negative.
    line 161 addString does not return an error, so we don't know if we really should increment (*numStrings) or not. This problem could happen if newstring is NULL (pem_StrNDup failed) or addString couldn't extend returnedstrings (calloc failed). In both of these cases we can returned returnedtrings that are shorter than numStrings, or return returnedstrings with 'holes' where newstring was not added. I suggest that addString return the current count so it can increment that count if everything is OK.
     General comment on the while at 147 to 169: It makes me nervous when I see while(*ptr) without knowing for sure ptr will get properly incremented. In this case it does, as long as strchr and strlen are functioning correctly. That's a reasonable assumption, but I would personally put some paranoic asserts to make sure instring is advancing [and not skipping over it's null byte] (just a preference, not a requirement).


pem_FreeParsedStrings - returns an error if numStrings== 0 or instrings == NULL. It should return success if both are NULL and an error if neither are. It's ok to try to free empty strings here. I think this is moot, since I have no callers actually checks the return code. (NOTE: if it's possible for instrings to be non-NULL and numStrings to be == 0, then we will leak. In that case the fix is only return if instrings == NULL, The rest of the code will handle numStings == 0 properly.


pem_parseFilePairsParameters -
   calling pem_ParseSting() seems like a lot of work (including lots of memory allocations that can go wrong) just to count the number of ';''s. It seems to me you could just assign the token entry for args and be done with it. Detecting more than 1 ';' doesn't seem all the worth while to me.

pem_parseTokenFiles -
    It's not clear what this is supposed to be parsing. It's clearly parsing the "files={...}" part of of the token= arguments, but what format is the {...} supposed to be in (there is nothing in any comments that say what it is).
    The options are 1) blank separated lists of filenames, 2) blank separated lists of name/value pairs (cert=<filename> certkey=<filename> ca=<filename> etc.). The function is doing neigher correctly.
    Given that SlotFiles is a blank separated list, so should files here. (the formats should be exactly the same, or the nsssysinit stuff won't work.
    The code is trying to parse name/value pairs, as seen by the call to secmod_argGetName() and secmod_argFetchValue(). This is confused because it fetches the name, but never actually looks at it. What is more, the code appears to be set to handle multiple certificates in the tokens=[##=(files={...})] spec, but if we specify more than one file here, we are going to allocate space all the files specified, but we are only going to store the first one found and ignore the rest.

line 282: answer to 'FIXME' is yes, sort of. The stuff you are doing with SlotFlags and SlotFiles should be handles the same way as files= and flags= inside the token= block. You should be able to use the same code (Also SlotFlags should just be flags and slotFiles should just be files.

pem_HasNewFormatParameters -- is busted if you don't have a slotFiles= line.You will wind the while with *index=0, but never reset when you check tokens= and files=. You can dispense with the files= check. You can check for tokens= in the same look as slotFiles= (which should be renamed to slotFiles=). You could also check if slotFlags= (renamed to flags=) if any of the 3 are there, then you are clearly in the new format, otherwise you can assume the old format.

pem_ParseModuleParameters --
   line 456 -- shouldn't you check and return rv?
   line 466 -- what about spaces in the filename?
   line 481 -- you are combining all the files from all the tokens into one list, the caller then loads these into a single token. This isn't what the caller requested!

pem_ParseParameters --
    You are statically creating the parameters list then freeing it. That should be a hint that you are loosing information!.
Also in pargs.c
comment 425 "Note: of the requested slots are in use, NSS will automatically load them into new slots. How do I know that?"

This is a bit confused.
NSS does not 'load' anything into new slots. I think the comment is referring to application requests to multiply load the pem module. NSS will detect that the pem module is loaded (because C_Initialize returns CKR_ALREADY_INITIALIZED). In that case NSS will map the tokens= and base slot parameters to new slotID's and will ask the pem module to create new tokens with these new slotID's. NSS knows which slotIDs are not in use because it queries the slots from C_GetSlotList().
review of pfind.c

Some of these errors appears to be errors in the vorlage of libpem (libcapi). Even so, the should be fixed.

Line 47 -
In ckpem.h we define pem_data. Now we are declaring gobj, which are appearantly the same thing. It's a global, so we should have a name with pem_. pem_gobj or pem_obj seems best to me.

NOTE: there is no lock of access to gobj or pem_nobj, or in pinst count or size.

PUT_Object() - There's a memory leak if ZREALLOCARRAY fails. *listp will not be freed, but will be set to zero. This can be solved by declaring a temp inside the if block pemInternal *list;
 The set list = *listp ? ...
 After size += set *listp = list;

Lines 144-172 delete this unused function.

Lines 325: first of several race conditions against the global gobj and pem_nobj.

We should be locking around access to these variables.

Line 330: This is a performance issue, so not required to be fixed to check in, but we should write a bug about this. We are storing all of our objects for all out slots in one massive global array. We then have to filter through all the objects looking for ones that are in our slot. Objects are always referenced with respect to slot, so we should keep slot specific lists of objects and keep everything in them.

Lines 377 and 383: need to set *pError on failure.
pinst.c

Line 50, another definition of gobj, and a declaration (with initializer of pem_nobjs. There should be zero or one instance of initializing pem_nobjs and gobjs (BSS is initialized to zero in C).

token_needsLogin[NUM_SLOTS}; This should be in a slot specific structure with no hardwired NUM_SLOTS.

line 54: global variables called 'size' and 'count'? really?. In their usage, count is exactly equal to pem_nobjs ans size is the size of gobj (rather than the number of objects on gobjs).

Same locking comment as pfind.c for gobjs.

lines 60-177: libpem already links with libfreebl, which itself requires libnssutil. There is no reason to avoid the asn1 decoder in nssutil in libpem. (This can be a bug that gets fixed in the future, but we need to track it).

CreateObject: lines 208-217. We should do sanity checks on type, certDER, and keyDER here against the objClass and ASSERT on failure. We should also have a default case and return an error or ASSERT.

AddObjectIfNeeded():  It's not clear what this function is intending to do. Is it trying to keep from loading duplicates objects? If so it's not really doing it's job. It will successfully keep from loading two of the same object type with the same nickname, but nicknames aren't always guarrenteed to be unique. Adding the objid check won't help because the caller is supposedly creating a unique objid for the object.

I suggest the following changes: 
   either 1) remove the check for identical objects altogether, or 2)remove the objid and nickname from the check and check keyDER or certDER depending on objClass.

Also note: It looks like objid is really CKA_ID which is different from objectID, which needs to be unique for all objects in the token (I believe the latter is assigned by ckfw for you.


NOTE: this functions is a major race between all the updated accesses to gobj and pem_nobjs, count, size.

Note: count is exactly equal to pem_nobjs and is unnecessary. size should be renamed.

Line 398: objectid is unused (well actually it may be used by the key size, but in that case we'll match up the key with a random ca cert, which we clearly do not wnat to do).

in the (keyfile) block, we should fail if cacert is set.

pem_Initialize: 

Line 517: The other half of the pargs argument where we loose the information about which cert maps to which slot.

line 550: We add certfiles in a loop into a single slot. This is fine for certs, but we can only take one cert/key pair into a slot because the key provides the password. AddCertificate properly checks for multiple keys within a file. Here we should make sure we don't try to add multiple keyfiles within a slot.


Line 570: We are blindingly initializing token_needsLogin to false for all tokens. This is a bug for the following reasons:
   1) the current code could have loaded a key into slot 0, in which case the token_needsLogin should be set to true for the current token.
   2) once you fix the problem of colapsing all the certs into a single token, several tokens will need to deal with token_needsLogin.
   3) the global should be a a slot/token specific structure that handles all the slot/token data (mdSlot would be a *very* good candidate).

pem_mdInstance_GetSlots(), pem_mdInstance_GetSlots()--
   I'm a little worried for 2 reasons.... 1) we are returning all the slots, whether or not we need them. This will break NSS's attempt to find an empty slot (because you will have none), and 2) we are allocating the slot structure fresh each time. We need to allocate NSSCKMDSlots once and return the same ones every time.
Comment on attachment 497578 [details] [diff] [review]
pem module

r-

I've gotten to pobject.c and see it does not implement CKO_NSS_NEWSLOT or CKO_NSS_DELSLOT, which is exactly what needs to be added before pem goes in.

please resubmit which the existing comments fixed and CKO_NSS_DELSLOT CKO_NSS_NEWSLOT.

bob
Attachment #497578 - Flags: review?(rrelyea) → review-
(In reply to comment #54)
> AddObjectIfNeeded():  It's not clear what this function is intending to do. Is
> it trying to keep from loading duplicates objects? If so it's not really doing
> it's job. It will successfully keep from loading two of the same object type
> with the same nickname, but nicknames aren't always guarrenteed to be unique.
> Adding the objid check won't help because the caller is supposedly creating a
> unique objid for the object.

AddObjectIfNeeded() is actually my work.  If you revert the patch adding AddObjectIfNeeded(), you'll run out of memory after ~200 SSL connections through libcurl on a 2GiB machine.

libcurl calls PK11_CreateGenericObject() to load the root CA bundle per each connection.  It means that each SSL connection allocates about ~10 MiB of memory, which is never freed until the shutdown of NSS.  The amount of allocated memory was growing so fast, that it limited the use of SSL in libcurl for one-shot applications only.

Although the design of AddObjectIfNeeded() may violate the NSS default approach, it has successfully terminated most of the bug reports about unusable SSL in libcurl.  If you want to drop AddObjectIfNeeded(), you need to find another working solution for libcurl first.

Btw. I don't think the problem affects only libcurl, but generally all (originally) OpenSSL-based applications that operate on PEM certificates.  For instance, nss_compat_ossl uses exactly same approach as libcurl.
> libcurl calls PK11_CreateGenericObject() to load the root CA bundle per each
> connection.  It means that each SSL connection allocates about ~10 MiB of
> memory, which is never freed until the shutdown of NSS.  The amount of
> allocated memory was growing so fast, that it limited the use of SSL in libcurl.

That sounds reasonable, then. The current code is working because it is recognizing the nickname of the object based on the filename. It would be better to reject the object itself (that handles the case where you have multiple root cert files with different names.

It's OK to reject attempts to load the same object twice (or actually map it to an existing object). We should just use the derCert/derKey to detect this.

bob
Reading the code once again, I see that the amount of allocated memory still grows per connection, but only by sizeof(pemObjectListItem).  It is not much, but still not perfect.

I roughly remember that the framework didn't allow me to return pointer of an already existing object from NSSCKMDSession::CreateObject() because of some internal pointer hashing ... but haven't had enough time to investigate if it was a bug or feature.
Given the numerous review requests, I thougth wise to have a review of the progress made so far.  Also, find out if I'm on the right track for the new functionality. This version addresses most of review comments but is only partial progress. I still have to support files with embedded spaces and support CKO_NSS_NEWSLOT and CKO_NSS_DELSLOT. There is some code for the latter but is not activated as currently pem_CreateObject sets the error to CKR_FUNCTION_NOT_SUPPORTED on that case.
Attachment #497578 - Attachment is obsolete: true
Attachment #509452 - Flags: review?(rrelyea)
Attachment #497578 - Flags: review?(kdudka)
Attachment #509452 - Attachment is obsolete: true
Attachment #547097 - Flags: review?(rrelyea)
Attachment #509452 - Flags: review?(rrelyea)
(In reply to comment #60)
> Created attachment 547097 [details] [diff] [review] [review]
> Patch V5 - partial progress

A summary like this says nothing.  Could you please summarize the changes made in the last two iterations more verbosely?  Thanks in advance.
(In reply to comment #61)
From the last reviewed to patch V4 I made the follwing changes:

Per Comment 5
In ckpem.h - Removed some unused fields: 
provName and containerName from pemKeyObjectStr
labelData and derSerial from pemCertObjectStr

also removed unused or duplicated globals
nss_pem_data, nss_pem_nObjects, pem_SlotDescription, and pem_TokenLabel

Added locking objects pem_slotsLock and pem_objsLock

Added the structs SlotStringStr and PSlotExtended
For the new parser we need to keep track of the slot
along with the pem files that specified in the module
initialization parameters, thus SlotStringStr.

PSlotExtended keeps track of the slotID, whether slot is present
or if it requires login, and mantain a module instance global slot list.

Added comments to the prototypes and fixed the wording on others.
Add various slot list access macros as the current array implementation as array
will likely change and we wan't the rest of the code be protectd from these aticipated changse.

Got rid of some-pem only allocation functions in favor PORT_Alloc.

pargs:
addString returns the new count. It it succeeed it return oldCount+1
and if it fails count stays the same signallling a failure to allocate
memory for the new string in the list.

addCertOrKeyFile is new.
In V4 I cleaned up pem_ParseString, I predict in the comment than it's days are numbered. I want only one parser, the new one. If the parameters are othe old style there is a simple way of transforming to the new format and pem_ParseString won't be needed. You'll see below.

pem_parseFilePairsParameters doesn't call the old heavy-duty pem_ParseString
and instead relies on strtok to split the string.

Refactored out common parsing code into pem_parseFilesCommon
that takes a ParamType type argument. This is a discriminator
for a switch statement
+        switch (type) {
+        case type_pemTokenParameters:
+            ((pemTokenParameters *)parsed)->filesCount = i;
+            ((pemTokenParameters *)parsed)->files = files;
+            break;
+        case type_pemModuleParameters:
+            ((pemModuleParams *)parsed)->filesCount = i;
+            ((pemModuleParams *)parsed)->files = files;
+            break;
to stuff the filesCount and files into the right stucture.
The caller can specifiy just files or files as part of a desired slot. The parsing is common.

pem_parseTokenFiles(char *params, pemTokenParameters *parsed)
= pem_parseFilesCommon(params, type_pemTokenParameters, parsed);

pem_parseFiles(char *params, pemModuleParams *parsed)
= pem_parseFilesCommon(params, type_pemModuleParameters, parsed);

pem_HasNewFormatParameters is fixed

In pinst.c:
Renamed gobj to pem_objs, added pem_objects_capacity and a pem_slotsLock
Similarly we have pem_nslots; pem_slots_capacity and pem_slotsLock;

Added AllocateSlots to allocate the slots once

AddObjectIfNeeded checks for duplicates based on certDER/keyDER
It acquires the pem_objsLock before searching and releases when done.

pem_Initialize creates a Lock, uses (char *) for the casting,
uses strtok to split a strings in instead of pem_ParseString
Fixed the freeing at the end.

pem_Finalize frees the pem_objsLock and pem_slotsLock
created by pem_Initialize

pem_mdInstance_GetSlots no longer allocates slot on each call and instead
returns a copy of the list.

pfind.c:
Fix a potentail memory leak in PUT_Object
Removed the #ifdef'd-out pem_derUnwrapInt

pobject.c:
Added AddNewSlot -- which eventually went away
Added RequestIsForSlot a convenience to keep pem_CreateObject tidy
Added pem_CreateNewSlot to create or slot. It's modelled on what 
I have seen in softoken (the only module that adds or delete slots)
and is just an outline to get some advise from Bob.

In pem_CreateObject I speculate how support for CKO_NSS_NEWSLOT and  CKO_NSS_DELSLOT may look like, it's not there yet.

psession.c:
Remove the NSS_EXTERN_DATA stuff which we get from the ckpem.h header,
Uses the locks in various places, and acces the slot array via macros.


From previous (v4) tothe  current (V5) the changes are not as many.

In ckpem.h: Fixed the wording of various comments
The one-line functions canonicalizeParameters and freeCanonicalizedParameters
became the canonicalizeParams and freeCanonicalizedParams macros.
Deleted pem_ParseString and pem_FreeParsedString

pargs.c:
Added comments and Removed pem_ParseString and pem_FreeParsedStrings as I had predicted in the comments of the previous version.

pinst.c:
Made pem_Initialize a lot easier on the eyes reviwers and maintainers alike.

pobject.c:
Got rid of more NSS_EXTERN_DATA lines.
pem_DestroyInternalObject has a nasty bug at the end
where if (NULL != pem_objs) was missing the braces arioud the three statements.

Removed AddNewSlot which intended to support Realloc in the slot list.
Fixed pem_CreateNewSlot uses canonicalizeParams/freeCanonicalizedParams
correctly.

Fix for a bug in pem_CreateObject 
https://bugzilla.redhat.com/show_bug.cgi?id=717338
Thank you Nalin.
A line count on the pem module under review shows me 7270 lines. Just counting lines in the diff relative to what ships on fedora/rhel 6 shows 1968.
Comment parg.c (so far).... This is a newly rewritten file, so it's not surprising that the list of comments are a bit numerous:


pargs.c

pem_StrNdup -- 

This is not a strndup function. The standard strndup copies to the end of the string up to length. this function always copies up to length. I think the usage is correct in the program. A better name would be pem_makeString() or some such. The function clearly depends on the input string being at least 'len' long or more. It should at least contain a comment warning about this.

addString -- 

1. first the definition of this function is weird. Usually one expects such a function to either return the new string, or return an error. (See also addCertOrKey). Looking at the usage of this function, imagine what code changes you would have if you make it return an error (not that you can get rid of some temparary variables, and some error prone additional increments of the count).

2. /*caller will free new string */ -- comment is correct, but in a weird place. it's where we didn't actually allocate any new strings.

3. if (stringarray) ...  This if is redundant. I don't mind paranoia, but in this case, it's wrong paranoia. That is if we actually could arrive here with stringarray == NULL, then we will incorrectly increment stringcount even though we didn't add the new string.

4. Note: on success we adopt newString.


addCertOrKeyFile --

< all the same issues as above >

pem_ParseStringFile --

This function is OK. I just want to know how changing the signature of addCertOrKeyFile will simplify this functions.
	PRInt32 newCount =
	    addCertOrKeyFile(returnedstrings, newCert, (*numStrings));
	if (newCount == *numStrings) {
	    PORT_Free(newCert->filename);
	    PORT_Free(newCert);
	    goto loser;
	}
	(*numStrings)++


becomes:

	rv =
	    addCertOrKeyFile(returnedstrings, newCert, numStrings);
	if (rv != CKR_OK) {
	    PORT_Free(newCert->filename);
	    PORT_Free(newCert);
	    goto loser;
	}
'nuf said.

Also Note: you are including a C++ style variable declaration here in C code. Not all compiliers will like this...

pem_parseFilePairsParameters --

This function appears to be a function that meerly validates input,  which is confirmed by the comment. I would think 'parse' should be 'verify' (though it does store the param into the pemFileParams structure as a side effect).

Note: tok=strtok(NULL,";") isn't thread safe. You should either use strtok_r or strcchr().
Note2: strtok and strtok_r modify the original string. This means you many not be getting the full string stored in the certAndKey field of the parameter block (see man strtok!).

Also lastOne variable is never used.

pem_parseFilesCommon --
BUG: parameters come in name value pairs. This function is reading a name value pair, it is ignoring name. It looks like the code is trying to handle the case where the params are (cert=cert_file keyCert=key_cert_file), etc. not the case where the params are (file1 file2 file3;file4 file5) etc. which is what I was expecting from the comments so far in the code.
LEAK: your file parameters array may contain certAndKey data that's allocated. When you free it at line 362, you Leak all the data you've allocated to this point.

pem_HasNewFormatParameters -
BUG: SECMOD_HANDLE_STRING_ARG(index,files, "flags=") should be flags, "flags=");

pem_ParseModuleParameters --
Line 546  pem_ParseStringFiles(parsed->files, ' ', &numFound, &certs);
parsed->files is a char ** (or at least that's how it's set), and pem_ParseStringFiles assumes a char *).
ling 550 if addCertOrKeyFile fails, we leak certs[i]
Line 576 if addCertOrKeyfile fails, we leak 'newOne'.
Line 574 parse->tokens[i].files->certAndKey... at this point the space for the value certAndKey is now owned by newOne->file. We should NULL out the certAndKey entry to protect from double frees or reference to freed objects.
Mostly for my own benefit.
Attachment #549673 - Attachment description: interdiff last_reviewed to under_review - not for review → interdiff last_reviewed to under_review
Ah, you decided to review the risky one, pargs, first. Your comments have inspired me review as well.

pem_StrNdup can me removed, it's only used by pem_ParseStringFiles
pem_ParseStringFiles which isn't very very memory smart and
can easly be replaced with a CountStringFiles whose function I'll explain.

We are duplicating strings (or SlotStringStr's) and adding them
one by one in addString (addCertKeyFile) and reallocating the array
one at a time. In a old comment you alluded to this I think.

Wouldn't it  be better if we could anticipate how much that
certstrings list (array) needs to grow and grow it once. Then
add the items to it in a batch? The information is available once
pem_ParseModuleParameters has finished parsing the strings from
pkcs11.txt in that while (*index) {} loop. Theat pemModuleParams structure 
has the info of needed. anpem_ParseModuleParameters could query it
onec or twice and allocate the certs and add them all at once (or in two calls)
knowing that there is room. You use the adopt term. It's apprpriate.

Amother thing memory management wise is that there are huge memory leaks.
pem_ParseModuleParameters owns those lists and should free them before exiting. 
Freeing the stuff hanging off the pemModuleParams should the responsibility of 
pem_ParseParameters who own it. It's a stack based structure that made me forget
all of that heap-based stuff attached to it.

Don't mean to interrupt and spoil all that fun you are having reviewing pargs.c but I thought I would share my thoughts.
> Wouldn't it  be better if we could anticipate how much that
> certstrings list (array) needs to grow and grow it once

That would even be better.

I was just noting that the signature existing signature for the function was a bit odd, and generated unnecesssary complication when it was used.
(In reply to comment #66)
>...
> pargs.c
> 
> pem_parseFilesCommon --
> BUG: parameters come in name value pairs. This function is reading a name
> value pair, it is ignoring name. It looks like the code is trying to handle
> the case where the params are (cert=cert_file keyCert=key_cert_file), etc.
> not the case where the params are (file1 file2 file3;file4 file5) etc. which
> is what I was expecting from the comments so far in the code.
Line 320:        char *name = name = secmod_argGetName(filesIndex,&next);
Line 350:        char *args = secmod_argFetchValue(filesIndex,&next);
and args can be file1 or file3:file4 which I check for in
Line 337:        rv = pem_parseFilePairsParameters(args, &files[i]);

> LEAK: your file parameters array may contain certAndKey data that's
> allocated. When you free it at line 362, you Leak all the data you've
> allocated to this point.
>
It's in the else part which when an error has been found that I free files. No I don't want to free files with parsed now had adopted. The code style of bailing out as soon as we know there is an error makes reading better. 

On Lines 307:308 requiring count to be one is wrong. I meant to require one when type is type_pemTokenParameters which the files list is nested in tokens.
... tokens=0x2=[files={server1.crt:server1.key} is okay but
... tokens=0x2=[files={server1.crt;server1.key server2.crt:server2.key} isn't. Did we discuss at some time such a constraint?
> Line 320:        char *name = name = secmod_argGetName(filesIndex,&next);

Gets the named part of the name/value pair (ie. cert=test, this will return cert).
If there is no '=', then it just returns the token.

>filesIndex += next;

Advances past the name.

>Line 350:        char *args = secmod_argFetchValue(filesIndex,&next);

Reads to the next unescaped blank (including embedded spaces in quotes). In our example, test.

If you have a list of certs: server1.cert server2.cert server3.cert.

This code would assign server1.cert to name, and server2.cert to arg.
> It's in the else part which when an error has been found that I free files.

Right. The problem is your failure is coming from a loop. If several succeed, but the last fails, you leak data. It's easier to loop through all the files (as long as files had been properly initialized to NULL) and free everything than it is to try to figure out how many succeeded and how man failed after the fact.
OK, I see that purpose of your comment about requiring count to be one. If that's what you want, then I wouldn't code it as a loop. I'm not sure why the constraint is there, whoever, since you pretty much have all the infrastructure to support more than one occurance, other then the error processing.

bob
find.c


PUT_Object(obj,err)    line 57.
    The test 'if ((pemInternalObject **)NULL == *listp)'
        should be 'if ((pemInternalObject **)NULL == list)'

Also, the Macro uses the following variables that are 'silently' passed in:
   listp, size, count. It seems like it should be declared much closer to where it's actually used. It can't possible a general macro with it's intimate knowledge of the of it's caller's local variables.



pem_FindObjectsInit() Line 341-377. now I'm confused. How is Find objects even working? The variable temp is initialized to NULL. Then it's address is passed into collect_objects and it becomes listp. So *listp == NULL, but the error in the macro above causes the PUT_Object to fail on it's first call. So either pem_FindObjectInit will return success in the case where there are not objects to find, or return CKR_HOST_MEMORY if it found an object.


pInst.c

dataStart... This function looks familiar;). Unfortunately it looks like someone was trying to detect buffer overflow errors, but did so in a singuarly incorrect way. Line 83, While '0' is not a valid tag, the comments makes me thing the person who inserted the check is checking for the wrong thing. Instead, before the buf[used_length++] we should check the length variable. We know that length must be at least 2  at this point. Once we know that we can proceed to check the next to buf bytes.

In the new code would be something like:

    unsigned char tag;
    unsigned int used_length = 0;

+   /* blow out when we come to the end */
+   if (length < 2) {
+       *data_length = 0;
+       return NULL;
+
    tag = buf[used_length++];

    if (rettag) {
        *rettag = tag;
    }

-   /* blow out when we come to the end */
-   if (tag == 0) {
-       return NULL;
-   }

    *data_length = buf[used_length++];

    if (*data_length & 0x80) {

        int len_count = *data_length & 0x7f;

        *data_length = 0;

+       if (length < len_count+2) {
+           return NULL;
+       }
+
        while (len_count-- > 0) {
         *data_length = (*data_length << 8) | buf[used_length++];
        }
    }


AllocateSlots - at line 127, We free the slots we allocated (good), but we don't free the array before we return (bad). We need to do both. NOTE: I'm assuming pem_NewSlot() allocates the slots in regular memory, not an arena....

GetCertFields --This appears to be a copy from coolkey. Coolkey sometimes whats to get a few fields from the cert, other times wants more (like validity period and subject_key (read subject public key)). PEM only needs issuer, serial, and subject. We can dispense with derSN, valid, and subjkey).

AddObjectIfNeeded -- 
Linse 352 to 357: The FIXME: copy-pasted from... and the resulting nickname could can be deleted. nickname does not appear to be used in theis function.
lines 367 and 374, we blindly check u.cert.derCert.data and u.key.key.privateKey->data in the pem_obj array without checking to see if the pem_objs should have a cert or key object. Also, we check the data without first checking the length, which could lead to UMR's (and, in theory, crashes).
line 362: In the for loop, we should check that the pem_objs[] matches the objClass of the thing we are checking before we do any other tests.
Line 395: Race condition. io->gobjIndex doesn't need to be locked, but reading pem_nobjs does. If we task switch here, and another thread creates another object, it will increment pem_nobjs on us and io->globjIndex won't match. (just move int inside the lock).

AddCertificate --
Line 457 -- We try to add a trust object unconditionally. AddObjectIfNeeded does not check to see if there is a matching trust object already added. (The fix would likely be in AddObjectIfNeeded.
Lines 497 and 501 -- unless something tricky was done here by ReadDERFile, These will leak memory. SECItems point to allocated data, so you need to loop through and free the alocated data before you free the array.
line 487. At this point you have keyobjs alocated, but you never free it (see previous comment on how to free SECItems.

pem_Initiallize
Line 572 ... canonicalizeparams -- canonicalize should do the equivalent of secmod_addEscape() (see mozilla/security/nss/lib/pk11wrap ). NOTE: the canonicalizeparams macro is defined in nsspem.h.
Line 609: Move CERT_KEY_SEP to nsspem.h and use it in other places where you are separating a key from a cert. NOTE: all your docs say this separator is ';', but this code is using ':'! Also note: this is different from your validity check code in pargs.c!
line 611: why are you using this array, why not just cert and key. You never pass the array around as an array?
pem_Initialize isn't thread safe. I don't think it needs to be, but we are doing an atomic set at the end that implies it's supposed to be thread safe.
Line 614: If you find a sep, you need to set it to zero, otherwise cert will still be the original string. (NOTE this will change the data in the parsedStrings. If something else depended on the strings, it would fail. I don't think we want that. Much better, create 2 string fields in parsedStrings, certFilename and keyFilename. Then make pargs.c do he heavy lifting and split these two itself. This code would then be suitably simplified.

pem_Finalize:
Line 663: pem_objs --- like SECItems, I'm pretty sure that have extra data associated with them. We are definately leaking here. We probably need an iobjs_free routine here. I need to look at the memory model of pem_objs more closely. I think we are trying to many short cuts and generating a bunch of problems for ourselves.
Line 672: Same thing with pem_slots.

-----------------------------------------------------------------------------------
pobject.c
Line 190: What's this SEC_ASN1 template doing here? If you are linking with nssutil (and thus have access to the nssutil der decoder), why are we still using the mini-hand decoder from coolkey in pInst.c. Also, why are we using the decoder, when we've already parsed out the subject? If we've removed the nssutil der decoder, then we need to remove this template.

pem_FetchPrivKeyAttribute
Lines 312 to 359 are RSA specific attributes. There is some noise of supporting keys other than RSA. If so, these should split out into a pem_FetchRSAPrivKeyAttribute() function. Also pem_PoplateModulusExponent should be called pemPopulateRSAKey(). It makes if functionality more obvious.

pem_FecthPubKeyAttribute
Lines 413 to 418: See above

pem_FetchTrustAttribute
Line 514: default to &pem_trusted? really? I think we should return NULL if we don't understand the attribute.

pem_CreateNewSlot
Line 1070: See canonicalizeParams comment in pinst.c
Line 1088. You should return CKR_ATTRIBUTE_VALUE_INVALID the application requests an already inuse slot on CKO_NSS_NEWSLOT. and CKR_ATTRIBUTE_VALUE_INVALID if CKO_NSS_DELSLOT.
Question, why aren't you closing the slot on CKO_NSS_DELSLOT? It should at least be marked 'removed'.

pem_CreateObject
I think we're going to run into a compatibility issue here. The old way Rob used to populate slots is to use C_CreateObject, which is fine, but he used the standard PKCS #11 attributes in a non-standard way (which is bad). We're going to have to do some interesting dancing to make writeable objects work correctly.
Line 1327: Not the correct way to fix a race condition, This sleep needs an explanation...
Line 1344: Why Am I reviewing this when #if is set to 1?
Line 1354: I'm not sure why you are setting CKR_SLOT_ID_INVALID here... Softoken doesn't do it. Why do you want the framework to see CK_INVALID_HANDLE?
Comment on attachment 547097 [details] [diff] [review]
Patch V5 - partial progress

r- rrelyea
Attachment #547097 - Flags: review?(rrelyea) → review-
Analysis of the reference model of pemInternalObjects, in particular how the relate to the pem_obj array.

Overview:

The pem_obj array holds pointers to pemInternalObjects. All the objects in the pem_obj array should have a reference owned by the pem_obj array, which should be released at shutdown. This guarrentees that the object will not disappear while it's stored in the array. In practice it looks there is at least one reference which the pem_obj array holds, but that reference is never freed.

In addition, functions that return pemInternalObjects to caller, in general, should return references that the caller is responsible to free. This guarrentees that the objects will not disappear while the caller is using them. Short cutting this model, even when it is safe, can lead to confusion about when to free these objects and can easily lead to reference leaks or multiple frees. The module is rife with these short-cuts and thus the predicable issues associated with them.


Analysis by file:

pfind.c

When looking up objects, pem_FindObjectsInit() calls collect objects to find objects stored in the pem_obj array and copy the pointers to a local array of found objects. References to these objects are *NOT* obtained, the entire find infrastructure 'piggybacks' on the references to the pem_obj itself. This means if the objects are removed from pem_obj while a find is in operation, the find will return pointers to freed memory.

As each object is returned in pem_FindNext(), it's a reference is acquired by the framework exactly once for it's lifetime. This is correct becasue the framework has only one abstract object for any object in the system, and will only free one reference to any internal object. This is an acceptable way of dealing with this issue, though a cleaner way would be to modify pem_CreateMDObject to get it's own reference if when it 'creates' the MD object.

pem_FindObjectsFinal. the array that stores the pointers is freed, but no references are freed. This is correct with the current code since no references are acquired.

Conclusion: find.c uses pemInternalObjects self-consistantly, and therefore currently does not leak them. It does not handle them safely in a dynamic system where objects could be deleted. 

**Recommendation: collect_objects should acquire references to them. pem_FindObjectsInit() should free those references in the error cases. pem_FindObjectsFinal() should free them before freeing the array.

pinst.c

CreateObject returns pemInternalObjects with references of zero! This is counter intuitive, and leads to some asymetries in some other functions below. One expects the the following sequence will proberly clean up our objects:

     pemInternalObject *o = CreateObject(....);
         .
         /* use 'o' */
         .
     pem_DestoryInternalObject(o);

However, since CreateObject returns o with a reference count already at zero, our free routing will be quite confused.
**Recommendation: CreateObject should return a reference count.

AddObjectIfNeeded returns an object with a new reference. If the object already exists, we increment the reference. If the object doesn't exist, we create one (with CreateObject) and increment the reference, and stores it in the pem_obj. NOTE: this is correct if CreateObject returned a reference of 1. In that case the caller receives a reference he is responsible to free (from the increment), and pem_objs receives a reference it is responsible to free (adopted from CreateObject). In the case of an old object, the caller receives a new reference.
**Recommendation: While AddObjectIfNeeded has an appearant bug, it is created by real bugs in CreateObject and AddCertificate. There is no need to change AddObjectIfNeeded.

AddCertificate calls AddObjectIfNeeded strictly for the side effect of storing the object in the pem_objs. Even though AddObjectIfNeeded always returns a reference that the caller is supposed to free, AddCertificate never frees that reference. This is likely because of the first bug identified. If CreatObject returns an object with no reference count, then freeing the reference count from AddObjectIfNeeded will delete the object immediately. Also, the looser: block calls nssZFreeIf(o), which is always wrong. Fortunately it appears we always reset o to NULL, probably because of the line. Just remove it.
**Recommendation: Once CreateObject is fixed, free the reference returned by AddObjectIfNeeded using pem_DestroyInteralObject(o). Do this for each object returned by AddObjectIfNeeded. Also remove the bogus nssZFreeIf(o).

pem_Finalize frees the pem_objs array, but not the references pem_objs holds.
**Recommendation: Free the referneces pem_objs holds in it's array.



There's also some work to deal with in pobject.c, notable pem_CreateObject.
Comment on attachment 497571 [details] [diff] [review]
pem module test application

clearing the flag until we actually get a pem module r+'ed.

bob
Attachment #497571 - Flags: review?(rrelyea)
Target Milestone: --- → 3.14
Target Milestone: 3.14 → 3.14.1
Can we please add a quick summary to this bug, what is the next step? Are we waiting for review comments, or are we waiting for better code?
This bug needs a new submission to address the numerous defects pointed out in the last review, where it was rejected. There requests are many and the salient ones are:
1) Major fix memory management, in specific poor and inconsitent use of reference counting.
2) Implement CKO_NSS_NEWSLOT or CKO_NSS_DELSLOT
3) The rest of the requests, though numerous, are easier than the first two.
4) Write a pem module test tool and shell scrips with test cases. T
The current one is very inadequate.

The work for a new submission is significant and I estimate one worth of dedicated effort.
I plan to resume work on clanup up the  pem module for submission once my workload downstream subsides a bit. My best guess is it will be another month before I can get the necessary bandwith to focus on this effort.
I would prefer to have the changes in the git repository [1] rather than patching patches with no track of history, ideally one commit per issue.  Maybe we could create a new branch for this to separate the changes from the regular bug fixes of the production version?

[1] http://git.fedorahosted.org/git/nss-pem.git
I agree with Kamil's preference and suggestions. By using a branch in the git repo I would start with a code base that is shipping already and built upon up it. The work could be split in phases for easier review. Some requests are a lot easier than others. Once the code is in decent shape I then submit the whole cvs patch here where it belongs, but this time I would be doing it with a much higher level of confidence in getting it approved without too many iterations
Target Milestone: 3.14.1 → 3.14.2
Target Milestone: 3.14.2 → 3.14.4
needs a new target milestone
Target Milestone: 3.14.4 → ---
Any news of this bug we may be interested on the debian side
Blocks: 924404
Attachment #497571 - Flags: review?(kdudka)
(In reply to roucaries.bastien+bugs from comment #83)
> Any news of this bug we may be interested on the debian side

The nss-pem PKCS#11 module can now be built separately against public NSS API, which is what we currently do in supported versions of Fedora.  The latest release of nss-pem is available here:

https://github.com/kdudka/nss-pem/releases/tag/nss-pem-1.0.2
(In reply to Kamil Dudka from comment #84)
> (In reply to roucaries.bastien+bugs from comment #83)
> > Any news of this bug we may be interested on the debian side
> 
> The nss-pem PKCS#11 module can now be built separately against public NSS
> API, which is what we currently do in supported versions of Fedora.  The
> latest release of nss-pem is available here:
> 
> https://github.com/kdudka/nss-pem/releases/tag/nss-pem-1.0.2

Given that the nss-pem module has it's own upstream, shouldn't this bug be closed?
Flags: needinfo?(kdudka)
(In reply to Elio Maldonado from comment #85)
> Given that the nss-pem module has it's own upstream, shouldn't this bug be
> closed?

Fine by me.
Flags: needinfo?(kdudka)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.