Closed Bug 833857 Opened 8 years ago Closed 8 years ago

fix ocspclnt crash regression

Categories

(NSS :: Libraries, defect, P1)

3.14.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.14.2

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(2 files, 3 obsolete files)

As explained in bug 811317 comment 28,
we crash in ocspclnt, because we've changed the value of an enum,
without adjusting ocspclnt.

I'll attach a fix using Wan-Teh's proposal from bug 811317 comment 29.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #705412 - Flags: review?(rrelyea)
Comment on attachment 705412 [details] [diff] [review]
Patch v1

Actually my intention was to ask Wan-Teh for review.
Attachment #705412 - Flags: review?(rrelyea) → review?(wtc)
Comment on attachment 705412 [details] [diff] [review]
Patch v1

Actually, this approach has the side effect that we would require additional changes in both ocspclnt and ssltap.

Because both have an array named "responseStatusNames" that has error strings referenced using the enum value as an index into the array.
Attachment #705412 - Attachment is obsolete: true
Attachment #705412 - Flags: review?(wtc)
Attached patch Patch v2 (obsolete) — Splinter Review
Can you please review this updated patch?
Attachment #705419 - Flags: review?(wtc)
Blocks: 360420
Comment on attachment 705419 [details] [diff] [review]
Patch v2

Kai:

Thank you for the bug report and sorry about causing the crash.
The crash is this assertion failure, right?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/ocspclnt/ocspclnt.c&rev=1.13&mark=856#835

After looking at this in more detail, I suggest the following.

1. This MXR query shows that no code assigns the ocspResponse_other value
to response->statusValue, and that ocspResponse_other is only used as the
maximum ocspResponseStatus value:

http://mxr.mozilla.org/security/ident?i=ocspResponse_other

So I suggest we first revert this change of mine from lib/certhigh/ocspti.h:

@@ -189,14 +189,14 @@
  * }
  */
 typedef enum {
+    ocspResponse_other = -1,		/* unknown/unrecognized value */
     ocspResponse_successful = 0,
     ocspResponse_malformedRequest = 1,
     ocspResponse_internalError = 2,
     ocspResponse_tryLater = 3,
     ocspResponse_unused = 4,
     ocspResponse_sigRequired = 5,
-    ocspResponse_unauthorized = 6,
-    ocspResponse_other			/* unknown/unrecognized value */
+    ocspResponse_unauthorized = 6
 } ocspResponseStatus;
 
 /*

This should fix the assertion failure and should be enough to consider
this bug fixed. Please make this change first.

2. This MXR query shows that we set response->statusValue in only one
place: http://mxr.mozilla.org/security/ident?i=statusValue

That place is here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/ocsp.c&rev=1.76&mark=2572#2571

We can add code to change |sv| to ocspResponse_other if |sv| < ocspResponse_other
or |sv| > ocspResponse_unauthorized.

Alternatively, we can remove ocspResponse_other and update the responseStatusNames
array and the assertion in ocspclnt.c. This will allow us to preserve the out-of-range
response->statusValue.

3. We can add new ocspResponseStatus enum values, ocspResponse_min and ocspResponse_max,
to make the range check easier:

 typedef enum {
     ocspResponse_successful = 0,
     ocspResponse_malformedRequest = 1,
     ocspResponse_internalError = 2,
     ocspResponse_tryLater = 3,
     ocspResponse_unused = 4,
     ocspResponse_sigRequired = 5,
     ocspResponse_unauthorized = 6,
     ocspResponse_other,			/* unknown/unrecognized value */
     ocspResponse_min = ocspResponse_successful,
     ocspResponse_max = ocspResponse_unauthorized
 } ocspResponseStatus;
Comment on attachment 705419 [details] [diff] [review]
Patch v2

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

::: security/nss/cmd/ocspclnt/ocspclnt.c
@@ +857,5 @@
> +	fprintf (out_file, "Response Status: %s\n",
> +		 responseStatusNames[response->statusValue]);
> +    else
> +	fprintf (out_file, "Response Status: %s\n",
> +		 "other (Status value out of defined range)");

In the "else" branch, we should print (int)response->statusValue
using the %d format.

::: security/nss/lib/certhigh/ocspti.h
@@ +201,1 @@
>  } ocspResponseStatus;

