350200 - (dhmac) Implement DHMAC based POP (ProofOfPossession)

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Kai Engert [:KaiE:]]

We need to produce cert requests in the Mozilla clients.

Nelson proposed in bug 326159 comment 25, despite the Mozilla clients' lack of ECC signing ability, such cert requests should use an alternate mechanism of proofing possession of the private key (POP).

We would like to go with the mechanism explained in RFC 2511 Appendix A: DHMAC.

When the CRMF library got implemented, this portion was left out.

I will attach a patch that is meant to add the missing functionality to add the DHMAC piece to a cert request message.

I'm adding a function crmf_add_privkey_dhmac, but this is effectively a slightly adjusted clone of function crmf_add_privkey_thismessage.

(I'll shortly attach a patch to bug 326159 that makes use of this new code).

This is what the a derdump of the POP looks like:
      C-[3]  (23)
         [2]  (21)
            00 b9 68 5e 83 c3 9c 38 98 76 bc 7c 96 ba c3 51 37 13 38 cb
            6e

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 2

[Kai Engert [:KaiE:]]

Not sure whether target milestone should be 3.11.3 or 3.11.5 ?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 235424 [details] [diff] [review]
Patch v1

Uninvited review comment:   :)

> const SEC_ASN1Template CRMFDHMACTemplate[] = {
>-    { SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_XTRN | 0,
>+    { SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_XTRN | 2,
>       0,
>       SEC_ASN1_SUB(SEC_BitStringTemplate) },
>     { 0 }
> };
> 
> const SEC_ASN1Template CRMFPOPOKeyEnciphermentTemplate[] = {
>     { SEC_ASN1_EXPLICIT | SEC_ASN1_CONSTRUCTED | 
>       SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_XTRN | 2,
>       0,
>       SEC_ASN1_SUB(SEC_AnyTemplate) },

The above change creates two templates that are both "[2]"
that is, both context specific 2's.  They can't both be used
as (say) choices or optional members in the same Sequence.
Please double check this for correctness.

Comment 4

[Kai Engert [:KaiE:]]

(In reply to comment #3)
> 
> The above change creates two templates that are both "[2]"
> that is, both context specific 2's.  They can't both be used
> as (say) choices or optional members in the same Sequence.
> Please double check this for correctness.

Excerpt from RFC 2511 Section 4.4 Proof of Possession Syntax:

   ProofOfPossession ::= CHOICE {
       raVerified        [0] NULL,
       -- used if the RA has already verified that the requester is in
       -- possession of the private key
       signature         [1] POPOSigningKey,
E      keyEncipherment   [2] POPOPrivKey,
       keyAgreement      [3] POPOPrivKey }

   POPOPrivKey ::= CHOICE {
       thisMessage       [0] BIT STRING,
       -- posession is proven in this message (which contains the private
       -- key itself (encrypted for the CA))
       subsequentMessage [1] SubsequentMessage,
       -- possession will be proven in a subsequent message
D      dhMAC             [2] BIT STRING }
       -- for keyAgreement (only), possession is proven in this message
       -- (which contains a MAC (over the DER-encoded value of the
       -- certReq parameter in CertReqMsg, which must include both subject
       -- and publicKey) based on a key derived from the end entity's
       -- private DH key and the CA's public DH key);
       -- the dhMAC value MUST be calculated as per the directions given
       -- in Appendix A.

D lives one nesting level below E, does that mean it's ok?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Kai, Thanks for looking into it.
Yes, they're members of separate choices, so it's OK.
It would be nice if the code bore comments about that.

Comment 6

[Robert Relyea]

Comment on attachment 235424 [details] [diff] [review]
Patch v1

r+ = relyea

Comment 7

[Robert Relyea]

Comment on attachment 235424 [details] [diff] [review]
Patch v1

Kaie, you'll need a second review for this to go into the 3.11 branch. I've marked the request for nelson, but if he's too busy you can ask Julien or wan-teh.

bob

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 235424 [details] [diff] [review]
Patch v1

Wan-Teh, could you please do a second review on this patch? This patch is needed before we can land bug 326159.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: diff between existing -crmf_add_privkey_thismessage and new crmf_add_privkey_dhmac

Patch v1 adds a new function.
Note that most of that function is identical to an existing function.
I duplicated code and modified it.
I think it was appropriate, because according to my understanding, the spec specifies identical encoding rules for those choices.

I am attaching a diff between the existing function and the new function I'm adding. That should minimize the amount of code that needs to be reviewed.

Instead of code duplication, I can offer to parametrize and rename the existing function.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 235424 [details] [diff] [review]
Patch v1

I have 3 problems/concerns/issues with this patch.
1) it puts C++ style comments in a .c file.  On many platforms,
this will cause a syntax error.

2) This patch modifies function CRMF_CertReqMsgSetKeyAgreementPOP.
I wanted to look at the caller of that function to ensure that it
is properly DER encoding the key passed as encPrivKey, but I
could not find any caller of that function in the entire mozilla
source repository!  Is there a PSM patch that I need to review
along with this one?

3) I gather that the <<= 3 is to convert a byte length to a
bit length, and I presume that is because the data is to be
encoded as a bit string.  Now, I know that DER encoding rules
for bit strings require that the length not include any trailing
zero bits.  The last bit encoded (as indicated by the length)
must be a "1" bit.  I do not know if the ASN.1 encoder handles
that automatically, or if the caller must correctly compute the
number of bits to exclude all trailing zero bits.  So, this is a
review question that needs to be answered/resolved before this 
patch (or its successor) can get r+.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to comment #10)
> 1) it puts C++ style comments in a .c file.  On many platforms,
> this will cause a syntax error.

