Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Andreas Bartelt]

On 09/06/17 16:24, Bob Beck wrote:

effectivelyu providing a limitless OCSP staple is kind of stupid - you may
as well simply *not staple*



I guess a stapled response without the next_update field set would be 
treated as valid until the client considers this_update to be too old 
(for ocspcheck, this seems to be set to 14 days via MAXAGE_SEC). In the 
case of stapling, I agree that it typically would be much better to use 
a short period for next_update than not to provide it at all.


In my case, I didn't want to use ocspcheck specifically for storing OCSP 
responses for stapling but in order to check if my local OCSP responder 
is actually working (i.e., the out-of-band way). In the out-of-band 
case, clients could also check for freshness by using nonces.


During these kinds of tests, I've also noticed that ocspcheck currently 
only connects to HTTP and HTTPS over their well-known ports which seems 
to be fine for all public CAs but not necessarily for all local CAs with 
a corresponding OCSP daemon.


In case this lack of flexibility is intended in order to keep the tool 
simple, I'm also fine with it.

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Bob Beck]

effectivelyu providing a limitless OCSP staple is kind of stupid - you may
as well simply *not staple*

On Wed, Sep 6, 2017 at 8:23 AM, Bob Beck  wrote:

> I'm not super inclined to make this "flexible" unless we see this used int
> the wild, which I have not. We are more restrictive than
> OpenSSL in many areas.
>
> On Wed, Sep 6, 2017 at 1:31 AM, Andreas Bartelt  wrote:
>
>> On 09/06/17 04:40, Bob Beck wrote:
>>
>>> Andreas where are you seeing this as being a real issue - who is shipping
>>> out OCSP responses without a next update field?
>>>
>>>
>> I've noticed this while playing with a local CA and a corresponding OCSP
>> responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is
>> optional. If these arguments are not explicitly provided, the next update
>> field will not be set.
>>
>>
>>
>>>
>>> On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt 
>>> wrote:
>>>
>>> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
 always provides a warning and no staplefile is written out. According to
 RFC 6960, the nextUpdate field is optional. The following patch should
 handle this case more gracefully and include a suitable debug message
 only
 in case -vv is specified.

 OK?

 Index: src/usr.sbin/ocspcheck/ocspcheck.c
 ===
 RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
 retrieving revision 1.21
 diff -u -p -u -r1.21 ocspcheck.c
 --- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
   1.21
 +++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
 @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
   {
  ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL,
 *nextupd =
 NULL;
  const unsigned char **p = (const unsigned char **)
 -   int status, cert_status=0, crl_reason=0;
 +   int status, cert_status=0, crl_reason=0, next_update=0;
  time_t now, rev_t = -1, this_t, next_t;
  OCSP_RESPONSE *resp;
  OCSP_BASICRESP *bresp;
 @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
  return 0;
  }
  if ((next_t = parse_ocsp_time(nextupd)) == -1) {
 -   warnx("unable to parse next update time in OCSP reply");
 -   return 0;
 +   if (verbose >= 2)
 +   fprintf(stderr, "Optional timestamp for next
 update not included in OCSP reply\n");
  }
 +   else
 +   next_update = 1;

  /* Don't allow this update to precede next update */
 -   if (this_t >= next_t) {
 +   if (next_update == 1 && this_t >= next_t) {
  warnx("Invalid OCSP reply: this update >= next
 update");
  return 0;
  }
 @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
  /*
   * Check that next update is still valid
   */
 -   if (next_t < now - JITTER_SEC) {
 +   if (next_update == 1 && next_t < now - JITTER_SEC) {
  warnx("Invalid OCSP reply: reply has expired (%s)",
  ctime(_t));
  return 0;
 @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size

  vspew("OCSP response validated from %s\n", host);
  vspew("This Update: %s", ctime(_t));
 -   vspew("Next Update: %s", ctime(_t));
 +   if (next_update == 1)
 +   vspew("Next Update: %s", ctime(_t));
  return 1;
   }



>>>
>>
>

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Bob Beck]

I'm not super inclined to make this "flexible" unless we see this used int
the wild, which I have not. We are more restrictive than
OpenSSL in many areas.

On Wed, Sep 6, 2017 at 1:31 AM, Andreas Bartelt  wrote:

> On 09/06/17 04:40, Bob Beck wrote:
>
>> Andreas where are you seeing this as being a real issue - who is shipping
>> out OCSP responses without a next update field?
>>
>>
> I've noticed this while playing with a local CA and a corresponding OCSP
> responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is
> optional. If these arguments are not explicitly provided, the next update
> field will not be set.
>
>
>
>>
>> On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt  wrote:
>>
>> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
>>> always provides a warning and no staplefile is written out. According to
>>> RFC 6960, the nextUpdate field is optional. The following patch should
>>> handle this case more gracefully and include a suitable debug message
>>> only
>>> in case -vv is specified.
>>>
>>> OK?
>>>
>>> Index: src/usr.sbin/ocspcheck/ocspcheck.c
>>> ===
>>> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
>>> retrieving revision 1.21
>>> diff -u -p -u -r1.21 ocspcheck.c
>>> --- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
>>>   1.21
>>> +++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
>>> @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
>>>   {
>>>  ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd
>>> =
>>> NULL;
>>>  const unsigned char **p = (const unsigned char **)
>>> -   int status, cert_status=0, crl_reason=0;
>>> +   int status, cert_status=0, crl_reason=0, next_update=0;
>>>  time_t now, rev_t = -1, this_t, next_t;
>>>  OCSP_RESPONSE *resp;
>>>  OCSP_BASICRESP *bresp;
>>> @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
>>>  return 0;
>>>  }
>>>  if ((next_t = parse_ocsp_time(nextupd)) == -1) {
>>> -   warnx("unable to parse next update time in OCSP reply");
>>> -   return 0;
>>> +   if (verbose >= 2)
>>> +   fprintf(stderr, "Optional timestamp for next
>>> update not included in OCSP reply\n");
>>>  }
>>> +   else
>>> +   next_update = 1;
>>>
>>>  /* Don't allow this update to precede next update */
>>> -   if (this_t >= next_t) {
>>> +   if (next_update == 1 && this_t >= next_t) {
>>>  warnx("Invalid OCSP reply: this update >= next update");
>>>  return 0;
>>>  }
>>> @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
>>>  /*
>>>   * Check that next update is still valid
>>>   */
>>> -   if (next_t < now - JITTER_SEC) {
>>> +   if (next_update == 1 && next_t < now - JITTER_SEC) {
>>>  warnx("Invalid OCSP reply: reply has expired (%s)",
>>>  ctime(_t));
>>>  return 0;
>>> @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size
>>>
>>>  vspew("OCSP response validated from %s\n", host);
>>>  vspew("This Update: %s", ctime(_t));
>>> -   vspew("Next Update: %s", ctime(_t));
>>> +   if (next_update == 1)
>>> +   vspew("Next Update: %s", ctime(_t));
>>>  return 1;
>>>   }
>>>
>>>
>>>
>>
>

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Andreas Bartelt]

On 09/06/17 04:40, Bob Beck wrote:

Andreas where are you seeing this as being a real issue - who is shipping
out OCSP responses without a next update field?



I've noticed this while playing with a local CA and a corresponding OCSP 
responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is 
optional. If these arguments are not explicitly provided, the next 
update field will not be set.





On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt  wrote:


ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
always provides a warning and no staplefile is written out. According to
RFC 6960, the nextUpdate field is optional. The following patch should
handle this case more gracefully and include a suitable debug message only
in case -vv is specified.

OK?

Index: src/usr.sbin/ocspcheck/ocspcheck.c
===
RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 ocspcheck.c
--- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
  1.21
+++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
@@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
  {
 ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd =
NULL;
 const unsigned char **p = (const unsigned char **)
-   int status, cert_status=0, crl_reason=0;
+   int status, cert_status=0, crl_reason=0, next_update=0;
 time_t now, rev_t = -1, this_t, next_t;
 OCSP_RESPONSE *resp;
 OCSP_BASICRESP *bresp;
@@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
 return 0;
 }
 if ((next_t = parse_ocsp_time(nextupd)) == -1) {
-   warnx("unable to parse next update time in OCSP reply");
-   return 0;
+   if (verbose >= 2)
+   fprintf(stderr, "Optional timestamp for next
update not included in OCSP reply\n");
 }
+   else
+   next_update = 1;

 /* Don't allow this update to precede next update */
-   if (this_t >= next_t) {
+   if (next_update == 1 && this_t >= next_t) {
 warnx("Invalid OCSP reply: this update >= next update");
 return 0;
 }
@@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
 /*
  * Check that next update is still valid
  */
-   if (next_t < now - JITTER_SEC) {
+   if (next_update == 1 && next_t < now - JITTER_SEC) {
 warnx("Invalid OCSP reply: reply has expired (%s)",
 ctime(_t));
 return 0;
@@ -489,7 +491,8 @@ validate_response(char *buf, size_t size

 vspew("OCSP response validated from %s\n", host);
 vspew("This Update: %s", ctime(_t));
-   vspew("Next Update: %s", ctime(_t));
+   if (next_update == 1)
+   vspew("Next Update: %s", ctime(_t));
 return 1;
  }

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Andreas Bartelt]

On 09/06/17 04:40, Bob Beck wrote:

Andreas where are you seeing this as being a real issue - who is shipping
out OCSP responses without a next update field?



I've noticed this while playing with a local CA and a corresponding OCSP 
responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is 
optional. If these arguments are not explicitly provided, the next 
update field will not be set.





On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt  wrote:


ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
always provides a warning and no staplefile is written out. According to
RFC 6960, the nextUpdate field is optional. The following patch should
handle this case more gracefully and include a suitable debug message only
in case -vv is specified.

OK?

Index: src/usr.sbin/ocspcheck/ocspcheck.c
===
RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 ocspcheck.c
--- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
  1.21
+++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
@@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
  {
 ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd =
NULL;
 const unsigned char **p = (const unsigned char **)
-   int status, cert_status=0, crl_reason=0;
+   int status, cert_status=0, crl_reason=0, next_update=0;
 time_t now, rev_t = -1, this_t, next_t;
 OCSP_RESPONSE *resp;
 OCSP_BASICRESP *bresp;
@@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
 return 0;
 }
 if ((next_t = parse_ocsp_time(nextupd)) == -1) {
-   warnx("unable to parse next update time in OCSP reply");
-   return 0;
+   if (verbose >= 2)
+   fprintf(stderr, "Optional timestamp for next
update not included in OCSP reply\n");
 }
+   else
+   next_update = 1;

 /* Don't allow this update to precede next update */
-   if (this_t >= next_t) {
+   if (next_update == 1 && this_t >= next_t) {
 warnx("Invalid OCSP reply: this update >= next update");
 return 0;
 }
@@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
 /*
  * Check that next update is still valid
  */
-   if (next_t < now - JITTER_SEC) {
+   if (next_update == 1 && next_t < now - JITTER_SEC) {
 warnx("Invalid OCSP reply: reply has expired (%s)",
 ctime(_t));
 return 0;
@@ -489,7 +491,8 @@ validate_response(char *buf, size_t size

 vspew("OCSP response validated from %s\n", host);
 vspew("This Update: %s", ctime(_t));
-   vspew("Next Update: %s", ctime(_t));
+   if (next_update == 1)
+   vspew("Next Update: %s", ctime(_t));
 return 1;
  }

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Bob Beck]

Andreas where are you seeing this as being a real issue - who is shipping
out OCSP responses without a next update field?



On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt  wrote:

> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
> always provides a warning and no staplefile is written out. According to
> RFC 6960, the nextUpdate field is optional. The following patch should
> handle this case more gracefully and include a suitable debug message only
> in case -vv is specified.
>
> OK?
>
> Index: src/usr.sbin/ocspcheck/ocspcheck.c
> ===
> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 ocspcheck.c
> --- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
>  1.21
> +++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
> @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
>  {
> ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd =
> NULL;
> const unsigned char **p = (const unsigned char **)
> -   int status, cert_status=0, crl_reason=0;
> +   int status, cert_status=0, crl_reason=0, next_update=0;
> time_t now, rev_t = -1, this_t, next_t;
> OCSP_RESPONSE *resp;
> OCSP_BASICRESP *bresp;
> @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
> return 0;
> }
> if ((next_t = parse_ocsp_time(nextupd)) == -1) {
> -   warnx("unable to parse next update time in OCSP reply");
> -   return 0;
> +   if (verbose >= 2)
> +   fprintf(stderr, "Optional timestamp for next
> update not included in OCSP reply\n");
> }
> +   else
> +   next_update = 1;
>
> /* Don't allow this update to precede next update */
> -   if (this_t >= next_t) {
> +   if (next_update == 1 && this_t >= next_t) {
> warnx("Invalid OCSP reply: this update >= next update");
> return 0;
> }
> @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
> /*
>  * Check that next update is still valid
>  */
> -   if (next_t < now - JITTER_SEC) {
> +   if (next_update == 1 && next_t < now - JITTER_SEC) {
> warnx("Invalid OCSP reply: reply has expired (%s)",
> ctime(_t));
> return 0;
> @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size
>
> vspew("OCSP response validated from %s\n", host);
> vspew("This Update: %s", ctime(_t));
> -   vspew("Next Update: %s", ctime(_t));
> +   if (next_update == 1)
> +   vspew("Next Update: %s", ctime(_t));
> return 1;
>  }
>
>

[patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Andreas Bartelt]

ocspcheck effectively treats a missing nextUpdate like an error, i.e., 
it always provides a warning and no staplefile is written out. According 
to RFC 6960, the nextUpdate field is optional. The following patch 
should handle this case more gracefully and include a suitable debug 
message only in case -vv is specified.


OK?

Index: src/usr.sbin/ocspcheck/ocspcheck.c
===
RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 ocspcheck.c
--- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -   1.21
+++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
@@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
 {
ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd = NULL;
const unsigned char **p = (const unsigned char **)
-   int status, cert_status=0, crl_reason=0;
+   int status, cert_status=0, crl_reason=0, next_update=0;
time_t now, rev_t = -1, this_t, next_t;
OCSP_RESPONSE *resp;
OCSP_BASICRESP *bresp;
@@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
return 0;
}
if ((next_t = parse_ocsp_time(nextupd)) == -1) {
-   warnx("unable to parse next update time in OCSP reply");
-   return 0;
+   if (verbose >= 2)
+			fprintf(stderr, "Optional timestamp for next update not included in 
OCSP reply\n");

}
+   else
+   next_update = 1;

/* Don't allow this update to precede next update */
-   if (this_t >= next_t) {
+   if (next_update == 1 && this_t >= next_t) {
warnx("Invalid OCSP reply: this update >= next update");
return 0;
}
@@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
/*
 * Check that next update is still valid
 */
-   if (next_t < now - JITTER_SEC) {
+   if (next_update == 1 && next_t < now - JITTER_SEC) {
warnx("Invalid OCSP reply: reply has expired (%s)",
ctime(_t));
return 0;
@@ -489,7 +491,8 @@ validate_response(char *buf, size_t size

vspew("OCSP response validated from %s\n", host);
vspew("   This Update: %s", ctime(_t));
-   vspew("   Next Update: %s", ctime(_t));
+   if (next_update == 1)
+   vspew("   Next Update: %s", ctime(_t));
return 1;
 }