66600 - Signtool prints incorrect & misleading error messages

Status: RESOLVED DUPLICATE of bug 177556

(NSS :: Tools, defect, P3)

Description

[Arshad Noor]

When the issuing CA's trust attributes are NOT set to trust the CA
for Object Signing, signtool -l prints an incorrect & misleading
error message for any certificate issued by this CA.

Reproducible: Always
Steps to Reproduce:
1.  Get an Object Signing certificate from somewhere (not self-signed)
2.  Using either certutil or the browser's security window, modify
    the trust attributes for the issuing CA to NOT trust the CA for
    anything.
3.  From the command line, type in signtool -l

Actual Results:  You will see output such as follows:

using certificate directory: /home/anoor/.netscape

Object signing certificates
---------------------------------------
Signtool 1.3 Testing's Sun Microsystems Inc ID
    Issued by: Sun Microsystems Inc TEST CA - Sun Microsystems Inc (Sun
Microsystems Inc TEST CA)
    Expires: Fri Jan 11, 2002
    ++ Error ++ THIS CERTIFICATE IS NOT VALID (Certificate Authority certificate
invalid)
    ++ Error ++ ISSUER CERT "Sun Microsystems Inc TEST CA - Sun Microsystems
Inc" IS NOT VALID (extension not found)
---------------------------------------
For a list including CA's, use "signtool -L"


Expected Results:  Hopefully something along the lines that the issuer is 
not trusted for Object Signing, instead of sending us down the wrong path, 
with "ISSUER CERT ... IS NOT VALID (extension not found)"

Changing the trust attributes to trust the CA for network sites or for
email users changes the error output message to:

using certificate directory: /home/anoor/.netscape

Object signing certificates
---------------------------------------
Signtool 1.3 Testing's Sun Microsystems Inc ID
    Issued by: Sun Microsystems Inc TEST CA - Sun Microsystems Inc (Sun
Microsystems Inc TEST CA)
    Expires: Fri Jan 11, 2002
    ++ Error ++ THIS CERTIFICATE IS NOT VALID (Certificate Authority certificate
invalid)
---------------------------------------
For a list including CA's, use "signtool -L"

Comment 1

[Keyser Sose]

Marking NEW.

Comment 2

[Ian McGreer]

marking signtool bugs as future until 3.3 plan is ready.

Comment 3

[Wan-Teh Chang]

Set Target Milestone to NSS 3.3.  Assigned the bug
to Bob for evaluation.

Comment 4

[Robert Relyea]

Do this only if it matched the PRD.

Comment 5

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 6

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 7

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 9

[Biswatosh Chakraborty]

I could reproduce it(the first case given in the description). Assigning myself this bug.

Comment 10

[Biswatosh Chakraborty]

We know that changing the trust in the issuing CA causes this error. 
Actually, the function cert_VerifyCertChain() is called which checks the trust of the issuer cert. It gets the Trust Flags and checks whether it has the required Flags. If it has, it comes out sucessfully. Otherwise, like in the case given in the bug desc.,
it checks whether the issuer cert is self signed. If it is, it returns error by
just stating that the issuer is untrusted. It does not tell it is untrusted for
what. The code for this is (in function cert_VerifyCertChain()):
**************
 if (issuerCert->isRoot) {
             PORT_SetError(SEC_ERROR_UNTRUSTED_ISSUER);
             LOG_ERROR(log, issuerCert, count+1, 0);
             goto loser;
 } 

*************

Now, there could be multiple ways to solve it. One solution could be,just to
inform the user the CertUsage for which these error messages are being given.
Now, in this function cert_VerifyCertChain(), CertUsage is given as an input
and we can always give a user friendly one line printf, describing in words
the meaning of the integer value certUsage. This could be possible by making
a switch statement based on certUsage in this function itself. The action would be to give printf statements.

Or, instead of the current PORT_SetError(SEC_ERROR_UNTRUSTED_ISSUER), we can
possibly say PORT_SetError(SEC_ERROR_UNTRUSTED_ISSUER_FOR_OBJ_SIGNING) or
PORT_SetError(SEC_ERROR_UNTRUSTED_ISSUER_SSLCA), depending upon the CertUsage
value. We have to add some new entries in secErrorString(long code) function in
signtool/util.c and also in secerr.h for that.

So, the fact that errors are being given is not wrong. signtool -l shows error when it should and does not show when it should not. The only concern is the content of the message, which could be misleading. 
 

 

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Biswatosh,  No printfs will go into cert_VerifyCertChain.