Sorry, I will fix this obvious mistake.


> 2) This patch modifies function CRMF_CertReqMsgSetKeyAgreementPOP.
> I wanted to look at the caller of that function to ensure that it
> is properly DER encoding the key passed as encPrivKey, but I
> could not find any caller of that function in the entire mozilla
> source repository!  Is there a PSM patch that I need to review
> along with this one?

The application code you are looking for is in bug 326159, the bug blocked by this one.
If you look at the most recent patch in bug 326159, you'll find the caller.

The key passed in as parameter encPrivKey is the result of a call to PK11_DigestFinal.
But inspired by your question I had a look myself, and noticed I directly pass in the result SECItem of that operation. I did not attempt to encode it as a bitstring myself, I blindly assumed this might happen automatically.

I looked at some other existing code, that encodes identical data types, the keyEncipherment choice combined with key escrow. In that case a zero length bit string is included (because the actual escrowed key is contained in another part of the message).

I saw that the initial author of that zero length bitstring encoding explicitly used a two-byte array containing {0x03, 0x00}. (You can look at that code in existing PSM function nsSetKeyEnciphermentPOP).

I figured I need to learn how to encode our array of 20 bytes SHA1 data as a bitstring, and got myself document X.690 which explains BER and DER encoding. I read portions of that document and now understand how encoding of a bit string works.

I believe I have now correctly implemented the bit string encoding. Because when using the existing code to produce a message with
 ProofOfPossession : keyEncipherment : thisMessage : zero length bit string
a dump of that message looks like this:

1105 A2    5:     [2] {
1107 80    3:       [0]
           :         00 03 00
           :       }
           :     }

When using my latest code (attached to bug 326159 in patch v8) I get this dump:

212 A3   26:     [3] {
 214 82   24:       [2]
            :         00 03 14 00 8E CD C5 4D D2 33 E7 7B B4 8C 74 6E
            :         41 D4 A4 45 81 87 83 98
            :       }
            :     }

Please feel free to have a look at the application code that encodes this message.
You'll find it in "bug 326159 comment 40" as Patch v8. Inside the patch search for
  "Encode our result item as a DER bit string"
which is the new encoding section, terminated by the call to
CRMF_CertReqMsgSetKeyAgreementPOP.

Comment 12

[Kai Engert [:KaiE:]]

> 3) I gather that the <<= 3 is to convert a byte length to a
> bit length, and I presume that is because the data is to be
> encoded as a bit string.  

