1312141 - SECMOD_OpenUserDB will allow multiple opens of the same database.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

Attachment: Unformated patch to fix the problem - not yet ready for review.

Description of problem:
It's not always safe to have multiple instances of the same database open in the same process. Because of this, the NSS initialization code will automatically combine multiple database opens into a single one. There is another way, however, to open databases: SECMOD_OpenUserDB().

This multiple open combined with a bug in softoken leads to the issue in the slapd in FIPS mode described in bug https://bugzilla.redhat.com/show_bug.cgi?id=1352109.

Comment 1

[Robert Relyea]

Attachment: nss-load-single.patch

Ok, this patch reuses some logic from pk11_load.c when loading a module that has already been loaded.I've kept the related parsing logic in pk11parse.c. Now if you try to open a userDB you will get a reference to an already opened slot.

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8804519 [details] [diff] [review]
nss-load-single.patch

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

This looks like it will address the basic problem well enough.

The formatting of this is quite sloppy.  Can you try to clean that up?

Also, I have some concerns that none of this is commented. I would like to at least have a brief description of why functions exist.

::: lib/pk11wrap/pk11pars.c
@@ +1118,5 @@
> +PRBool
> +secmod_matchConfig(char *configDir1, char *configDir2,
> +		   char *certPrefix1, char *certPrefix2,
> +		   char *keyPrefix1, char *keyPrefix2,
> +		   PRBool isReadOnly1, PRBool isReadOnly2)

I would pass a SECMODConfigList* here instead.