It is the responsibility of the function's caller (the application program)
to properly interpret the error code and provide any error messages to the 
user.  NSS shared libraries should NEVER undertake to speak to the user 
directly through any means, including printf.  All User Interface must be
supplied by the application program, not the shared libraries.

As you've already noted, an input argument to this function is the cert
usage for which the chain must be valid.  When the function returns 
UNTRUSTED_ISSUER, it means untrusted for the purpose that was passed 
into the function by the function's caller.  So separate error codes 
are not needed.  The caller knows what cert usage it passed to the verify
function, and when it gets that one error, it can provide adequate user
information based on the information it already has at hand.

That is why this bug is a TOOLS bug instead of a LIBRARIES bug.
The fix will be in the application program, not in the libraries.

Comment 12

[Biswatosh Chakraborty]

Change in 2 lines in nss/cmd/signtool/list.c seem to make the output
message clearer.
The changed output on running signtool -l is this:
***************************************
signtool -l -d db

using certificate directory: db

Object signing certificates
---------------------------------------
mytestcert
    Issued by: mytestcert (h)
    Expires: Sun May 20, 2007
    ++ Error ++ THIS CERTIFICATE IS NOT VALID FOR OBJECT SIGNING (untrusted issuer)
    ++ Error ++ ISSUER CERT "mytestcert" IS NOT A TRUSTED CA     (untrusted issuer)
************************************

The change is done in cert_trav_callback() function in list.c in the
PR_fprintf statements just after calling CERT_VerifyCertNow().
The cert is being verified for 2 cert usages, namely as certUsageObjectSigner
and as certUsageVerifyCA. And, in the PR_fprintf statement, the certusage
is mentioned now. I am attaching the patch. 

Comment 13

[Biswatosh Chakraborty]

Attachment: Not for review.

Details on this patch is in comment #12 .

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Biswatosh,  After cert verification fails, Signtool should report the 
specific error, the specific problem that explains what was wrong with 
the cert chain.  

When verification of the signing cert fails, your patch now reports the
cause as being that the cert was not qualfied for object signing, 
regardless of the real reason the verification failed.  it may be that 
the cert WOULD be valid for object signing, but the issuer is unknown
or untrusted.  In such a case, Signtool needs to tell the user the 
real reason that the verification failed.

CERT_VerifyCert & CERT_VerifyCertificate set an error code to indicate 
what error has occurred.  You can get it by calling PORT_GetError().  
You can use that error code to get an error string by calling SECU_Strerror
or you can use the special printf function that prints out the error string
in addition to other values you choose, SECU_PrintError().  You will find
many examples of the usage of these functions in cmd/lib/*

Comment 15

[Biswatosh Chakraborty]

Nelson,  As mentioned in comment #10 , the failure happens in 
function cert_VerifyCertChain() in nss/lib/certhigh/certvfy.c:
**************
 if (issuerCert->isRoot) {
             PORT_SetError(SEC_ERROR_UNTRUSTED_ISSUER);
             LOG_ERROR(log, issuerCert, count+1, 0);
             goto loser;
 } 

*************
Here, the error is being set to SEC_ERROR_UNTRUSTED_ISSUER
which is being retrived by cert_trav_callback() in cmd/signtool/list.c,
using secErrorString(), this way:
****************
rv = CERT_VerifyCertNow (cert->dbhandle, cert,PR_TRUE,
certUsageObjectSigner, NULL);
    
    if (rv != SECSuccess) {
	rv = PORT_GetError();
	PR_fprintf(outputFD,
	"    ++ Error ++ THIS CERTIFICATE IS NOT VALID (%s)\n",
				secErrorString(rv));            
    }

*******************************
And, this error code is translated to the string "untrusted issuer".
It is defined there in nss/cmd/signtool/util.c as:
secErrorString(long code)
{
    static char	errstring[80];
    char	*c;

    switch (code) {
    case SEC_ERROR_UNTRUSTED_ISSUER: 
	c = "untrusted issuer";
	break;
}

Now, one immediate question is, can't an issuer cert be a root?
But here, the code is returning with error just after it finds
that the issuer is root. Now, this is happening only when we make
the trust flags of the issuer cert "unfavourable" for Obj Signing.
It looks, to prevent infinite looping, it comes out just when it finds
the issuer as root. 
But, if the trust flag is supposed modified to "TCP,TCP,TCP" or something
favourable, the code in  cert_VerifyCertChain() does not even reach
to this point of verifying whether the issuer is root or not. It comes to
this point in cert_VerifyCertChain().
*************************
flags = SEC_GET_TRUST_FLAGS(issuerCert->trust, trustType);           
if (flags & CERTDB_VALID_CA) {
    if ( ( flags & requiredFlags ) == requiredFlags) {
       /* we found a trusted one, so return */
         rv = rvFinal; 
         goto done;
    }
    validCAOverride = PR_TRUE;
}
***************************************
And, upon finding that flags have the required flags, it goes to done.
and returns Success. If the code fails here, it then checks whether the
issuer is root and if root, it returns error.