Right (although it wasn't me who had added that <<= 3 for bit string encoding, I assume the initial author knew what he did).


> Now, I know that DER encoding rules
> for bit strings require that the length not include any trailing
> zero bits.  

Luckily the data we encode is a series of "full bytes" with no bits unused, as SHA1_LENGTH is exactly 20 bytes.


> The last bit encoded (as indicated by the length)
> must be a "1" bit.  

You are right, I just found section 11.2.2 in the X.690 document.


> I do not know if the ASN.1 encoder handles
> that automatically, or if the caller must correctly compute the
> number of bits to exclude all trailing zero bits. So, this is a
> review question that needs to be answered/resolved before this 
> patch (or its successor) can get r+.

We already have another, existing function in NSS that will add a bit string to the encoding. Function crmf_add_privkey_thismessage.

I can not yet answer the question, whether NSS' internal encoder already handles the trailing bits removal. I suspect it is not.

As this new code is not modifying the existing processing of bit strings passed in, I propose we define the following: If NSS' existing encoding of bit strings does not handle the trailing bits removal correctly, it shall be the responsibility of the application code to pass in bit strings that will work correctly - both with the existing functions, and the one I would like to add with this patch.


In patch v8 attached to bug 326159 I have not yet verified whether trailing zero bits are removed before encoding. I will test and if required will attach an improved application code patch to the PSM bug.

Comment 13

[Kai Engert [:KaiE:]]

(In reply to comment #10)
> Now, I know that DER encoding rules
> for bit strings require that the length not include any trailing
> zero bits.  

And now I finally understand the purpose of NSS internal function PrepareBitStringForEncoding. It tries to find the last bit set to 1.

I propose I adjust the comments to make it more obvious what this function does. Will do in the next patch to this message.


And now I can confirm that our existing CRMF library does not attempt to shorten the bit string.

I figure, the fact the existing code blindly multiplies the input length by 8 is not compatible with the idea to have the caller pass in the bit string with the correct number of bits. I will come up with a new proposal.

Comment 14

[Kai Engert [:KaiE:]]

After reading more details in the X.680 and X.690 documents, I would like to further clarify my understanding of bit string encoding.


Nelson, you said in comment 10:
> Now, I know that DER encoding rules
> for bit strings require that the length not include any trailing
> zero bits.

I can not confirm your understanding.

Document X.690 talks about unused bits in a bit string.
It requests that unused bits shall be set to zero.
That does not really apply to us, because we only have full bytes.

From my understanding of "X.690 11.2.2" and "X.680 21.7" I conclude: Only when using a "NamedBitList" the bit string shall have all trailing 0 bits removed.

I believe we are not using a NamedBitList.

I believe it is ok to include the trailing zero bits.

(And I believe this is reasonable, as it makes it easier for a decoding application to know what the original encoded set of bits was.)

Nelson, unless you disagree, I would like to keep any trailing 0 bits.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: Patch v2

@Bob and @Nelson

@Nelson, this patch fixes the // comment to use /* */

I also added a comment to the PrepareBitString function used by some other internal NSS code.


Assuming you agree to my previous comment (not removing trailing 0 bits), we have the following 3 options to call the encoding function - from my point of view.

a)
Add a new patch v9 to bug 326159, which is similar to v8, and fixes my small length calculation flaw.
The produced encoding will look like this:
212 A3   26:     [3] {
 214 82   24:       [2]
            :         00 03 *15* 00 8E CD C5 4D D2 33 E7 7B B4 8C 74 6E
            :         41 D4 A4 45 81 87 83 98
            :       }
            :     }

b)
Instead of using the 30 new lines of code I had added with Patch v8,
go back to use Patch v7, and in the asn.1 template for crmf dhmac we could add the SEC_ASN1_EXPLICIT flag. In my testing, this produced the same encoding that I listed in a)

c)
Both a) and b) encode the bit string "implicitly".
As a result tools like dumpasn1 do not know a bit string is to be expected in the current position, and we get that output listed in a) - it does not flag the raw bytes as a bit string.
According to Bob this implicit encoding is fine, as a decoding application will follow the choices and will be able to conclude the data must be a bit string.
I am listing this option c) just for the sake of completeness.
I found that by adding the SEC_ASN1_CONSTRUCTED flag to the asn.1 template for crmf dhmac, we get an encoding that is nicely dumped by the generic dumpasn1 tools as a bitstring, the output looks like this:
213 A3   25:     [3] {
 215 A2   23:       [2] {
 217 03   21:         BIT STRING 0 unused bits
            :           F0 38 B6 C1 78 9A B3 9E BE 9B EB DB A8 35 2E 84
            :           2D 48 7A 54
            :         }
            :       }


Nelson, if you have an opinion of what you prefer, please let me know.