@@ +1127,5 @@
> +	    /* this last test -- if we just need the DB open read only,
> +	     * than any open will suffice, but if we requested it read/write
> +	     * and it's only open read only, we need to open it again */
> +	    (isReadOnly1 || !isReadOnly2)) {
> +	return PR_TRUE;

This could be split out to be more readable.

if (strcmp(configDir1, configDir2) != 0) {
  return PR_FALSE;
}
if (...) {
  return PR_FALSE;
}
return PR_TRUE;

Comment 3

[Robert Relyea]

Thanks Martin,

> The formatting of this is quite sloppy.  Can you try to clean that up?

Other then adding a few line breaks and cleaning up some trailing whitespace (which I'll clean up), the formatting should be consistant with everything else in the parent file (and in pk11wrap altogether). Is there something specific I should address?

> Also, I have some concerns that none of this is commented. I would like to at least have a brief 
> description of why functions exist.

will add.

> I would pass a SECMODConfigList* here instead.

Not all the uses have SECMODConfigLists available, which is why I broke it out. (I'll include this explanation in the comment.

> This could be split out to be more readable.

Will do

Comment 4

[Robert Relyea]

  https://hg.mozilla.org/projects/nss/rev/b7b419e4e2f6c7bdda9700ac37883dec5e1ddef8

Comment 5

[Martin Thomson [:mt:]]

Bob, check your logic, I think that you made a pretty big mistake in your refactor of secmod_matchConfig().  Check the last few lines.

I think that you want if (readOnly2) return PR_TRUE if (!readOnly1) return PR_FALSE return PR_TRUE

Comment 6

[Martin Thomson [:mt:]]

Bob, see comment 5

Comment 7

[Kai Engert (:KaiE:)]

> Bob, check your logic, I think that you made a pretty big mistake in your
> refactor of secmod_matchConfig().  Check the last few lines.

Let me try to help, because I'm not convinced Martin's code is correct either.


The original code is:
	    if (otherstuff &&
            (isReadOnly || !conflist[i].isReadOnly)) {
            ret = PR_TRUE;
            goto done;
        }
        return = PR_FALSE;

Let me simplify all verions:


Original:

    if (ro1 || !ro2)
        return true
    else
        return false

    a: ro1 true  + ro2 true  => true
    b: ro1 true  + ro2 false => true
    c: ro1 false + ro2 true  => false
    d: ro1 false + ro2 false => true
    

Bob's new code:

    if (ro1)
        return true
    if (ro2)
        return false
    return false

    a: ro1 true  + ro2 true  => true
    b: ro1 true  + ro2 false => true
    c: ro1 false + ro2 true  => false
    d: ro1 false + ro2 false => false

Bob's new code seems wrong, it changes the result for d.


Martin's suggestion:

    if (ro2)
        return true
    if (!ro1)
        return false
    return true

    a: ro1 true  + ro2 true  => true
    b: ro1 true  + ro2 false => true
    c: ro1 false + ro2 true  => true
    d: ro1 false + ro2 false => false

Martin's suggestion seems wrong, it changes the result for c.


Unless I'm mistaken, 
I'd prefer to fix Bob's version, and just fix the last line, which handles d:

    if (ro1)
        return true
    if (ro2)
        return false
    return true

Comment 8

[Kai Engert (:KaiE:)]

Attachment: incremental fix

Comment 9

[Robert Relyea]

So my patch is clearly wrong.

What we are trying to avoid the case where we have a readonly database (2) and we are asking for a read/write database(1).

so file if ((!ro1) && ro)

That's probably what we should code. The original was succeed if (ro1) || (!ro2) which loose right according to boolean algrebra (!A)*B = !(A | !B).

So Kai's fix is correct.
We could also have

if ((!ro1) && r0)
    return FALSE;
return TRUE;

Comment 10

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #9)
> 
> So Kai's fix is correct.

Thanks for following up quickly.

Landed: https://hg.mozilla.org/projects/nss/rev/e7bb7d872659

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8805116 [details] [diff] [review]
incremental fix

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

::: lib/pk11wrap/pk11pars.c
@@ +1147,5 @@
>       * and it's only open read only, we need to open it again */
>      if (isReadOnly1) {
>          return PR_TRUE;
>      }
>      if (isReadOnly2) { /* isReadonly1 == PR_FALSE */

This comment is probably the worst I've ever seen.  Would you mind nuking it?

Comment 12

[Robert Relyea]

Hmm. The point was that we don't need to check that isReadonly1 is false. 
When I wrote this part of the code, I was worried that we were loosing the actual condition for failure.

To me, reviewing the code later, I would think "shouldn't I check the value of read only here?" the comment was to fore stall that question.

Comment 13

[Robert Relyea]

It was in lui of actually writing

if (!readonly1 && readonly2)

Now that I think about it, it would probably be clearer to ax the 'if(readonly1)' and write:

if (!readonly1 && readonly2) {
    return PR_FALSE;
}

since that's the actual condition we are trying to trigger false on. The original code was opaque because it was the converse statement with the ! distributed. I didn't because you suggested breaking up the if, but I now think it was broken up too far.

Comment 14

[Franziskus Kiefer [:franziskus]]

This leaks

*** CID 1374320:  Resource leaks  (RESOURCE_LEAK)
/lib/pk11wrap/pk11pars.c: 1250 in secmod_GetSlotIDFromModuleSpec()
1244             goto done;
1245         }
1246
1247         /* find id of the token */
1248         for (thisChild = children, thisID = ids; thisChild && *thisChild; thisChild++,
1249             thisID++) {
>>>     CID 1374320:  Resource leaks  (RESOURCE_LEAK)
>>>     Overwriting "thisConfig" in "thisConfig = secmod_getConfigDir(*thisChild, &thisCertPrefix, &thisKeyPrefix, &thisReadOnly)" leaks the storage that "thisConfig" points to.
1250             thisConfig = secmod_getConfigDir(*thisChild, &thisCertPrefix,
1251                                              &thisKeyPrefix, &thisReadOnly);
1252             if (thisConfig == NULL) {
1253                 continue;
1254             }
1255             if (secmod_matchConfig(inConfig, thisConfig, inCertPrefix, thisCertPrefix,

Comment 15

[Franziskus Kiefer [:franziskus]]

Attachment: coverity-1374320.patch

This should fix the leak and a possible null deref.

Comment 16

[Robert Relyea]

Comment on attachment 8805475 [details] [diff] [review]
coverity-1374320.patch

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

r+ Franziskus patch is correct, but incomplete (there's still a leak path in a very unlikely corner case. The leak is in the original code).
This patch is fine, we just need another to fix the problem identified below.

bob

::: lib/pk11wrap/pk11pars.c
@@ +1265,4 @@
>      }
>  
>  done:
>      if (inConfig) {

OK, I looked at secmod_getConfDir to make sure that thisCertPrefix and thisKeyPrefix are set to NULL on in the error case. It turns out the are initialized to NULL, but it's possible for them to be set without thisConfig being set (same with inConfig). So these two if's should also go away. (the if (inConfig) and the thisConfig below).

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8805475 [details] [diff] [review]
coverity-1374320.patch

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

I think that Bob covered it.

Comment 18

[Franziskus Kiefer [:franziskus]]

> OK, I looked at secmod_getConfDir to make sure that thisCertPrefix and thisKeyPrefix are set to NULL on in the error case. It turns out the are initialized to NULL, but it's possible for them to be set without thisConfig being set (same with inConfig). So these two if's should also go away. (the if (inConfig) and the thisConfig below).

Thanks, that's a good catch. I removed the ifs and landed the fix as https://hg.mozilla.org/projects/nss/rev/bb3843a984e69a781bca6040b3293ef36f1815b0