663733 - Better decoding of OCSP cert status, and OCSP code cleanup and "const" API changes

Status: RESOLVED INCOMPLETE

(NSS :: Libraries, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This will be used to test OCSP response handling in general, and OCSP stapling in particular. In particular, I am planning to modify selfserv to sign its own stapled responses for the OCSP stapling tests.

To start, I am going to try to modify the ASN.1 templates for OCSP responses so that choices are handled automatically by the ASN.1 encoder/decoder, instead of being hand-decoded as they currently are.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Decode more parts of OCSP responses using the ASN.1 template instead of code

This patch improves the ASN.1 template for OCSP responses so that more of the processing is handled automatically by the ASN.1 decoder. This should allow us to implement the encoder with less code.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add const modifier to declarations of functions used by ocspclnt and certhigh/ocsp.c

In my patch, I will be making more things const in ocsp.c and ocspti.h, and that will require changes to the declarations of these functions. It is a long patch, but it consists almost entirely of adding "const " in front of parameter declarations.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add const modifier to declarations of functions used by ocspclnt and certhigh/ocsp.c (v2)

More complete and without wrong ocspclnt.c bits.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: [BACKED OUT on 2011-11-16] Add const modifier to declarations of functions used by ocspclnt and certhigh/ocsp.c, make const warning an error on Windows (v3)

By adding -we4090 to the compile flags on WIN32, I was able to find some places that I didn't update with the correct const modifiers in the previous version of the patch. Also, found & fixed a few existing instances of wrong const-ness elsewhere (softoken, selfserv).

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Allow SEC_ASN1_SAVE | SEC_ASN1_SKIP | SEC_ASN1_INNER combination for SEC_QuickDERDecodeItem

When decoding a choice template, we can only have one template entry for each choice. When decoding a "byName" OCSP ResponderID, we need to save the inner part of the DER encoding and then skip it. This patch gives those results for the combination (SEC_ASN1_SAVE | SEC_ASN1_SKIP | SEC_ASN1_INNER). Without this patch, the SEC_ASN1_INNER flag will be ignored whenever either SEC_ASN1_SAVE or SEC_ASN1_SKIP is set.

Comment 6

[Konstantin Andreev]

(In reply to comment #4)
> Created attachment 539149 [details] [diff] [review]
> Add const modifier to declarations [...] update with the correct const
> modifiers [...] fixed a few existing instances of wrong const-ness [...]

Brian, sometimes I feel an irresistible craving to have not only SECItem to be constant, but it's content too. Something like that:

  typedef struct {
    ...
    CONST unsigned char *data;
    ...
  }
  SECCItem;

What do you think about adding such thing into the NSS ?

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: add const modifier to declarations of OCSP functions and data structures

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 539150 [details] [diff] [review]
Allow SEC_ASN1_SAVE | SEC_ASN1_SKIP | SEC_ASN1_INNER combination for SEC_QuickDERDecodeItem

This patch is not needed.

Comment 9

[Robert Relyea]

Comment on attachment 539646 [details] [diff] [review]
add const modifier to declarations of OCSP functions and data structures

r+ rrelyea

Comment 10

[Robert Relyea]

Comment on attachment 539149 [details] [diff] [review]
[BACKED OUT on 2011-11-16] Add const modifier to declarations of functions used by ocspclnt and certhigh/ocsp.c, make const warning an error on Windows (v3)

r+

In addition to const changes, this patch moves some global functions to local functions. These global functions are not exported in nss.def, so this is fine (as long as the patch compilies.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: add const modifier to declarations of OCSP functions and data structures

Comment 12

[Konstantin Andreev]

Brian, I have tried to investigate your changes in the OCSP code, but unable to do it, because patch 539080 (ocsp-use-template-for-decoding) conflicts with patches 539149 (nss-add-const-3) and 540344 (nss-ocsp-const-2). Could you, please, provide a single patch or consistent set of patches ?

By the way, I have noted the following defect, implying r-.

(In reply to comment #11)
> Created attachment 540344 [details] [diff] [review]

| static SECStatus
| ocsp_CopyRevokedInfo(PRArenaPool *arena, ocspCertStatus *dest, 
| ...
|   copy = (ocspRevokedInfo *) PORT_ArenaZAlloc ...
|   if (!dest->certStatusInfo.revokedInfo)     <<<<< loose unless !copy
| ...
|   if (src->revocationReason) {
|      copy->revocationReason = ...
|      if (!dest->certStatusInfo.revokedInfo->revocationReason) 
|
|           ^^^^^^^ <<<<< crash unless !copy->revocationReason

Comment 13

[Robert Relyea]

Comment on attachment 540344 [details] [diff] [review]
add const modifier to declarations of OCSP functions and data structures

r+ rrelyea

One issue I'd liked fixed: in ocsp_CopyCertStatus no longer sets the status of the dest->certStatusType cert unless the function succeed. I think we should at least default it to something safe (unknown or revoked). In actuality no one should be looking at 'dest' if we fail anyway, so I presume the code change is prudent paranoia. If we are going to be paranoid we should be completely paranoid;).

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #13)
> One issue I'd liked fixed: in ocsp_CopyCertStatus no longer sets the status
> of the dest->certStatusType cert unless the function succeed. I think we
> should at least default it to something safe (unknown or revoked). In
> actuality no one should be looking at 'dest' if we fail anyway, so I presume
> the code change is prudent paranoia. If we are going to be paranoid we
> should be completely paranoid;).

Thanks for catching this. It shouldn't be part of the patch. I will revert this change. I filed bug 672127 for the problem it was (partially) addressing.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

> -        dest->certStatusInfo.revokedInfo->revocationReason = 
> +        copy->revocationReason = 
>              SECITEM_ArenaDupItem(arena, src->revocationReason);
>          if (!dest->certStatusInfo.revokedInfo->revocationReason) {
>              goto loser;
>          }

This condition must become if (!copy->revocationReason). I will make this change before checking in.

Comment 16

[Robert Relyea]

Comment on attachment 539080 [details] [diff] [review]
Decode more parts of OCSP responses using the ASN.1 template instead of code

r+

I like this change. There are some areas that should be tightened up in a final patch:

1) I'd like a second pair of eyes on or a specific description on the changes to the quick decoder. I'm also curious if we may have a similar bug in the SEC_ASN1Decoder (which is supposed to work with exactly the same templates as the quick decoder).

2) changes ot ocsptl.h. Is it a public header? I don't really think these structures are part of the NSS public API. If they are, we have a bit of a problem, but I'm pretty confident that they aren't. We should just make sure.

3) I'm not so sure about the semantic of an unknown responder id. If we get an OCSP response with multiple responses in it, We may not want to fail if we get a response with a responderIDType that we don't recognize. We can't use that response, but if it's not for a cert we requested we shouldn't necessarily fail. I think the old code would continue one, but the new code I think fails in the decoder. Maybe that's the right semantic, but again we should convince ourselves of that fact.


bob

Comment 17

[Kai Engert [:KaiE:]]

I don't support for testing from the client side in your patches. I've filed bug 700701 (plus patch) to enhance tstclnt and ssltap.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 539149 [details] [diff] [review]
[BACKED OUT on 2011-11-16] Add const modifier to declarations of functions used by ocspclnt and certhigh/ocsp.c, make const warning an error on Windows (v3)

I needed this patch checked in for work on other bugs.

Checking in cmd/lib/pppolicy.c;
/cvsroot/mozilla/security/nss/cmd/lib/pppolicy.c,v  <--  pppolicy.c
new revision: 1.4; previous revision: 1.3
done
Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.111; previous revision: 1.110
done
Checking in cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.38; previous revision: 1.37
done
Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.97; previous revision: 1.96
done
Checking in cmd/signtool/verify.c;
/cvsroot/mozilla/security/nss/cmd/signtool/verify.c,v  <--  verify.c
new revision: 1.7; previous revision: 1.6
done
Checking in lib/certdb/alg1485.c;
/cvsroot/mozilla/security/nss/lib/certdb/alg1485.c,v  <--  alg1485.c
new revision: 1.42; previous revision: 1.41
done
Checking in lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.87; previous revision: 1.86
done
Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.118; previous revision: 1.117
done
Checking in lib/certdb/certv3.c;
/cvsroot/mozilla/security/nss/lib/certdb/certv3.c,v  <--  certv3.c
new revision: 1.11; previous revision: 1.10
done
Checking in lib/certdb/certxutl.c;
/cvsroot/mozilla/security/nss/lib/certdb/certxutl.c,v  <--  certxutl.c
new revision: 1.7; previous revision: 1.6
done
Checking in lib/certdb/certxutl.h;
/cvsroot/mozilla/security/nss/lib/certdb/certxutl.h,v  <--  certxutl.h
new revision: 1.3; previous revision: 1.2
done
Checking in lib/certdb/genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.40; previous revision: 1.39
done
Checking in lib/certdb/genname.h;
/cvsroot/mozilla/security/nss/lib/certdb/genname.h,v  <--  genname.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/certdb/polcyxtn.c;
/cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v  <--  polcyxtn.c
new revision: 1.12; previous revision: 1.11
done
Checking in lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.87; previous revision: 1.86
done
Checking in lib/certdb/xauthkid.c;
/cvsroot/mozilla/security/nss/lib/certdb/xauthkid.c,v  <--  xauthkid.c
new revision: 1.9; previous revision: 1.8
done
Checking in lib/certdb/xbsconst.c;
/cvsroot/mozilla/security/nss/lib/certdb/xbsconst.c,v  <--  xbsconst.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/certdb/xconst.c;
/cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v  <--  xconst.c
new revision: 1.14; previous revision: 1.13
done
Checking in lib/certhigh/xcrldist.c;
/cvsroot/mozilla/security/nss/lib/certhigh/xcrldist.c,v  <--  xcrldist.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/cryptohi/keyhi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keyhi.h,v  <--  keyhi.h
new revision: 1.19; previous revision: 1.18
done
Checking in lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.64; previous revision: 1.63
done
Checking in lib/nss/utilwrap.c;
/cvsroot/mozilla/security/nss/lib/nss/utilwrap.c,v  <--  utilwrap.c
new revision: 1.5; previous revision: 1.4
done
Checking in lib/pkcs7/p7decode.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v  <--  p7decode.c
new revision: 1.27; previous revision: 1.26
done
Checking in lib/pkcs7/secpkcs7.h;
/cvsroot/mozilla/security/nss/lib/pkcs7/secpkcs7.h,v  <--  secpkcs7.h
new revision: 1.7; previous revision: 1.6
done
Checking in lib/softoken/pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.57; previous revision: 1.56
done
Checking in lib/util/dersubr.c;
/cvsroot/mozilla/security/nss/lib/util/dersubr.c,v  <--  dersubr.c
new revision: 1.7; previous revision: 1.6
done
Checking in lib/util/secalgid.c;
/cvsroot/mozilla/security/nss/lib/util/secalgid.c,v  <--  secalgid.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/util/secder.h;
/cvsroot/mozilla/security/nss/lib/util/secder.h,v  <--  secder.h
new revision: 1.14; previous revision: 1.13
done
Checking in lib/util/secoid.h;
/cvsroot/mozilla/security/nss/lib/util/secoid.h,v  <--  secoid.h
new revision: 1.15; previous revision: 1.14
done

Comment 19

[Ismail Al-Jubbah]

I would like to contribute on this bug.

Comment 20

[Kai Engert [:KaiE:]]

(In reply to Ismial Al-Jubbah from comment #19)
> I would like to contribute on this bug.

That would be great! We are looking forward to it!

Comment 21

[Ismail Al-Jubbah]

(In reply to Kai Engert (:kaie) from comment #20)
> (In reply to Ismial Al-Jubbah from comment #19)
> > I would like to contribute on this bug.
> 
> That would be great! We are looking forward to it!

- Can this task be assigned to me ?
- And can I get more description like (Source code, Destination OS, OCSP client source code) etc.. ?

Comment 22

[Kai Engert [:KaiE:]]

Dear Ismial, have you been able to make any progress on this task? Thank you in advance for an update.

Comment 23

[Ismail Al-Jubbah]

I'm sorry...
I haven't been able to start working on this bug, because of some unexpected external conditions on my environment.
I still waiting a chance to get started to it...
Thank you

Comment 24

[Kai Engert [:KaiE:]]

In my opinion it's confusing that this bug has patches marked as r+ but they aren't checked in.

I understand Bob marked the patches in this bug as r+ but at the same time you have asked for changes. That's confusing.

Apparently Brian had concluded the patches weren't good enough and had to be backed out.

Because of that, I think we should change the r+ to r- on the patches.

Comment 25

[Kai Engert [:KaiE:]]

I believe this work doesn't strictly block OCSP stapling, removing dependency to bug 360420.

Also, so far, this bug was mostly focused on cleanup - and that cleanup work isn't a requirement to get the task done.

Therefore I'm changing the bug description, as I had announced to Brian on IRC.

I've filed new bug 811317 for the purpose of creating signed OCSP responses, and I have a patch in hand that I'll attach over there.