(please do not compare the actual bytes between a and c, they are from different tests)

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #14)

> I believe we are not using a NamedBitList.
> I believe it is ok to include the trailing zero bits.

Kai, thanks for looking into this more deeply.  I agree with your analysis.  
I was deceived by a certain ASN.1 dumper tool that always complains about 
trailing zero bits.  I shall no longer be deceived by it.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Kai, WRT reviewing "patch v2", it would help me very much to have two things:

a) all the actual relevant ASN.1 definitions for what's being encoded, or 
URLs for them.  (Maybe everyting I need is in comment 4 above.) and 

b) the actual raw encoded data in hex (and/or what is proposed to be
encoded). 

The dumped output in comment 11 and comment 15 above doesn't show all the
data because some of it was consumed by the dumper program.  dumpasn1 has 
a command line option to dump the raw hex along with the decoded output.  
That would help, I think.

As for explicit vs implicit tagging, this is an area that's difficult to get
right.  I know there's a rule that says that, under certain circumstances, tags must be encoded as EXPLICIT, even if they are explicitly declared IMPLICIT. 
We've had to correct that a couple times before, but at the moment, I'm having
a difficult time finding the bugs or checkins or related email for those past
fixes.  

As a rule of thumb, if dumpasn1 cannot parse it, it's probably wrong. 

There is one place where ASN.1 DER encoded data is placed inside another 
DER encoded object, and the enclosing object is not marked constructed.
That is when an OCTET STRING "encapsulates" another ASN.1 object.  
The entire encoded ASN.1 object becomes the "value" of an OCTET STRING,
and the OCTET STRING is not marked constructed.  

But dumpasn1 even looks for that case.  So, when dumpasn1 looks at an
encoded object and does not decode an inner object, it is probably 
incorrect encoded.

Comment 18

[Kai Engert [:KaiE:]]

I will answer Nelson's questions in multiple comments.


> a) all the actual relevant ASN.1 definitions for what's being encoded, or 
> URLs for them.  (Maybe everyting I need is in comment 4 above.)

What is a ASN.1 definition? Do you refer to the definition in the RFC?

Yes, comment 4 lists everything that I'm trying to encode. It is a subset of some other encoding, but I propose we ignore the context.

If you would like to see the full definition, please look at http://www.ietf.org/rfc/rfc2511.txt section 4.4 Proof of Possession Syntax

When looking at the encoded/dumped data, what I'm trying to encode will be displayed at the end of the dump.

Or are you asking for ASN.1 templates?

All involved templates are in this file:
http://lxr.mozilla.org/seamonkey/source/security/nss/lib/crmf/crmftmpl.c

The new code calls
  CRMF_CertReqMsgSetKeyAgreementPOP
which calls
  crmf_add_privkey_dhmac
which uses
  CRMFPOPOKeyAgreementTemplate
and calls
  crmf_encode_popoprivkey
which uses
  CRMFDHMACTemplate

Comment 19

[Kai Engert [:KaiE:]]

> b) the actual raw encoded data in hex (and/or what is proposed to be
> encoded). 

We are intending to encode:
- choice 3 for ProofOfPossession
- choice 2 for POPOPrivKey
- 20 bytes of data (SHA1_LENGTH) as a bit string

The actual 20 bytes bit string will of course differ with each program execution, so I can't provide you with "stable" encodings, they will differ in those 20 bytes with every example that I will provide.


> The dumped output in comment 11 and comment 15 above doesn't show all the
> data because some of it was consumed by the dumper program.  dumpasn1 has 
> a command line option to dump the raw hex along with the decoded output.  
> That would help, I think.

For the future dumps I will enable the hex output.

Comment 20

[Kai Engert [:KaiE:]]

Before we continue with Nelson's other questions,
before we continue reviewing code,
I would like to gain some more clarity, and possibily we can first agree on what our encoded data should look like.

I'm not sure whether I understand the following right:

We already have existing code that encodes the POP-with-escrow case.
Following the definition listed in comment 4, it encodes:
- choice 2 for ProofOfPossession
- choice 0 for POPOPrivKey
- empty bit string (because the private key is already contained in 
  some other part of the request message)

When I look at the encoded data, produced by existing code, dumped with dumpasn1, I get this:

1105 A2    5:     [2] {
    <80 03 00 03 00>
1107 80    3:       [0]
            :         00 03 00
            :       }
            :     }

