rt icon indicating copy to clipboard operation
rt copied to clipboard

Add support for rfc822Name SAN in SMIME certificates

Open mkosmach opened this issue 8 years ago • 15 comments

replace #176 request

mkosmach avatar Jan 28 '16 15:01 mkosmach

Can anybody review my patch? I think that patch is needed in 4.4-(stable,trunk) branches too.

mkosmach avatar May 02 '17 07:05 mkosmach

SubjectAltName is undefined if there is no subjectaltname attribute in the certificate. In that case there is an error

Can't use an undefined value as an ARRAY reference at /usr/local/rt4/sbin/../lib/RT/Crypt/SMIME.pm line 902, <$fh> line 1.

I am not sure if Subject can ever be empty, but I guess it wouldn't hurt to check that is defined, too before using it and initializing data{String} to some placeholder string if the subject is empty. Then you should have covered all bases...

I also think it would be better to prefer the email address from the dn if it is set. subjectAltName means "subject alternative name", thus it's only an alternative if the dn has also an email address...

gvde avatar Jul 28 '17 13:07 gvde

AFAIR rfc822Name should be preferred way to store email. And yes, I should add some checks to patch.

mkosmach avatar Jul 28 '17 14:07 mkosmach

You are correct. RFC 5280 says in section 4.1.2.6:

Conforming implementations generating new certificates with electronic mail addresses MUST use the rfc822Name in the subject alternative name extension (Section 4.2.1.6) to describe such identities. Simultaneous inclusion of the emailAddress attribute in the subject distinguished name to support legacy implementations is deprecated but permitted.

subjectAltName is the preferred way to set the email address.

gvde avatar Jul 28 '17 15:07 gvde

Added fixes for certificates w/o rfc822name

mkosmach avatar Jul 28 '17 17:07 mkosmach

I have also noticed that the smime verification does not verify the sender address against the address in the certificate. That seems strange. If the email is signed by a certificate of a trusted ca, it's shown as good signature with full trust, even though it may actually a certificate by someone else not matching the sender address...

gvde avatar Jul 29 '17 13:07 gvde

https://tools.ietf.org/html/rfc5750#section-3 Receiving agents MUST check that the address in the From or Sender header of a mail message matches an Internet mail address, if present, in the signer's certificate, if mail addresses are present in the certificate. A receiving agent SHOULD provide some explicit alternate processing of the message if this comparison fails, which may be to display a message that shows the recipient the addresses in the certificate or other certificate details.

So yes, RT must check sender address and should provide alternative variant (certificate in user certificate store?) if check is failed. So please add issue for this problem at https://issues.bestpractical.com/

mkosmach avatar Jul 30 '17 09:07 mkosmach

I'm on board, in principal, with switching to openssl cms or openssl smime

@mkosmach commented on this pull request.

In lib/RT/Crypt/SMIME.pm:

@@ -883,8 +884,18 @@ sub GetCertificateInfo { my $method = $type . "" . $USER_MAP{$}; $data{$_} = $cert->$method if $cert->can($method); }

  •    $data{String} = Email::Address->new( @data{'Name', 'EmailAddress'} )->format
    
  •        if $data{EmailAddress};
    
  •    if ($type eq 'issuer') {
    
  •        $data{String} = 'DN: ' . join(',',@{$cert->Issuer});
    
  •    } elsif ($type eq 'subject') {
    
  •        if ($cert->SubjectAltName && grep /rfc822name/i, @{$cert->SubjectAltName}) {
    
  •            $data{String} = join( ',', map { s/rfc822name=//i; $_ } grep /rfc822name/i, @{$cert->SubjectAltName});
    

Hi, @alexmv. Glad to see You in this issue.

What Do You think about moving SMIME from openssl smime to openssl cms? Or using some of existing perl smime packages (Crypt::SMIME / Crypt::OpenSSL::SMIME / Crypt::SMIMEEngine/etc).

Now RT work with smime keys/messages on their own. And I think that native smime in openssl (now openssl cms ) can do this better. I think that openssl cms already can verify sender email, key status (crl/ocsp), CA chains, etc. So maybe we should switch to use native openssl functions instead of partially reimplement it in RT code?

― You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

sartak avatar Sep 05 '17 22:09 sartak

Would it still be possible to pull this? Currently, there are errors in the logs and the text shown is "broken" ('SMIME: The signature is good, signed by , trust is full') if the certificate has the only e-mail address set as subjectaltname.

See https://forum.bestpractical.com/t/smime-support-does-not-support-email-address-in-x509v3-extension/32145

Reimplementing the code may be a better long-term solution but for now (as this pull request is dated back to January 2016...) a quick fix would be much appreciated...

gvde avatar Sep 06 '17 05:09 gvde

I've succeeded in bending openssl ca to my whims; I'll push a few commits to document and make use of it tomorrow.

alexmv avatar Sep 06 '17 09:09 alexmv

Thank You very much, Alex! I'm sorry but I have no time now to integrate all suggestions in my PR or to make new PR with openssl cms inside. So I'll be glad if You make correct fix and push it to upstream.

PS. Please add checks for sender email == signer email as Gerald suggested earlier.

mkosmach avatar Sep 06 '17 11:09 mkosmach

Pushed 4.2/smime-subjectaltname with tests. Will follow up with the commits to resolve this issue atop those.

alexmv avatar Sep 07 '17 10:09 alexmv

@alexmv, thank You is this branch only for 4.2? It's needed for 4.4 too.

mkosmach avatar Sep 07 '17 10:09 mkosmach

Bugfixes are branched from the earliest supported release that the bug was found in. After it's merged into 4.2-trunk, 4.2-trunk will be merged up to 4.4-trunk, thus picking up the fix in 4.4. And 4.4-trunk is merged up into master.

alexmv avatar Sep 07 '17 11:09 alexmv

Now we have 4.4.4 and 2019 and this update still hasn't been merged. I can't help it but it seems to me RT is a dying product... For several years now I have to repatch our installation after each update.

gvde avatar Mar 18 '19 07:03 gvde