So, if really the issuer can be a root, then the real reason for failure
is not clear from what the error code is being set to, that is as SEC_ERROR_UNTRUSTED_ISSUER.
The reason for failure is that the trust flags of the issuer does not have
the required flags and in the library, we are not setting the error code
to reflect that.  

Comment 16

[Biswatosh Chakraborty]

Attachment: A patch which adds some code only in signtool/list.c

This patch just adds a few lines in the callback function in list.c.
It has been observed that the errors SEC_ERROR_UNTRUSTED_ISSUER and SEC_ERROR_UNKNOWN_ISSUER occur only when the issuer exists in the db
but does not have sufficient trusts for object signing.
If the issuer is self-signed, we get
the first error(SEC_ERROR_UNTRUSTED_ISSUER) and if the issuer is not 
self signed then we get the second error. More details, as to exactly what
happens in CERT_VerifyCertChain, is in Comment #15.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

> It has been observed that the errors SEC_ERROR_UNTRUSTED_ISSUER and
> SEC_ERROR_UNKNOWN_ISSUER occur only when the issuer exists in the db
> but does not have sufficient trusts

Unknown issuer means the issuer cert cannot be found at all.
Untrusted issuer means that the cert can be found but is not trusted.

Comment 18

[Biswatosh Chakraborty]

Comment on attachment 266340 [details] [diff] [review]
A patch which adds some code only in signtool/list.c

Cancelling the request to review this patch.

Comment 19

[Biswatosh Chakraborty]

Attachment: Patch to give some extra information on the reported signtool error

The earlier patch 266340 is slightly modified in that this patch is not checking
the error SEC_ERROR_UNKNOWN_ISSUER. It now only checks for SEC_ERROR_UNTRUSTED_ISSUER.
If it gets this error, it has to do with the trust flags for object signing of the
issuer.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 266861 [details] [diff] [review]
Patch to give some extra information on the reported signtool error

The original report of this bug, in comment 0, reported that the 
error code was "(extension not found)".  This patch attempts to 
detect another error code, and to embellish its description.
But since it is not addressing the error code that was reported,
it doesn't address the problem that is the subject of this bug.

In order to make some real progress on this bug, it will be 
necessary to find the code path that sets the error code that is
reported as "(extension not found)", and determine what in the cert
causes that error code to be displayed.  It may be necessary 
(and would certainly be helpful) to reproduce the problem before
the proper diagnosis can be made.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Arshad,  
Some questions for you about the cert chain with which you experienced this
problem.  

1) Do you still have the cert, or a copy of any object or email message that
was succesfully signed with that cert?
2) What CA issued that cert?  Perhaps we can find other certs issued by 
that same CA.
3) Would any of your former coworkers at Sun still have that cert, or have
other certs that experienced the same failures?  If so, please email me 
information about them (like their names).  

Comment 22

[Biswatosh Chakraborty]

1) The first error (++ Error ++ THIS CERTIFICATE IS NOT VALID (Certificate Authority certificate invalid) can be reproduced.

2) The second error (  ++ Error ++ ISSUER CERT "Sun Microsystems Inc TEST CA - Sun Microsystems Inc" IS NOT VALID (extension not found)) seems to be irreproducable.
It was probably possible during the time of reporting but does not seem to be now.



Details on the above two points:
(1): How to reproduce the first error.
(a)Create a self signed cert which should be with Basic Constraints but not having obj signing CA as one of it's extns.
Eg:
 Signed Extensions:
            Name: Certificate Type
            Critical: True
            Data: <Object Signing,SSL CA,S/MIME CA>

            Name: Certificate Basic Constraints
            Critical: True
            Data: Is a CA with a maximum path length of 0.

(b)Then, let this issue another cert whose desc. is as follows:
 Signed Extensions:
            Name: Certificate Basic Constraints
            Critical: True
            Data: Is a CA with a maximum path length of 0.

            Name: Certificate Type
            Critical: True
            Data: <SSL Client,Object Signing>

(c)And, change the trusts of the issuer this way:
certutil -M -t ",," -n selfsigned -d dbnew
(d)And, now run signtool -l and this would produce this error code


(2)Why it seems that the second error cannot be reproduced?

