erlang / otp

Erlang/OTP
http://erlang.org
Apache License 2.0
11.34k stars 2.95k forks source link

PKCS-7 SignerInfo RFC compatibility #5989

Open nwalker opened 2 years ago

nwalker commented 2 years ago

Is your feature request related to a problem? Please describe. I recently implemented very basic PKCS-7 detached signature verification and I found SignerInfo and SignerInfoAuthenticatedAttributes definitions not compatible with message digesting as per RFC

To be honest, I did not start from reading RFC, I tried to translate Python implementation to Erlang.
I assumed that to verify SignerInfo I should DER-encode SignerInfo.authenticated_attributes and verify signature(SignerInfo.encryptedDigest) against result, but verification failed. Next step was to read RFC and I found that my understanding basically correct, but ASN.1 defininitions from RFC differ significantly from those in OTP sources. Those differences resulted in incorrect tag in DER representation, and one really dirty workaround resolved the issue.

Additional context Reduced version of my code, not sure if it is entirely correct, but I think it's fine as an example.

As far as I understand, my workaround is to replace [0] IMPLICIT SET OF in SignerInfoAuthenticatedAttributes with a SET OF.

  AASetDER = case public_key:der_encode('SignerInfoAuthenticatedAttributes', AuthenticatedAttributes) of
    <<16#A0:8, Rest/binary>> -> <<16#31:8, Rest/binary>>;
    Other -> Other
  end,

Relevant differences between RFC and OTP: RFC SignerInfo:

SignerInfo ::= SEQUENCE {
        version CMSVersion,
...
        signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL,
...
 }

SignedAttributes ::= SET SIZE (1..MAX) OF Attribute
Attribute ::= SEQUENCE { ... }

OTP SignerInfo at moment of writing

SignerInfo ::= SEQUENCE {
...
  version         INTEGER {siVer1(1), siVer2(2)},
...
  authenticatedAttributes SignerInfoAuthenticatedAttributes OPTIONAL, 
...
} (WITH COMPONENTS { ..., version (siVer1),
  authenticatedAttributes       (WITH COMPONENTS { ..., aaSequence ABSENT }),
  unauthenticatedAttributes     (WITH COMPONENTS { ..., uaSequence ABSENT })
} | WITH COMPONENTS { ..., version (siVer2),
  authenticatedAttributes       (WITH COMPONENTS { ..., aaSet ABSENT }),
  unauthenticatedAttributes     (WITH COMPONENTS { ..., uaSet ABSENT })
})

SignerInfoAuthenticatedAttributes ::= CHOICE {
    aaSet         [0] IMPLICIT SET OF AttributePKCS-7 {{Authenticated}},
    aaSequence    [2] EXPLICIT SEQUENCE OF AttributePKCS-7 {{Authenticated}}
    -- Explicit because easier to compute digest on sequence of attributes and then reuse
    -- encoded sequence in aaSequence.
}
AttributePKCS-7 ::= SEQUENCE OF {}

Describe the solution you'd like A clear and concise description of what you want to happen. - I have none because I do not understand why it is as it is.

Why this declaration differ so much from RFC? What is this siVer2? Maybe my google-fu is too weak, but I was unable to find any mentions of it. I found merge commit where these declaration was introduced, OTP-10509 in branch name looks like jira issue key, but internet knows nothing about it, it was some internal jira.

Is my approach the only possible, or there is some better way to implement it which it was unable to see? I understand that it would be much cleaner to copy SignedAttributes definition from RFC to my project and use asn1ct, but I wonder about built-in functionality.

Can someone sched some light on this?

Obviously, it would be really nice to have SignerInfo.authenticatedAttributes declared in a way which does not require workarounds like mine, or maybe even have fully RFC-compliant definition, but I wonder if it can be introduced in backwards-compatible way, and I'm sure there was reasons for current approach.

IngelaAndin commented 2 years ago

It is mostly legacy reasons. The ASN-1 spec used in the public_key application was adopted many years ago, and a special spec was written for certificates (before I started working for OTP, although the spec back then where part of the ssl application). Anyway, it is a bigger job to modernize the spec for later RFCs to make sure not to break backward compatibility or TLS. The TLS protocol does not use PKCS-7, but a simplified version of that spec was added for other reasons in OTP-R15 (if I remember correctly). The reason for this, as I remember it, is because it was not possible to add the latest version without updating the other PKCS-specs, as at least then it was very hard to find clean specs that would work well together and some specs did not even compile. I think we want to address this sooner or later but can not say when it can be prioritized.