Here is my attempt to understand what we see here:
- a choice [2] followed by 5 bytes
- the encoder consumes (80, 03) as choice [0] and a remainder of 3 bytes
- it finally dumps the 3 remaining bytes 00 03 00

Here is my attempt to understand the meaning of 00 03 00:
- I have no idea what the initial 00 byte means
- the 03 is the universal id for the primitive bit string type
- the final 00 is the encoding of an empty bit string

Can you please help me out understanding this better?

Assuming the consuming application knows by context that 00 03 00 should be decoded as a bit string, is it a valid encoding?

Comment 21

[Kai Engert [:KaiE:]]

Once we have the answer to comment 20 we will know what the new encoding should look like.

As I said in comment 19, we are intending to encode
- choice 3 for ProofOfPossession
- choice 2 for POPOPrivKey
- 20 bytes of data (SHA1_LENGTH) as a bit string

I see multiple options:


Option (A)
(raw 20 bytes with a 00 unused bits prefix byte)

My initial code tried to match the existing encoding that I describe in comment 20, and the data looked like this:

212 A3   23:     [3] {
    <82 15 00 F9 82 0E D9 5A 29 D9 6B D7 39 B6 EC DD 9D 6A AC 42 F4 BE 38>
 214 82   21:       [2]
            :         00 F9 82 0E D9 5A 29 D9 6B D7 39 B6 EC DD 9D 6A
            :         AC 42 F4 BE 38
            :       }
            :     }


Option (B)
(20 bitstring bytes prefixed by id 03 len 15 unused count 00)

Use EXPLICIT in the template, and it will produce:

212 A3   25:     [3] {
    <82 17 03 15 00 E2 7C 09 B6 EC E1 4C 90 1E 6F 61 F3 DB 2C 09 15 C7 00 D4>
 214 82   23:       [2]
            :         03 15 00 E2 7C 09 B6 EC E1 4C 90 1E 6F 61 F3 DB
            :         2C 09 15 C7 00 D4 C8
            :       }
            :     }


Option (C)
(which is B prefixed with an additional 00 byte, matching comment 20)

I got this encoding, by using application code that calucates the "03 15 00" prefix out of the bitstring data. Not using explicit.

This is the best match when compared to the existing encoding described in comment 20. But I will be surprised if this is correct:

 212 A3   26:     [3] {
    <82 18 00 03 15 00 FB DF 36 ED 4C E9 09 C1 CD 60 BA 4C 07 B7 17 83 85 C6>
 214 82   24:       [2]
            :         00 03 15 00 FB DF 36 ED 4C E9 09 C1 CD 60 BA 4C
            :         07 B7 17 83 85 C6 40 89
            :       }
            :     }


Option (D)
(plain 20 bytes encoded with both EXPLICIT and CONSTRUCTED in the template)

This is the only case where the encoded data will actually be interpreted by dumpasn1 as a bitstring.

213 A3   25:     [3] {
    
 215 A2   23:       [2] {
    <03 15 00 45 71 F0 70 7A 50 8A 76 FE EF 0E BB 80 5D 16 3E 39 20 5F 04>
 217 03   21:         BIT STRING 0 unused bits
            :           45 71 F0 70 7A 50 8A 76 FE EF 0E BB 80 5D 16 3E
            :           39 20 5F 04
            :         }
            :       }
            :     }

Comment 22

[Kai Engert [:KaiE:]]

Nelson asked for full samples for all A, B, C, D, both my initial base64 data, and a full dump produced with a latest dumpasn1 and command line options -ahhlo

I made those files available at http://kuix.de/mozilla/ecc-crmf/350200-20060924/

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

RFC 2511 has been obsoleted by RFC 4211.

RFC 4211 declares the module default for context dependent tags to be IMPLICIT.

In section 4.2 (Key Encipherment keys), RFC 4211 declares that "this Message" 
and "dhMAC" are deprecated.  It adds two new alternatives to the POPOPrivKey CHOICE: agreeMAC and encryptedKey.

RFC 4211 deprecates thisMessage in favor of a new alternative, encryptedKey. 
RFC 4211 clarifies the definition of thisMessage from previous RFCs, and 
notes that some implementations got it wrong (maybe NSS?)

