394182 - Eliminate pkix_pl_Pk11CertStore_CheckTrust or PKIX_PL_Cert_IsCertTrusted

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Kai Engert (:KaiE:)]

Spun off from bug 391454.
Nelson is surprised that we have two similar functions for trust checking in libPKIX.
He proposes we should have only a single one.

Right now we have:

static PKIX_Error *
pkix_pl_Pk11CertStore_CheckTrust(
        PKIX_CertStore *store,
        PKIX_PL_Cert *cert,
        PKIX_Boolean *pTrusted,
        void *plContext)

and

PKIX_Error *
PKIX_PL_Cert_IsCertTrusted(
        PKIX_PL_Cert *cert,
        PKIX_Boolean *pTrusted,
        void *plContext)


The former seems to be a static helper function.

The former has an additional PKIX_CertStore parameter, but it appears, besides a null check and an "enter/return" call, the function does not use it.


Should pkix_pl_Pk11CertStore_CheckTrust be converted to be a wrapper and call PKIX_PL_Cert_IsCertTrusted ?

Comment 1

[Kai Engert (:KaiE:)]

In bug 391454, Nelson proposed that we should use CERT_TrustFlagsForCACertUsage to learn about the required trust flag to check against and he proposed to add a parameter.

Now I'm learning that PKIX_PL_Cert_IsCertTrusted is already calling CERT_TrustFlagsForCACertUsage, and it obtains the usage from the plContext parameter.

However, it always does a hard cast from void* to (PKIX_PL_NssContext*).
This worries me.
I have no idea whether this cast is always valid.
I guess, the cast might sometimes be wrong, because the context was typed as void* and could be anything else???

Or can we assume, all callers of both functions will always provide a callback of this type?

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Nelson or Alexei, can you please review?

I'm still lacking most information about the data structures used by libPKIX, so I'm pretty sure you will tell me this patch is a hack.

This patch is a naive attempt to wrap the existing, better trust function and "stick in" the additional certstore argument. The PKIX_PL_Cert_IsCertTrusted accesses the store using cert->store.

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 278790 [details] [diff] [review]
Patch v1

Whoops, this doesn't work, it introduces a recursion.

PKIX_PL_Cert_IsCertTrusted calls into pkix_pl_Pk11CertStore_CheckTrust using a trustCallback function pointer.

Comment 4

[Kai Engert (:KaiE:)]

Let me rename the two functions for discussion, for mental sanity:

MAIN = PKIX_PL_Cert_IsCertTrusted

SUB  = pkix_pl_Pk11CertStore_CheckTrust


MAIN seems to be using a "two phase trust checking".

First, MAIN asks:

  "is there a reason to abort early and assume not trusted"?

In order to answer this question, it calls a callback, which is SUB.

SUB does that "incorrect check using incorrect trust flags".

Because of that, SUB has the chance to return a "false positive". But I think, SUB has no chance of a "false negative".

As a result, in some scenarios, MAIN will not "abort early", even though it would have been able to, if SUB did better.

I think, this "sometimes we don't abort early" is not causing harm, because MAIN will not assume the results of SUB as authorative.

MAIN will call CERT_TrustFlagsForCACertUsage and CERT_GetCertTrust and do a flag comparison.


But I do see a single bug in this code.

MAIN might incorrectly use the "false positive" result from SUB, if MAIN gets a failure when attempting to CERT_GetCertTrust.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The answer presently produced by pkix_pl_Pk11CertStore_CheckTrust is 
fiction.  I'd rather see it output FALSE.  It should fail safe.
False positives are intolerable.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I think it's probably OK to remove the call to pkix_pl_Pk11CertStore_CheckTrust
from PKIX_PL_Cert_IsCertTrusted, since pkix_pl_Pk11CertStore_CheckTrust is 
incapable of adding any correct information to the procedure.  
PKIX_PL_Cert_IsCertTrusted already has code to make the right computations 
without it.

Julien, what do you think of that?

Comment 7

[Kai Engert (:KaiE:)]

What are we learning from all of this?

It seems that both functions are required.
Well, at least the current design requires both functions.

I guess this bug should be marked INVALID then.