This error "extension not found" can only happen when in the call
sequence, somewhere cert_FindExtensionByOID() is called. 
The only way that could have produced this error and this function
cert_FindExtensionByOID() getting called, is when
CERT_VerifyCertNow() is called in the callback function present in
signtool/list.c. 
The exact chain is:
CERT_VerifyCertNow(CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert, 
		   PR_TRUE, certUsageVerifyCA, NULL) 
calls
CERT_VerifyCert()
calls
CERT_VerifyCertChain()
calls
cert_VerifyCertChain()
calls
CERT_FindBasicConstraintExten(issuerCert, &basicConstraint).

Now, here when it calls CERT_FindBasicConstraintExten(), 
it does it this way(in /nss/lib/certhigh/certvfy.c):
******************
rv = CERT_FindBasicConstraintExten(issuerCert, &basicConstraint);
	if ( rv != SECSuccess ) {
	    if (PORT_GetError() != SEC_ERROR_EXTENSION_NOT_FOUND) {
		LOG_ERROR_OR_EXIT(log,issuerCert,count+1,0);
	    } 
	    pathLengthLimit = CERT_UNLIMITED_PATH_CONSTRAINT;
	    /* no basic constraints found, if we're fortezza, CA bit is already
	     * verified (isca = PR_TRUE). otherwise, we aren't (yet) a ca
	     * isca = PR_FALSE */
	    isca = isFortezzaV1;
	} else  {
 ********************************
And, we see that even if SEC_ERROR_EXTENSION_NOT_FOUND was recieved, it is not
being popped out. 
Thus, it looks, with this present codebase the second error is not possible
to be reproduced.

Comment 23

[Biswatosh Chakraborty]

When I said in my previous comment that "extension not found" can only happen when cert_FindExtensionByOID()
is called, I meant that only in our context. In our context, if at all SEC_ERROR_EXTENSION_NOT_FOUND is being the error set, it can be by calling 
cert_FindExtensionByOID() only although this error is set by other functions as well.

Comment 24

[Biswatosh Chakraborty]

The error  "++ Error ++ ISSUER CERT "Sun Microsystems Inc TEST CA - Sun Microsystems Inc" IS NOT VALID (extension not found)" is reproducable with the old NSS(NSS_3_2_1_RTM) for the following reason.
In signtool/list.c, in the callback function, we have this in that old NSS lib.
(http://ftp.mozilla.org/pub/mozilla.org/security/nss/releases/NSS_3_2_1_RTM/src/nss-3.2.1/mozilla/security/nss/cmd/signtool/list.c)

************************

if (rv == SECSuccess)
         {
          /*
         rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert,                    PR_TRUE, certUsageVerifyCA, NULL);
            */
         rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert,                    PR_TRUE, certUsageAnyCA, NULL);

         if (rv != SECSuccess)
           {
           rv = PORT_GetError();
           PR_fprintf(outputFD,
                "    ++ Error ++ ISSUER CERT \"%s\" IS NOT VALID (%s)\n", issuerCert->nickname, secErrorString(rv));
           }
         }

******************************
Whereas in the current NSS lib, we are not passing certUsageAnyCA to CERT_VerifyCertNow() but instead we are passing certUsageVerifyCA.
Now, that makes a big difference when CERT_VerifyCertNow() is called.
CERT_VerifyCertNow() calls CERT_VerifyCert() and inside CERT_VerifyCert(), we see this:
************************
...
CERT_GetCertType(cert);
    certType = cert->nsCertType;
    switch ( certUsage ) {
      case certUsageSSLClient:
      case certUsageSSLServer:
      case certUsageSSLServerWithStepUp:
      case certUsageSSLCA:
      case certUsageEmailSigner:
      case certUsageEmailRecipient:
      case certUsageObjectSigner:
      case certUsageStatusResponder:
        rv = CERT_KeyUsageAndTypeForCertUsage(certUsage, PR_FALSE,
                                              &requiredKeyUsage,
                                              &requiredCertType);
        if ( rv != SECSuccess ) {
            PORT_Assert(0);
            EXIT_IF_NOT_LOGGING(log);
            requiredKeyUsage = 0;
            requiredCertType = 0;
        }
        break;
      case certUsageVerifyCA:
        requiredKeyUsage = KU_KEY_CERT_SIGN;
       requiredCertType = NS_CERT_TYPE_CA;
        if ( ! ( certType & NS_CERT_TYPE_CA ) ) {
            certType |= NS_CERT_TYPE_CA;
        }
        break;
      default:
        PORT_Assert(0);
        EXIT_IF_NOT_LOGGING(log);
        requiredKeyUsage = 0;
        requiredCertType = 0;
    }

*******************************
Hence, in the old NSS, it always comes to the default section. But,
it already had called CERT_GetCertType(cert) and there if the extn
is not found, error is set to SEC_ERROR_EXTENSION_NOT_FOUND.
If the lib is optimised, we won't see an abort from PR_Assert(0) in
the default section.
And,lastly, the error "extension not found" will be output by the callback
function in signtool/list.c.

But, in the current lib, this won't happen because the default section(as
shown above) wont get executed. 