RFC 4211 deprecates dhMac in favor of a new alternative, agreeMAC, and cites 
RFC 2875 for the method of computing dhMAC.  (RFC 2511 cited its own appendix
A for the method of computing dhMAC.)

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

X.690 section 8.14 "Encoding of a Tagged Value" has a set of examples
which show the proper encoding of nested tagged types, incuding the 
various combinations of IMPLICIT and EXPLICIT nested tags.  

The example "Type5" is a tagged IMPLICIT type, where the TaggedType is
also a tagged IMPLICIT primitive type (VisibleString).  For those without
access to a copy, Type5 is:

Type1 ::= VisibleString
Type2 ::= [APPLICATION 3] IMPLICIT Type1
Type5 ::= [2] IMPLICIT Type2

and with a value of "Jones", Type5 is encoded as: 
 [2] Length  Contents
  82   05    4A 6F 6E 65 73

I believe this is exactly analgous to the situation of interest in this 
bug, where: 

ProofOfPosession choice keyAgreement is a tagged implicit POPOPrivKey and 
POPOPrivKey      choice dhMAC        is a tagged implicit BIT STRING.  

The example of Type5 makes it quite clear that the tag of the inner type
(Type2 in the example) is simply discarded.  The encoded tag is primitive 
because the innermost "base" type primitive.  

So, taking the example of Type5, I must conclude that NONE of the 
previously encoded Options A-D is correct.  Instead it appears that the 
correct encoding would look like:

    <83 15 00 F5 DE C3 CC 54 FB F3 F3 E6 A0 8C 75 83 1B 36 5A 4A 20 9C 8E>
 213   21:     [3]
         :         00 F5 DE C3 CC 54 FB F3 F3 E6 A0 8C 75 83 1B 36
         :         5A 4A 20 9C 8E
         :       }
         :     }

This seems counterintuitive and ambiguous, yet I find the example of Type5
to be quite clear that this is the intended encoding.  

So, this leads me to suspect that the writers of RFCs 2511 and 4311 didn't
specify what they really intended to specify.  I think we should write to 
them for clarification.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Oh, I should add that I think our ASN.1 encoder does not encode Type5 
as shown in X.690.  This is probably a bug in our encoder.  :(

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

I wonder what Julien, author of our DER decoder, has to say about this.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

X.680 section 30.6 says:

> The tagging construction specifies explicit tagging if any of the following 
> holds: 
>    [...]
>   c) the "Tag Type" alternative is used and the value of "TagDefault" for 
>      the module is IMPLICIT TAGS or AUTOMATIC TAGS, but the type defined 
>      by "Type" is an untagged choice type, an untagged open type, or
>      an untagged "DummyReference" (see ITU-T Rec. X.683 8.3).

This sounds very close to what we have here, but I think that POPOPrivKey 
is not an "untagged choice type", because all its choices are tagged.  
But maybe that is not what is meant by "an untagged choice type"?

Comment 28

[Kai Engert [:KaiE:]]

(In reply to comment #24)
> So, taking the example of Type5, I must conclude that NONE of the 
> previously encoded Options A-D is correct.  Instead it appears that the 
> correct encoding would look like:
> 
>     <83 15 00 F5 DE C3 CC 54 FB F3 F3 E6 A0 8C 75 83 1B 36 5A 4A 20 9C 8E>
>  213   21:     [3]
>          :         00 F5 DE C3 CC 54 FB F3 F3 E6 A0 8C 75 83 1B 36
>          :         5A 4A 20 9C 8E
>          :       }
>          :     }


I do not understand how this could work.

You are encoding choice [3], but you omit choice [2].

How is the decoding application supposed to know the bit string is meant as choice [0] or [2], which are both allowed? (as shown in comment 4)

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #28)

> I do not understand how this could work.
> You are encoding choice [3], but you omit choice [2].

Yes, just as the encoding of X.690's Type5 example omits the encoding of Type2.

> How is the decoding application supposed to know the bit string is meant as
> choice [0] or [2], which are both allowed? (as shown in comment 4)

That's a good question.  Personally, I think the ASN.1 definitions in RC 2511
and RFC 4311 are flawed because of this issue.  I have written to the author
of RFC 4311.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

In comment 24, I showed an excerpt from X.690 dated 2002 that defined the
encoding of nested tagged IMPLICIT types as dropping the tag and length
encodings of the inner tagged type, and using the outer tag with the length
and body octets of the "base" type.