The initial problem we wanted to fix is: 

  Make sure pkix_pl_Pk11CertStore_CheckTrust uses the right flags.


I'm just learning from Alexei that he already filed bug 391595 for that problem.

Comment 8

[Kai Engert (:KaiE:)]

In reply to comment 6, I have worked on a patch that clones the code from the good function into the bad function. I will attach that patch to bug 391595 now. Just in case you want it...

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

There is no point in having MAIN and SUB contain the same code 
if MAIN calls SUB.  The test should be done once, not twice.  

Comment 10

[Kai Engert (:KaiE:)]

Nelson, I agree with your statement.

But first, I think we are waiting for an answer from Julien, whether the call can be eliminated. Let's see what he responds. Maybe it's best if we move that discussion into this bug.

And second, I'm not sure whether who is supposed to call that "trust callback that belongs to the cert store". I don't see the full picture here. Is SUB designed to have additional callers, in addition from being called from MAIN? Then we would have to duplicate that check? Or move it to a common function...

Comment 11

[Kai Engert (:KaiE:)]

Please let's move the discussion of "which of MAIN and SUB should do what" over to bug 391595, because Alexei made some comments there and I have responded.

I think this bug should be resolved INVALID, because we need both functions (although I agree one could potentially be a wrapper to the other, or both could call a new common function to do the real work).

Comment 12

[Julien Pierre]

Kai, Nelson,

Sorry it took me a while to respond. I have been behind on my bugmail.

The plContext argument is supposed to be specific to each implementation of libpkix, which can be done with different libraries. That's why it is defined as a void* . PKIX_PL_NssContext was the structure type chosen for the information needed by the NSS implementation of PKIX_PL. The cast should be safe by definition.

The structure contains information that needs to be passed in order for PKIX_PL functions to operate, but which is not normally passed through arguments to the platform-independent libpkix API.

The structure is currently defined as :

struct PKIX_PL_NssContextStruct {
        SECCertificateUsage certificateUsage;
        PRArenaPool *arena;
        void *wincx;
};

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

IMO, the right way to code this is NOT with a void *, but rather with an
opaque structure type.  Let each implementation define the opaque struct
in its own way, but let them all share the opaque definition.  This avoids
all sorts of type errors due to casting the wrong type.  And it makes it 
easier for those reading the code to find the correct structure declaration.

Comment 14

[Julien Pierre]

Nelson, that could be confusing too, since it would lead to multiple different structures with the same name for each PL implementation. And if a single program ever wanted to use more than one of them, the name conflict would prevent it.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Julien, how many different porting layer (PL) implemetnations can there be 
within the space of a single program (and more to the point, within NSS)?

Comment 16

[Julien Pierre]

As many as the author of the program needs. For example one could write a QA test to check the results of calling libpkix with one new PL layer in development against another pre-existing PL layer known to be good. This is just a theory. I don't think the void* is a major deficiency in the API here anyway. It would be nicer if it was strongly typed, but C doesn't really have nice things like function templates which would be needed here. But we could use macros maybe to have something other than void*.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Julien, 
Given that libPKIX is private to NSS, I think the answer to my question is
"as many as NSS implements", and that number today is 1.  

Comment 18

[Julien Pierre]

Well, the idea of the project was to preserve the pkix layer and keep it independent of the PL layer as much as possible. So even if NSS itself doesn't expose the libpkix API, some other project could still pick it up. This is mainly theoritical at this point, but we shouldn't close the door to that possibility.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Julien, you wrote "if a single program ever wanted to use more than one of 
them, the name conflict would prevent it.".  The conflict would exist only
for the scope of a .c file, not the whole program.  Presumably the struct
declaration is private to the implementation.  It is not shared with the 
common libPKIX code (libPKIX code itself would not include a header that 
declared the structure) nor with the program sources (e.g. sources of the
executable that calls the library).  Eliminating this void * need not reduce
the portability of the program at all.  It would solve the problem Kai 
encountered.  It closes no doors.

Comment 20

[Julien Pierre]

Nelson,

Code that calls libpkix directly needs to know about the content of the plcontext structure. Code that calls it through a wrapper, like the ones we are writing in NSS, does not.

Comment 21

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

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 22

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.