485669 - CERT_DecodeAuthKeyID reports bogus errors and misses other errors

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

While working on Bug 485370, I found the vfyserv and vfychain programs
reporting bogus SEC_ERROR_INVALID_ARGS errors.  I tracked down the 
immediate cause, which was an error in CERT_DecodeAuthKeyID, and wrote 
a tiny patch to fix it.  Wan-Teh then reviewed CERT_DecodeAuthKeyID and
found other logic errors in it.  So, this bug exists to resolve those
errors.  

CERT_DecodeAuthKeyID exists to decode a cert's Authority Key ID (AKID) 
extension.  That extension has 3 components, all of which are declared
OPTIONAL in the ASN.1 syntax.  They are:
       Issuer cert's Issuer Name
       Issuer cert's Serial Number
       Issuer Cert's Subject Key ID value
Since they are all optional, it is not an ASN.1 SYNTAX error for any of
them to be missing.  It may be a semantic error, however.

The RFC's semantic rules require that Issuer Cert's Issuer Name and 
Issuer Cert's Serial number must both be present, or both be absent.
They are a pair.  The pair is either present, or it is absent.

The RFC also requires that one or both of the following must be present:
- The Issuer cert's Issuer Name and Serial Number pair of values,
- The Issuer cert's Subject Key ID value

So, Out of the 8 possible combinations of presence or absence of those 3 
optional components, there are only 3 valid combinations.  This table
summarizes them.

  Issuer's   Issuer's  Issuer's
  Issuer     Serial    Subject
  Name       Number    Key ID      Validity
  --------   --------  --------    -----------------------
   Absent    Absent    Absent      Invalid
   Absent    Absent    Present     Valid
   Absent    Present   Absent      Invalid
   Absent    Present   Present     Invalid   (but accepted)
   Present   Absent    Absent      Invalid
   Present   Absent    Present     Invalid   (but accepted)
   Present   Present   Absent      Valid
   Present   Present   Present     Valid
   
I believe it has been NSS's practice to treat an AKID value as valid if 
it bears an Issuer cert Subject Key ID value, regardless of whether it 
has a valid pair or invalid pair of Issuer cert's Issuer name and Serial
number.  We simply treat an invalid Issuer's Issuer name and serial number
as if the pair was entirely absent.  I think this is reasonable to continue.

To summarize the errors:

1. CERT_DecodeAuthKeyID calls cert_DecodeGeneralNames to decode the 
Issuer cert's Issuer Name, even if that component value is absent.
That causes bogus SEC_ERROR_INVALID_ARGS errors to sometimes be reported.

2. If the Issuer Cert's Issuer Name is present, but cannot be decoded, 
that error is ignored, and the value is treated as absent.
This MAY have been deliberate.  I suspect it was.

3. There is a block of code that cannot be reached if the Issuer cert's 
Issuer Name was absent our could not be decoded, but inside that block 
there are tests for the presence or absence of the decoded issuer cert's 
Issuer name.  Those tests are silly.  Wan-Teh noticed this.  

I wrote a patch for the first of those 3 issues, and attached it to 
bug 485370, where it received a positive review.  I will embellish it for
this bug, to also address issue number 3.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 for NSS trunk

I think this addresses Wan-Teh's observations, as well as mine.
Wan-Teh, do you agree?

Comment 2

[Wan-Teh Chang]

Comment on attachment 369802 [details] [diff] [review]
patch v2 for NSS trunk

>-	if ((value->authCertSerialNumber.data && !value->authCertIssuer) ||
>-	    (!value->authCertSerialNumber.data && value->authCertIssuer)){
>+	if (!value->authCertSerialNumber.data != !value->authCertIssuer) {
> 	    PORT_SetError (SEC_ERROR_EXTENSION_VALUE_INVALID);
>-	    break;
>+	    /* We set an error code for this, but do not report failure. */
> 	}

The new boolean expression is hard to understand.
I won't know what it means without the help of the
original boolean expression.

Re: error code: the common convention is that 'errno',
PR_GetError(), etc. is meaningless unless the function
call in question fails.  So in general it doesn't make
sense to set an error code when a function succeeds.
Why do we want to set SEC_ERROR_EXTENSION_VALUE_INVALID
but return successfully?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I agree that the most widely used convention in NSS and NSPR is that 
the error code from PR_GetError is only meaningful when an error status
is returned, e.g. SECFailure.  But there are some (few) functions that 
don't follow that exactly.  They set an error code that can be fetched,
and is meaningful even if they do not return SECFailure.  This function
appears to be one of those. 

Since this is a public function, I am trying to preserve the nuances of 
its behavior, just in case someone depends on it.  But I'm sure that 
getting a SEC_ERROR_INVALID_ARGS for every empty Issuer cert's Issuer Name
is not, and never was, a desired behavior.

Comment 4

[Wan-Teh Chang]

Comment on attachment 369802 [details] [diff] [review]
patch v2 for NSS trunk

I suggest two changes to this patch.

1. Assuming that we can't change the callers of
CERT_DecodeAuthKeyID to not check the error code when
CERT_DecodeAuthKeyID returns a non-NULL 'value', please
add a comment, before the declaration of
CERT_DecodeAuthKeyID in cert.h, to document that
the error code is meaningful when CERT_DecodeAuthKeyID
returns a non-NULL 'value'.

If a function behaves this way, shouldn't it call
PORT_SetError(0) at some point to ensure that it doesn't
use the error code set by an unrelated function?  For
CERT_DecodeAuthKeyID, the right place to call PORT_SetError(0)
is after the last place it sets 'rv' to SECSuccess:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/xauthkid.c&rev=1.8&mark=139#112

2. Your patch removes an "if (...) break" statement:

>-        value->authCertIssuer = cert_DecodeGeneralNames (arena, value->DERAuthCertIssuer);
>-	if (value->authCertIssuer == NULL)
>-	    break;

This "if (...) break" statement allows the original
code to ensure that the error code set by
cert_DecodeGeneralNames isn't replaced by
SEC_ERROR_EXTENSION_VALUE_INVALID subsequently:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/xauthkid.c&rev=1.8&mark=141-142,149#112

So your patch doesn't preserve that property of the
original code.  Is that intentional?  I would add that
"if (...) break" statement back, because DER decode
error is more useful than SEC_ERROR_EXTENSION_VALUE_INVALID.

Comment 5

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.