I asked the authors of RFC 2511 about this, and one of them (David P Kemp)
replied (in part):

> However, the 1994 version of X.680 has a key difference from the 
> version you quote:
> 
>    28.6 The tagging construction specifies explicit tagging
>         if any of the following holds:
>          ...
>         c) the "Tag Type" alternative is used and the value of
>         "TagDefault" for the module is "IMPLICIT TAGS" or "AUTOMATIC
>         TAGS", but the type defined by "Type" is a choice type,
>         open type, or a "DummyReference" (see ... clause 8.3).
> 
> Notice that in 1994 there was no reference to an "untagged choice type". 

> Our intention was to rely on EXPLICIT tagging for these types,
> but given that that might not happen when following current versions
> of X.680, the ASN.1 should be changed in the manner you suggest:
> 
>> ProofOfPossession ::= CHOICE {
>>  raVerified        [0] NULL,
>>  signature         [1] EXPLICIT POPOSigningKey,
>>  keyEncipherment   [2] EXPLICIT POPOPrivKey,
>>  keyAgreement      [3] EXPLICIT POPOPrivKey }

Now I will re-review Kai's 4 options based on that most-helpful input.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Kai, I believe your option A from comment 21 conforms to the intended form
documented in comment 30.

Comment 32

[Kai Engert [:KaiE:]]

Nelson, do you propose we delay this until we get a helpful answer from the RFC authors?

If we are not able to derive reasonable conclusions from the RFCs, chances are, other's won't be able to either.

Could we agree on what seems most reasonable to us, and explain/document our interpretation of the RFC on a public site?

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Kai, I believe we've already gotten the answers.  
See comment 30 and comment 31.

Comment 34

[Kai Engert [:KaiE:]]

Attachment: Patch v1 - comment fixed

Nelson,

I'm glad to hear we are now certain that we want (A).

This is what I initially had implemented, before I started asking you for review, in Patch v1 implemented to this bug (and in Patch v7 attached to bug 326159).

This patch fixes the single problem I'm aware of - the C++ style comment.
I changed that to a /* */ C comment.

I am carrying forward r=rrelyea

Nelson, can you please review?


You had come other questions regarding this patch, which you expressed in your comment 10 in this bug.

Please ignore anything I said in comment 11. It is wrong. By your statement in comment 10, section 2), I got mislead and concluded I must somehow DER encode that bit string data. We now know this was wrong, so let's simply ignore that discussion path completely.

Should you still be interested to look at the caller of the new function, as you had asked in your comment 10 section 2), please find the caller in bug 326159 Patch v7.

I also appears that your review comment 10 section 3) has also been obsoleted.

I conclude, this patch is exactly what we want.

I'm looking forward to your r+. Thanks a lot in advance.

Comment 35

[Kai Engert [:KaiE:]]

Comment on attachment 240502 [details] [diff] [review]
Patch v1 - comment fixed

I double checked that this patch produces exactly what I described in comment 21 as Option A, also uploaded as sample files a.* as noted in comment 22.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 240502 [details] [diff] [review]
Patch v1 - comment fixed

r=nelson
Thanks for your patience while we sorted out the ASN.1 issues.

Comment 37

[Kai Engert [:KaiE:]]

Nelson, thanks a lot for working on this review.

Checked in to NSS trunk:
cvs commit: Examining .
Checking in crmfpop.c;
/cvsroot/mozilla/security/nss/lib/crmf/crmfpop.c,v  <--  crmfpop.c
new revision: 1.10; previous revision: 1.9
done
Checking in crmftmpl.c;
/cvsroot/mozilla/security/nss/lib/crmf/crmftmpl.c,v  <--  crmftmpl.c
new revision: 1.7; previous revision: 1.6
done

Checked in to NSS 3.11 branch:
cvs commit: Examining .
Checking in crmfpop.c;
/cvsroot/mozilla/security/nss/lib/crmf/crmfpop.c,v  <--  crmfpop.c
new revision: 1.3.28.5; previous revision: 1.3.28.4
done
Checking in crmftmpl.c;
/cvsroot/mozilla/security/nss/lib/crmf/crmftmpl.c,v  <--  crmftmpl.c
new revision: 1.6.28.1; previous revision: 1.6
done