< Previous by Date	Date Index	Next by Date >
< Previous in Thread	Thread Index

Re: [reSIProcate] Proposed changes to support optional RFC 5922 TLS Certificate matching rules

From: Aron Rosenberg <arosenberg@xxxxxxxxxxxx>
Date: Sun, 13 Mar 2011 15:05:32 -0700

Based on Scott's feedback, here is a revised patch. It changes the setting name and moves the setting into the BaseSecurity class, which is probably a better place for the set function since users have to create a Security object to use TLS anyway.

-Aron

Index: resip/stack/ssl/Security.cxx

===================================================================

--- resip/stack/ssl/Security.cxx (revision 9050)

+++ resip/stack/ssl/Security.cxx (working copy)

@@ -171,6 +171,8 @@

}

+// .amr. RFC 5922 mandates exact match only on certificates, so this is the default, but RFC 2459 and RFC 3261 don't prevent wildcards, so disable if you want that mode.

+bool BaseSecurity::mAllowWildcardCertificates = false;

BaseSecurity::CipherList BaseSecurity::ExportableSuite("!SSLv2:aRSA+AES:aDSS+AES:@STRENGTH:aRSA+3DES:aDSS+3DES:aRSA+RC4+MEDIUM:aDSS+RC4+MEDIUM:aRSA+DES:aDSS+DES:aRSA+RC4:aDSS+RC4");

BaseSecurity::CipherList BaseSecurity::StrongestSuite("!SSLv2:aRSA+AES:aDSS+AES:@STRENGTH:aRSA+3DES:aDSS+3DES");

@@ -2434,12 +2436,22 @@

return Data::Empty;

}

/**

- Matchtes subjectAltName and cnames

- @todo looks incomplete, make better

+ Applies the certificate and domain name matching rules

int

BaseSecurity::matchHostName(const Data& certificateName, const Data& domainName)

{

+ if(mAllowWildcardCertificates)

+ return matchHostNameWithWildcards(certificateName,domainName);

+ return isEqualNoCase(certificateName,domainName);

+/**

+ Does a wildcard match on domain and certificate name

+ @todo looks incomplete, make better

+*/

+int

+BaseSecurity::matchHostNameWithWildcards(const Data& certificateName, const Data& domainName)

const char *dot = NULL;

const char *certName = certificateName.c_str();

Index: resip/stack/ssl/Security.hxx

===================================================================

--- resip/stack/ssl/Security.hxx (revision 9050)

+++ resip/stack/ssl/Security.hxx (working copy)

@@ -167,11 +167,15 @@

static bool isSelfSigned(const X509* cert);

- // match with wildcards

static int matchHostName(const Data& certificateName, const Data& domainName);

// allow particular classes to acces the functions below

// friend class TlsConnection;

+ // Allow overriding of RFC 5922 rules on certificate matching.

+ static void setAllowWildcardCertificates(bool bEnable) { mAllowWildcardCertificates = bEnable; }

+ static bool allowWildcardCertificates() { return mAllowWildcardCertificates; }

public:

SSL_CTX* getTlsCtx ();

SSL_CTX* getSslCtx ();

@@ -216,6 +220,10 @@

Data getPrivateKeyPEM (PEMType type, const Data& name) const;

Data getPrivateKeyDER (PEMType type, const Data& name) const;

void addPrivateKeyPKEY(PEMType type, const Data& name, EVP_PKEY* pKey, bool write) const;

+ // match with wildcards

+ static int matchHostNameWithWildcards(const Data& certificateName, const Data& domainName);

+ static bool mAllowWildcardCertificates;

};

class Security : public BaseSecurity

Index: resip/stack/ssl/TlsConnection.cxx

===================================================================

--- resip/stack/ssl/TlsConnection.cxx (revision 9050)

+++ resip/stack/ssl/TlsConnection.cxx (working copy)

@@ -280,25 +280,12 @@

bool matches = false;

for(std::list<BaseSecurity::PeerName>::iterator it = mPeerNames.begin(); it != mPeerNames.end(); it++)