We need to add a comment before this enum typedef to remind people
to update the responseStatusNames array in ocspclnt.c if they add
a new enum value at the end.
This changes ocspResponse_other back to its original value
because some code in ocspclnt.c (the responseStatusNames
array and an assertion) depends on the value of ocspResponse_other.
Attachment #705486 - Flags: review?(kaie)
(In reply to Kai Engert (:kaie) from comment #3)
> Comment on attachment 705412 [details] [diff] [review]
> Patch v1
> 
> Actually, this approach has the side effect that we would require additional
> changes in both ocspclnt and ssltap.
> 
> Because both have an array named "responseStatusNames" that has error
> strings referenced using the enum value as an index into the array.

This MXR query shows the responseStatusNames string is only found in ocspclnt.c:
http://mxr.mozilla.org/security/search?string=responseStatusNames
Comment on attachment 705486 [details] [diff] [review]
Revert the change to ocspResponse_other in ocspti.h, rev. 1.9

> The crash is this assertion failure, right?

correct


> So I suggest we first revert this change of mine from lib/certhigh/ocspti.h:

agreed. r=kaie
Attachment #705486 - Flags: review?(kaie) → review+
(In reply to Wan-Teh Chang from comment #5)
> 
> We can add code to change |sv| to ocspResponse_other if |sv| <
> ocspResponse_other
> or |sv| > ocspResponse_unauthorized.

I prefer this approach.


> 3. We can add new ocspResponseStatus enum values, ocspResponse_min and
> ocspResponse_max,
> to make the range check easier:
> 
>  typedef enum {
>      ocspResponse_successful = 0,
>      ocspResponse_malformedRequest = 1,
>      ocspResponse_internalError = 2,
>      ocspResponse_tryLater = 3,
>      ocspResponse_unused = 4,
>      ocspResponse_sigRequired = 5,
>      ocspResponse_unauthorized = 6,
>      ocspResponse_other,			/* unknown/unrecognized value */
>      ocspResponse_min = ocspResponse_successful,
>      ocspResponse_max = ocspResponse_unauthorized
>  } ocspResponseStatus;


I find it confusing that max is smaller than the largest possible enum value.

I would prefer this style:

  typedef enum {
      ocspResponse_min = 0,
      ocspResponse_successful = 0,
      ocspResponse_malformedRequest = 1,
      ocspResponse_internalError = 2,
      ocspResponse_tryLater = 3,
      ocspResponse_unused = 4,
      ocspResponse_sigRequired = 5,
      ocspResponse_unauthorized = 6,
      ocspResponse_other,			/* unknown/unrecognized value */
      ocspResponse_max = ocspResponse_other     /* please keep updated */
  } ocspResponseStatus;
well, ok, now I get your point. I'll attach a patch shortly that I'll ask you to review.
Attached patch Patch v4 (obsolete) — Splinter Review
I now agree with the proposal to remove the "_other" enum value,
because, even if we updated the code the existing code to use this value, we can never be sure that future code might directly set the variable to a number received from the wire. Because of this uncertainity, it would be good to make our value-testing in ocspclnt failsafe anyway. And if we do that, we don't need the "_other" value.
Attachment #705419 - Attachment is obsolete: true
Attachment #705419 - Flags: review?(wtc)
Attachment #705558 - Flags: review?(wtc)
Comment on attachment 705486 [details] [diff] [review]
Revert the change to ocspResponse_other in ocspti.h, rev. 1.9

Patch checked in on the NSS trunk (NSS 3.14.2).

Checking in ocspti.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.10; previous revision: 1.9
done
Attachment #705486 - Flags: checked-in+
Comment on attachment 705558 [details] [diff] [review]
Patch v4

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

r=wtc. Thank you for the patch. I suggest some minor changes.
I'm glad that you agree ocspResponse_other should be removed.

::: security/nss/cmd/ocspclnt/ocspclnt.c
@@ +859,5 @@
> +		 responseStatusNames[response->statusValue]);
> +    else
> +	fprintf (out_file, "Response Status: %d - %s\n",
> +		 (int)response->statusValue,
> +		 "other (Status value out of defined range)");

Please add curly braces around the two fprintf statements. Use this style:

  if (... &&
      ...) {
      fprintf(...)
  } else {
      fprintf(...)
  }

We don't need to use %s to print the constant string
"other (Status value out of defined range)". Just say
        fprintf(out_file, "Response Status: %d - other (Status value out of defined range)\n",
                (int)response->statusValue);

or

        fprintf(out_file, "Response Status: other (Status value %d out of defined range)\n",
                (int)response->statusValue);

I prefer the latter slightly.

@@ +865,2 @@
>      fprintf (out_file, "Response Status: %s\n",
>  	     responseStatusNames[response->statusValue]);

You forgot to delete this fprintf statement :-)

::: security/nss/lib/certhigh/ocspti.h
@@ +196,5 @@
>      ocspResponse_tryLater = 3,
>      ocspResponse_unused = 4,
>      ocspResponse_sigRequired = 5,
>      ocspResponse_unauthorized = 6
> +    ocspResponse_max_valid = 6     /* please update when adding valid values */

Please remove _valid from these two enum names.

The comment also needs to remind people to update the responseStatusNames
array in ocspclnt.c.
Attachment #705558 - Flags: review?(wtc) → review+
Attached patch Patch v5Splinter Review
(In reply to Wan-Teh Chang from comment #14)
> 
> The comment also needs to remind people to update the responseStatusNames
> array in ocspclnt.c.

I'll use a general comment, because I'll shortly add that array to ssltap.c, too.

Thanks for the review. Attaching patch as checked in.

Checking in cmd/ocspclnt/ocspclnt.c;
/cvsroot/mozilla/security/nss/cmd/ocspclnt/ocspclnt.c,v  <--  ocspclnt.c
new revision: 1.14; previous revision: 1.13
done
Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.77; previous revision: 1.76
done
Checking in lib/certhigh/ocspti.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.11; previous revision: 1.10
done
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #705595 - Flags: review+
Attachment #705558 - Attachment is obsolete: true
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.