Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RC2CBC Parameter parsing. #337

Closed
wants to merge 1 commit into from
Closed

Conversation

alxbauer
Copy link

Parameters are encoded as Sequence. See: https://tools.ietf.org/html/rfc3370#section-5.2

jstedfast added a commit that referenced this pull request Sep 12, 2017
Thanks to Alexander Bauer for this patch.

Fixes issue #337
@jstedfast
Copy link
Owner

Thanks!

@jstedfast jstedfast closed this Sep 12, 2017
@jstedfast jstedfast added the bug Something isn't working label Sep 12, 2017
@jstedfast
Copy link
Owner

It seems that this fix creates a cast exception here:

var param = (DerSequence) identifier.Parameters;

identifier.Parameters is a DerInteger in my unit tests which is testing a message generated by Thunderbird.

Do you actually have any real-world messages where this is a DerSequence?

jstedfast added a commit that referenced this pull request Oct 21, 2017
It seems that PR #337 may be wrong as it causes a CastException
because identifier.Parameters is a DerInteger rather than a
DerSequence.

Restored my old logic for cases where identifier.Parameters
is not a DerSequence.
@12454j
Copy link

12454j commented Oct 21, 2017 via email

@jstedfast
Copy link
Owner

I'm not getting paid for this either.

As for why you, why are you getting this message? You aren't the original submitter for this pull request.

I was asking the original author of this pull request, not you.

@12454j
Copy link

12454j commented Oct 21, 2017

I am just trying to rekindle a old interest and make some money

@alxbauer
Copy link
Author

Yes i had some real-world messages. That's why i found the issue ;-)
I will check next week if i got a message for you.

If identifier.Parameter is a DerInteger in your message, then it must be wrong.
According to RFC 3370 identifier.Parameter must be a sequence:

5.2  RC2 CBC

   The RC2 algorithm is described in RFC 2268 [RC2].  The algorithm
   identifier for RC2 in CBC mode is:

      rc2-cbc OBJECT IDENTIFIER ::= { iso(1) member-body(2) us(840)
          rsadsi(113549) encryptionAlgorithm(3) 2 }

   The AlgorithmIdentifier parameters field MUST be present, and the
   parameters field MUST contain a RC2CBCParameter:

      RC2CBCParameter ::= SEQUENCE {
        rc2ParameterVersion INTEGER,
        iv OCTET STRING  }  -- exactly 8 octets

   The RC2 effective-key-bits (key size) greater than 32 and less than
   256 is encoded in the rc2ParameterVersion.  For the effective-key-
   bits of 40, 64, and 128, the rc2ParameterVersion values are 160, 120,
   and 58 respectively.  These values are not simply the RC2 key length.
   Note that the value 160 must be encoded as two octets (00 A0), since
   the one octet (A0) encoding represents a negative number.

@jstedfast
Copy link
Owner

Yea, I read that in the spec when you originally submitted the pull request which was one of the major reasons I figured the patch was correct :)

I was just wondering if maybe for whatever reason real-world usage (such as Thunderbird) were consistently using DerInteger instead.

I committed a fix yesterday that checks if the parameters are a DerInteger vs a DerSequence and handles them based on that. I gues that's how I'll have to handle this.

Anyway, knowing that you have messages that use DerSequence is nice to know. If you can get me a sample that I can use in the unit tests, that would be stellar :)

@12454j
Copy link

12454j commented Oct 22, 2017

I wish I could I don't have a computer to do this I am on my phone

@alxbauer
Copy link
Author

I committed a fix yesterday that checks if the parameters are a DerInteger vs a DerSequence and handles them based on that. I gues that's how I'll have to handle this.

That's perfect! Today i found both cases in real-word messages!
Maybe i found a reason for this.
In RFC section 4.3.2 the RC2 Key Wrap is specified, and in this case the parameter is only a DerInteger.

However. I've seen the RC2-Algorithm only in SmimeCapabilities for now.
I don't think anyone these days will use this algorithm to encrypt a message.

4.3.2  RC2 Key Wrap

   A CMS implementation MAY support mixed key-encryption and content-
   encryption algorithms.  For example, a 128-bit RC2 content-encryption
   key MAY be wrapped with a 168-bit Triple-DES key-encryption key.
   Similarly, a 40-bit RC2 content-encryption key MAY be wrapped with a
   128-bit RC2 key-encryption key.

   RC2 key encryption has the algorithm identifier:

      id-alg-CMSRC2wrap OBJECT IDENTIFIER ::= { iso(1) member-body(2)
          us(840) rsadsi(113549) pkcs(1) pkcs-9(9) smime(16) alg(3) 7 }

   The AlgorithmIdentifier parameter field MUST be RC2wrapParameter:

      RC2wrapParameter ::= RC2ParameterVersion

      RC2ParameterVersion ::= INTEGER

   The RC2 effective-key-bits (key size) greater than 32 and less than
   256 is encoded in the RC2ParameterVersion.  For the effective-key-
   bits of 40, 64, and 128, the rc2ParameterVersion values are 160, 120,
   and 58 respectively.  These values are not simply the RC2 key length.
   Note that the value 160 must be encoded as two octets (00 A0),
   because the one octet (A0) encoding represents a negative number.

   RC2 128-bit keys MUST be used as key-encryption keys, and they MUST
   be used with the RC2ParameterVersion parameter set to 58.

@jstedfast
Copy link
Owner

Aha! Thanks for this info :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants