169785 - CERT_AddOCSPAcceptableResponses prototype deficient

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug is the result of a review of functions being newly exported in 
NSS 3.6 that Wan-Teh asked me to perform.

Although it is not a new function, CERT_AddOCSPAcceptableResponses is being 
exported from NSS for the first time in NSS 3.6.  I believe its prototype 
should be fixed before 3.6 is released.

CERT_AddOCSPAcceptableResponses is declared as follows:

SECStatus 
CERT_AddOCSPAcceptableResponses(CERTOCSPRequest *request, ...)

The prototype specifies only one argument explicitly.  The code requires a
minimum of two arguments.  The first argument that is optional (may be omitted) 
is the third one.  The second and any subsequent arguments must all be of the 
same type, namely SECOidTag.  So, I think the proper declaration should be

SECStatus 
CERT_AddOCSPAcceptableResponses(CERTOCSPRequest *request, SECOidTag tag, ...)

I don't think any change to the executable code in the function would be 
required to implement that change to the prototype, but that assumption 
might not be portable (that is, true on all supported platforms).

Also, the code that traverses the variable list of arguments continues 
until it finds a SECOidTag == SEC_OID_PKIX_OCSP_BASIC_RESPONSE because the
OCSP standard requires one such tag in the list.  If the list contains no
such tag, the code would go past the end of the list of args.  I think the
code could be more robust in this regard.  Perhaps it should also look for
a zero SECOidTag.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Setting priority and target fix release per email from Wan-Teh

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Here are comments about 4 other OSCP functions being newly exported in NSS 3.6
that should, IMO, be fixed before 3.6 is released.  I'll just include them in 
this same bug report.

CERT_CreateOCSPCertID 
CERT_CreateOCSPRequest 
CERT_GetOCSPStatusForCertID and
CERT_VerifyCACertForUsage    

all have an "int64" argument whose value is really a PRTime value.  IMO,
the type of those arguments should be PRTime, not int64.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

OOps, one of those is obviously NOT an OCSP function.

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch

Note that I did not change 'int64' to 'PRTime' as Nelson
suggested in comment #2.  Although I agree that is a good
change, I found that many exported NSS functions are using
int64 for time.  It seems better to fix them all.  Otherwise
we will be left with inconsistency -- some functions use int64
while others use PRTime.

Comment 5

[Wan-Teh Chang]

Since we don't have any OCSP test program yet, I am
inviting your review of my patch.  Thanks.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This patch appears to be correct.  

Comment 7

[Wan-Teh Chang]

Patch checked in.

Comment 8

[Wan-Teh Chang]

I did not address another issue that Nelson pointed
out -- the code is not robust if the caller does not
terminate the optional argument list with a
SEC_OID_PKIX_OCSP_BASIC_RESPONSE.

I am not sure if looking for a zero SECOidTag is a
good solution.  (The enum constant for the zero
SECOidTag is SEC_OID_UNKNOWN, which looks strange
as an argument list teminator.)  Perhaps we should
change the function's prototype to take an int
argument "numResponseTypes".

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I think SEC_OID_UNKNOWN is perfect as an alternate list terminator.
We're searching the list until we come to either
a) an OID that is supposed to end the list, or 
b) an OID that is unknown and should never be in the list at all.

The enumerated type of OIDtags was specifically created so that zero would not
be a valid OIDtag, precisely so that code could detect zero values as invalid.

But this issue is lower priority than the other, because changing the code
to detect a zero element in the list has no effect on correctly written code.
and can be introduced after the function is exported without breaking any
backward compatibility with correctly written code.