We can force this error in the current lib by modifying just one part
(mentioned before) in signtool/list.c. That is, we would do what was there
in the old NSS lib.
************************************
- rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert, 
-      PR_TRUE, certUsageVerifyCA, NULL);


+ rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert,
+      PR_TRUE, certUsageAnyCA, NULL);

      if (rv != SECSuccess)
         {
         rv = PORT_GetError();
         PR_fprintf(outputFD,
                "    ++ Error ++ ISSUER CERT \"%s\" IS NOT VALID (%s)\n", issuerCert->nickname, secErrorString(rv));
         }

*************************************
And, compile and run signtool -l. This error "extension not found"
can be produced now.

Comment 25

[Biswatosh Chakraborty]

I just tested with the old optimised NSS lib and it indeed gave "extension not found" error.
                                                                                                                             
export LD_LIBRARY_PATH=
"/share/builds/components/nss/deprecated/NSS_3_2_1_RTM/SunOS5.8_OPT.OBJ/lib/:/share/builds/components/nspr20/deprecated/v4.1/SunOS5.8_OPT.OBJ/lib"
                                                                                                                             
export PATH=
"/share/builds/components/nss/deprecated/NSS_3_2_1_RTM/SunOS5.8_OPT.OBJ/bin/:$PATH"
                                                                                                                             
bash-2.03# signtool -l -d db
using certificate directory: db
                                                                                                                             
Object signing certificates
---------------------------------------
ca3
    Issued by: ca2 (CA2)
    Expires: Sat Aug 04, 2007
    ++ Error ++ ISSUER CERT "ca2" IS NOT VALID (extension not found)
ca2
    Issued by: myissuer16April (My Issuer16April)
    Expires: Sat Aug 04, 2007
    ++ Error ++ ISSUER CERT "myissuer16April" IS NOT VALID (extension not found)


Then, changing the LD_LIBRARY_PATH and PATH to point to the current NSS,
and then adding the same certs as used with the old NSS to a new db,
signtool -l -d newdb did not produce the same error(extension not found).

Comment 26

[Biswatosh Chakraborty]

Two observations:

1)The change in the calling of the function CERT_VerifyCertNow() for the 
issuer cert in the callback function in signtool/list.c was done in
NSS_3_8_RTM(dated 08-Jan-2004). In the previous versions, it was
rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert,            
       PR_TRUE, certUsageAnyCA, NULL);

and from NSS_3_8_RTM onwards, it is:
rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert,            
       PR_TRUE, certUsageVerifyCA, NULL);


2)Looking at the callback function of signtool/list.c of the old NSS,
it appears
rv = CERT_VerifyCertNow (issuerCert->dbhandle, issuerCert, 
                    PR_TRUE, certUsageAnyCA, NULL);
will always return a failure because of the reason explained in comment #24.

Comment 27

[Biswatosh Chakraborty]

Found the bug which resolved this error(see Comment #26 , point 1 and comment #24 ).
The bug is https://bugzilla.mozilla.org/show_bug.cgi?id=177556.

Now, do we need to do any modification in the present NSS code? We see that "extension not found" error is not expected now.

Comment 28

[Biswatosh Chakraborty]

A correction for comment #22 ,point 1.
Create an issuer with Basic Constraints and it should not be CA, like:
****************
Signed Extensions:
            Name: Certificate Key Usage
            Critical: True
            Usages: Digital Signature
                    Certificate Signing

            Name: Certificate Basic Constraints
            Critical: True
            Data: Is not a CA.

            Name: Certificate Type
            Critical: True
            Data: <Object Signing,ObjectSigning CA>
********************

Let this issue another cert with some properties, such as:
*******************
Signed Extensions:
            Name: Certificate Basic Constraints
            Critical: True
            Data: Is a CA with a maximum path length of 0.

            Name: Certificate Type
            Critical: True
            Data: <Object Signing,ObjectSigning CA>
******************************
Now, run signtool -l and it will produce the error code(Certificate
Authority certificate invalid)

Comment 29

[Biswatosh Chakraborty]

Based on Comments #25 ,#26, #27, it seems the error message(extn not found) is not
reproducible now by changing the trusts of the certificate. Hence, closing the bug
as INVALID.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

This was fixed in NSS 3.8 by Ian McGreer for bug 177556.