{

- if(it->mType == BaseSecurity::CommonName)

+ if(BaseSecurity::matchHostName(it->mName, who().getTargetDomain()))

{

- //allow wildcard match for subdomain name (RFC 2459)

- if(BaseSecurity::matchHostName(it->mName, who().getTargetDomain()))

- {

- matches=true;

- break;

- }

- else //it->mType == SubjectAltName

- {

- //no wildcards for SubjectAltName

- if(isEqualNoCase(it->mName, who().getTargetDomain()))

- {

matches=true;

break;

}

- }

if(!matches)

{

mTlsState = Broken;

On Sun, Mar 13, 2011 at 1:50 PM, Scott Godin <sgodin@xxxxxxxxxxxxxxx> wrote:

Hi Aron,

I think the default needs to be flipped. I think RFC5922 support should be enabled by default, and you must explicitly turn the option off to get the non-RFC compliant behavior.

I am a little concerned about people relying on wildcards in the CommonName today - but as long as we document the need to use the following setting - I think we are good:
TlsConnection::mRFC5922CertificateRules = false;

Scott

---------- Forwarded message ----------
From: Aron Rosenberg <arosenberg@xxxxxxxxxxxx>
Date: Sun, Mar 13, 2011 at 4:31 PM
Subject: [reSIProcate] Proposed changes to support optional RFC 5922 TLS Certificate matching rules
To: resiprocate-devel <resiprocate-devel@xxxxxxxxxxxxxxx>

All,

Below is a proposed patch to allow RFC 5922 TLS certificate common and subj-alt-name matching rules. RFC 5922 mandates that wildcard matching MUST NOT be used. However, current resip code allows wildcards on the common name field.

By default, this new mode is disabled. As part of this patch, the non 5922 mode is changed to allow wildcard matching in all fields, since this mode is allowed within HTTP over TLS, not prohibited by 3261(and normal web browsers allow it too.

If there are no complaints, I will commit sometime this week.

-Aron

Index: resip/stack/ssl/TlsConnection.cxx
===================================================================

--- resip/stack/ssl/TlsConnection.cxx (revision 9058)
+++ resip/stack/ssl/TlsConnection.cxx (working copy)

@@ -24,6 +24,8 @@

#define RESIPROCATE_SUBSYSTEM Subsystem::TRANSPORT

+bool TlsConnection::mRFC5922CertificateRules = false;
+
TlsConnection::TlsConnection( Transport* transport, const Tuple& tuple,

   Socket fd, Security* security,
   bool server, Data domain, SecurityTypes::SSLType sslType ,
@@ -280,25 +282,21 @@
   bool matches = false;

   for(std::list<BaseSecurity::PeerName>::iterator it = mPeerNames.begin(); it != mPeerNames.end(); it++)
   {
- if(it->mType == BaseSecurity::CommonName)
+ if(mRFC5922CertificateRules)

   {
- //allow wildcard match for subdomain name (RFC 2459)
- if(BaseSecurity::matchHostName(it->mName, who().getTargetDomain()))
+ if(isEqualNoCase(it->mName, who().getTargetDomain()))

   {
   matches=true;
   break;
   }
   }
- else //it->mType == SubjectAltName
- {

- //no wildcards for SubjectAltName
- if(isEqualNoCase(it->mName, who().getTargetDomain()))
+ //.amr. allow wildcard match (RFC 2459), RFC 3261 doesn't prevent wildcards.

+ else if(BaseSecurity::matchHostName(it->mName, who().getTargetDomain()))
   {
   matches=true;
   break;
   }
   }

- }
   if(!matches)
   {
   mTlsState = Broken;
Index: resip/stack/ssl/TlsConnection.hxx
===================================================================

--- resip/stack/ssl/TlsConnection.hxx (revision 9057)
+++ resip/stack/ssl/TlsConnection.hxx (working copy)

@@ -48,6 +48,8 @@

   typedef enum TlsState { Initial, Broken, Handshaking, Up } TlsState;
   static const char * fromState(TlsState);
+ static bool RFC5922CertificateRules() { return mRFC5922CertificateRules; }

+ static void setRFC5922CertificateRules(bool bEnable) { mRFC5922CertificateRules = bEnable; }

   private:
   /// No default c'tor
@@ -67,6 +69,7 @@
   SSL* mSsl;
   BIO* mBio;
   std::list<BaseSecurity::PeerName> mPeerNames;
+ static bool mRFC5922CertificateRules;
};

}

_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel

References:
- [reSIProcate] Proposed changes to support optional RFC 5922 TLS Certificate matching rules
  - From: Aron Rosenberg
- [reSIProcate] Fwd: Proposed changes to support optional RFC 5922 TLS Certificate matching rules
  - From: Scott Godin

Prev by Date: [reSIProcate] Fwd: Proposed changes to support optional RFC 5922 TLS Certificate matching rules
Next by Date: [reSIProcate] stack initialization proposed change
Previous by thread: [reSIProcate] Fwd: Proposed changes to support optional RFC 5922 TLS Certificate matching rules
Next by thread: [reSIProcate] stack initialization proposed change
Index(es):
- Date
- Thread