If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

fix ocspclnt crash regression

RESOLVED FIXED in 3.14.2

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

3.14.1
3.14.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 705412 [details] [diff] [review]
Patch v1
Attachment #705412 - Flags: review?(rrelyea)
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
Created attachment 705419 [details] [diff] [review]
Patch v2

Can you please review this updated patch?
Attachment #705419 - Flags: review?(wtc)
(Assignee)

Updated

5 years ago
Blocks: 360420

Comment 5

5 years ago
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 6

5 years ago
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.

Comment 7

5 years ago
Created attachment 705486 [details] [diff] [review]
Revert the change to ocspResponse_other in ocspti.h, rev. 1.9

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)

Comment 8

5 years ago
(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
(Assignee)

Comment 9

5 years ago
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+
(Assignee)

Comment 10

5 years ago
(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;
(Assignee)

Comment 11

5 years ago
well, ok, now I get your point. I'll attach a patch shortly that I'll ask you to review.
(Assignee)

Comment 12

5 years ago
Created attachment 705558 [details] [diff] [review]
Patch v4

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 13

5 years ago
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 14

5 years ago
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+
(Assignee)

Comment 15

5 years ago
Created attachment 705595 [details] [diff] [review]
Patch v5

(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
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Attachment #705595 - Flags: review+

Updated

5 years ago
Attachment #705558 - Attachment is obsolete: true

Updated

5 years ago
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.