Thread: Re: [COMMITTERS] pgsql: Add support for matching wildcard server certificates to the new

Magnus Hagander wrote:
> Tom Lane wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>> ... The other option is to have
>>> autoconf substitute our own local version of fnmatch when this happens.
>>> How would one go about to do that in autoconf asks the autoconf n00b?
>> I'd try adding a little test program that tries to compile
>>
>>     #include <fnmatch.h>
>>     ...
>>     n = fnmatch("foo","bar", FNM_CASEFOLD);
>>
>> and see if that succeeds.
>>
>> For extra credit, incorporate that into a new PGAC_FUNC_FNMATCH macro,
>> but I think you'd need to get Peter's help on the details --- I'm just
>> a duffer on autoconf myself.
> 
> Yeah, likely that someone who knows this will make it happen a lot faster.
> 
> Peter, got time to help me out with this? Thanks!

Well, FNM_CASEFOLD is not POSIX, and Autoconf thinks it's a GNU 
extension, which has obviously crept into other systems.  So you'd need 
to use AC_FUNC_FNMATCH_GNU, but that also requires you to use the GNU 
replacement implementation.  (A bit stupid, but then again, if you are 
trying to use GNU features, whose replacement implementation are you 
going to use.)

If you google for FNM_CASEFOLD, you will get about a million hits of 
other open-source projects having run into this same issue.

Then again, having looked into the libpq source now, is using fnmatch() 
even appropriate here?  The matching rules for https are in RFC 2818:
   Matching is performed using the matching rules specified by   [RFC2459].  If more than one identity of a given type
ispresent in   the certificate (e.g., more than one dNSName name, a match in any one   of the set is considered
acceptable.)Names may contain the wildcard   character * which is considered to match any single domain name
componentor component fragment. E.g., *.a.com matches foo.a.com but   not bar.foo.a.com. f*.com matches foo.com but not
bar.com.

Using fnmatch(), however, will also treat ? and [] special and it will 
not follow the "any single domain name component" rule.

Peter Eisentraut wrote:
> Magnus Hagander wrote:
>> Tom Lane wrote:
>>> Magnus Hagander <magnus@hagander.net> writes:
>>>> ... The other option is to have
>>>> autoconf substitute our own local version of fnmatch when this happens.
>>>> How would one go about to do that in autoconf asks the autoconf n00b?
>>> I'd try adding a little test program that tries to compile
>>>
>>>     #include <fnmatch.h>
>>>     ...
>>>     n = fnmatch("foo","bar", FNM_CASEFOLD);
>>>
>>> and see if that succeeds.
>>>
>>> For extra credit, incorporate that into a new PGAC_FUNC_FNMATCH macro,
>>> but I think you'd need to get Peter's help on the details --- I'm just
>>> a duffer on autoconf myself.
>>
>> Yeah, likely that someone who knows this will make it happen a lot
>> faster.
>>
>> Peter, got time to help me out with this? Thanks!
> 
> Well, FNM_CASEFOLD is not POSIX, and Autoconf thinks it's a GNU
> extension, which has obviously crept into other systems.  So you'd need
> to use AC_FUNC_FNMATCH_GNU, but that also requires you to use the GNU
> replacement implementation.  (A bit stupid, but then again, if you are
> trying to use GNU features, whose replacement implementation are you
> going to use.)

Meh, I looked at that, and considered having to implement it the GNU way
was bad. Since we can't just import the GNU sourcecode.



> If you google for FNM_CASEFOLD, you will get about a million hits of
> other open-source projects having run into this same issue.
> 
> Then again, having looked into the libpq source now, is using fnmatch()
> even appropriate here?  The matching rules for https are in RFC 2818:
> 
>    Matching is performed using the matching rules specified by
>    [RFC2459].  If more than one identity of a given type is present in
>    the certificate (e.g., more than one dNSName name, a match in any one
>    of the set is considered acceptable.) Names may contain the wildcard
>    character * which is considered to match any single domain name
>    component or component fragment. E.g., *.a.com matches foo.a.com but
>    not bar.foo.a.com. f*.com matches foo.com but not bar.com.
> 
> Using fnmatch(), however, will also treat ? and [] special and it will
> not follow the "any single domain name component" rule.

Grr. I didn't find that RFC when I was googling around for the rules.
Must've been a bad-google-day for me. And that does *not* match what I
found on a couple of SSL-certificate-selling-websites that told you how
it worked :-(

I guess it's back to the drawingboard. Can probably still base it on the
fnmatch stuff, but it'll need to be ripped apart. Basically, it should
match only with *, and * should not match "." - do you agree that's a
reasonable interpretation?

//Magnus

Peter Eisentraut <peter_e@gmx.net> writes:
> Well, FNM_CASEFOLD is not POSIX, and Autoconf thinks it's a GNU 
> extension, which has obviously crept into other systems.  So you'd need 
> to use AC_FUNC_FNMATCH_GNU, but that also requires you to use the GNU 
> replacement implementation.  (A bit stupid, but then again, if you are 
> trying to use GNU features, whose replacement implementation are you 
> going to use.)

It's also fair to wonder whether an appropriate set of case folding
rules is going to get used.
        regards, tom lane

On Monday 24 November 2008 16:55:17 Magnus Hagander wrote:
> > Then again, having looked into the libpq source now, is using fnmatch()
> > even appropriate here?  The matching rules for https are in RFC 2818:

> > Using fnmatch(), however, will also treat ? and [] special and it will
> > not follow the "any single domain name component" rule.

> I guess it's back to the drawingboard. Can probably still base it on the
> fnmatch stuff, but it'll need to be ripped apart. Basically, it should
> match only with *, and * should not match "." - do you agree that's a
> reasonable interpretation?

Some more information on this: 
https://www.switch.ch/pki/meetings/2007-01/namebased_ssl_virtualhosts.pdf 
slide 5 lists the matching rules for email, HTTP, and LDAP over TLS, 
respectively, which are not all the same.  Also note that these methods have 
rules for interpreting fields in the certificate other than the common name 
for the host name.

I think it is safest and easiest to allow a * wildcard only as the first 
character and only when followed immediately by a dot.

Maybe some DNS expert around here can offer advice on what a morally sound 
solution would be.

I wrote:
> Some more information on this:
> https://www.switch.ch/pki/meetings/2007-01/namebased_ssl_virtualhosts.pdf
> slide 5 lists the matching rules for email, HTTP, and LDAP over TLS,
> respectively, which are not all the same.  Also note that these methods
> have rules for interpreting fields in the certificate other than the common
> name for the host name.
>
> I think it is safest and easiest to allow a * wildcard only as the first
> character and only when followed immediately by a dot.
>
> Maybe some DNS expert around here can offer advice on what a morally sound
> solution would be.

This page summarizes the sadness pretty well:

http://wiki.cacert.org/wiki/WildcardCertificates

Peter Eisentraut wrote:
> I wrote:
>> Some more information on this:
>> https://www.switch.ch/pki/meetings/2007-01/namebased_ssl_virtualhosts.pdf
>> slide 5 lists the matching rules for email, HTTP, and LDAP over TLS,
>> respectively, which are not all the same.  Also note that these methods
>> have rules for interpreting fields in the certificate other than the common
>> name for the host name.
>>
>> I think it is safest and easiest to allow a * wildcard only as the first
>> character and only when followed immediately by a dot.
>>
>> Maybe some DNS expert around here can offer advice on what a morally sound
>> solution would be.
>
> This page summarizes the sadness pretty well:
>
> http://wiki.cacert.org/wiki/WildcardCertificates

Yuck, that was certainly sad.

I think the most reasonable thing is to match the way that "modern
browsers" appear to do, which is that it matches * against subdomains as
well.

Matching *only* as the first character will make it impossible to make
certificates for "www*.domain.com", which is AFAIK fairly popular - and
one of the examples you'll find on CA sites. But it would be fairly easy
to add this restriction if people feel that's a better way.

See attached patch which takes out the parts of fnmatch that we're not
interested in, and puts it directly in fe-secure.c. Obviously, if we go
down that way, we can remove fnmatch.c from port again :-)

Thoughts?

//Magnus

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 828e867..28e254f 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -55,6 +55,7 @@
 #endif

 #ifdef USE_SSL
+
 #include <openssl/ssl.h>
 #include <openssl/bio.h>
 #if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
@@ -64,16 +65,6 @@
 #include <openssl/engine.h>
 #endif

-/* fnmatch() needed for client certificate checking */
-#ifdef HAVE_FNMATCH
-#include <fnmatch.h>
-#else
-#include "fnmatchstub.h"
-#endif
-#endif   /* USE_SSL */
-
-
-#ifdef USE_SSL

 #ifndef WIN32
 #define USER_CERT_FILE        ".postgresql/postgresql.crt"
@@ -444,6 +435,55 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
 }

 /*
+ * Check if a wildcard certificate matches. This code is based on the
+ * NetBSD version of fnmatch(), but adapted to match wildcard certificates
+ * by removing much of the filename/directory specific functionality.
+ * Based on what "most others" do for https, we match the wildcard '*' to
+ * any part, *including* subdomains. This is contrary to RFC2818, but it is
+ * what most modern browsers match
+ * (see http://wiki.cacert.org/wiki/WildcardCertificates)
+ *
+ * Matching is always cone case-insensitive, since DNS is case insensitive.
+ */
+static int
+wildcard_certificate_match(const char *pattern, const char *string)
+{
+    const char *stringstart;
+    char c, test;
+
+    for (stringstart = string;;)
+    {
+        switch (c = tolower(*pattern++)) {
+        case '\0':
+            return (*string == '\0') ? 0 : 1;
+        case '*':
+            c = tolower(*pattern);
+            /* Collapse multiple stars. */
+            while (c == '*')
+                c = tolower(*++pattern);
+
+            if (c == '\0')
+                return 0;
+
+            /* General case, use recursion. */
+            while ((test = tolower(*string)) != '\0')
+            {
+                if (!wildcard_certificate_match(pattern, string))
+                    return 0;
+                ++string;
+            }
+            return 1;
+        default:
+            if (c != tolower(*string++))
+                return 1;
+            break;
+        }
+    /* NOTREACHED */
+    }
+}
+
+
+/*
  *    Verify that common name resolves to peer.
  */
 static bool
@@ -472,7 +512,7 @@ verify_peer_name_matches_certificate(PGconn *conn)
         if (pg_strcasecmp(conn->peer_cn, conn->pghost) == 0)
             /* Exact name match */
             return true;
-        else if (fnmatch(conn->peer_cn, conn->pghost, FNM_NOESCAPE/* | FNM_CASEFOLD*/) == 0)
+        else if (wildcard_certificate_match(conn->peer_cn, conn->pghost) == 0)
             /* Matched wildcard certificate */
             return true;
         else

Magnus Hagander <magnus@hagander.net> writes:
> See attached patch which takes out the parts of fnmatch that we're not
> interested in, and puts it directly in fe-secure.c. Obviously, if we go
> down that way, we can remove fnmatch.c from port again :-)

> Thoughts?

Generally +1, but a couple of comments:

* This seems to be still mostly NetBSD code, so I think you need to do
more than just credit them in an aside.  Should we repeat the full
NetBSD copyright notice for this one function?

* This is still making unjustified assumptions about the behavior of
tolower/toupper.  I think you probably want ASCII-only case folding,
ie use pg_toupper/pg_tolower.  If it actually should be locale aware
then it's still wrong because it won't work in multibyte encodings.
Also you forgot the de rigueur (unsigned char) casts for ctype.h calls.
        regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> See attached patch which takes out the parts of fnmatch that we're not
>> interested in, and puts it directly in fe-secure.c. Obviously, if we go
>> down that way, we can remove fnmatch.c from port again :-)
> 
>> Thoughts?
> 
> Generally +1, but a couple of comments:
> 
> * This seems to be still mostly NetBSD code, so I think you need to do
> more than just credit them in an aside.  Should we repeat the full
> NetBSD copyright notice for this one function?

Do you mean the* Copyright (c) 1989, 1993, 1994*      The Regents of the University of California.  All rights
reserved.**This code is derived from software contributed to Berkeley by* Guido van Rossum.
 

part, or the whole licence? Since the licence is the same as ours, doing
that seems like overkill.


> * This is still making unjustified assumptions about the behavior of
> tolower/toupper.  I think you probably want ASCII-only case folding,
> ie use pg_toupper/pg_tolower.  If it actually should be locale aware
> then it's still wrong because it won't work in multibyte encodings.
> Also you forgot the de rigueur (unsigned char) casts for ctype.h calls.

Will fix.

//Magnus

Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> * This seems to be still mostly NetBSD code, so I think you need to do
>> more than just credit them in an aside.  Should we repeat the full
>> NetBSD copyright notice for this one function?

> Do you mean the
>  * Copyright (c) 1989, 1993, 1994
>  *      The Regents of the University of California.  All rights reserved.
>  *
>  * This code is derived from software contributed to Berkeley by
>  * Guido van Rossum.

> part, or the whole licence? Since the licence is the same as ours, doing
> that seems like overkill.

Well, it does say
* 1. Redistributions of source code must retain the above copyright*    notice, this list of conditions and the
followingdisclaimer.
 

        regards, tom lane

On Friday 28 November 2008 17:13:54 Magnus Hagander wrote:
> Matching *only* as the first character will make it impossible to make
> certificates for "www*.domain.com", which is AFAIK fairly popular - and
> one of the examples you'll find on CA sites. But it would be fairly easy
> to add this restriction if people feel that's a better way.

Are there actual technical or administrative or security arguments for or 
against this?  For example, what are the criteria one has to fulfill in order 
to get such a certificate?  Or is there a "defensive certification" security 
line of reasoning?

Now certificate issuing is a real business, so we need to play in that context 
as well, but I would like to dig a little deeper why things should be done in 
a certain way.

I am quite confortable, for example, with * matching subdomains, because if I 
own example.com, then I can create any level of subdomain I want, without 
making a real difference to user/client program.  But then I don't really get 
the point of having * inside of words -- would "www*.domain.com" also match 
dots then?

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Tom Lane wrote:
>>> * This seems to be still mostly NetBSD code, so I think you need to do
>>> more than just credit them in an aside.  Should we repeat the full
>>> NetBSD copyright notice for this one function?
> 
>> Do you mean the
>>  * Copyright (c) 1989, 1993, 1994
>>  *      The Regents of the University of California.  All rights reserved.
>>  *
>>  * This code is derived from software contributed to Berkeley by
>>  * Guido van Rossum.
> 
>> part, or the whole licence? Since the licence is the same as ours, doing
>> that seems like overkill.
> 
> Well, it does say
> 
>  * 1. Redistributions of source code must retain the above copyright
>  *    notice, this list of conditions and the following disclaimer.
> 

Right, maybe we'd better do that even if it's identical to our own.
Better credit too much than not enough. I'll put the whole thing in -
will move the function to the bottom of the file as well to clearly
isolate which parts it's referring to.

//Magnus

Peter Eisentraut wrote:
> On Friday 28 November 2008 17:13:54 Magnus Hagander wrote:
>> Matching *only* as the first character will make it impossible to make
>> certificates for "www*.domain.com", which is AFAIK fairly popular - and
>> one of the examples you'll find on CA sites. But it would be fairly easy
>> to add this restriction if people feel that's a better way.
> 
> Are there actual technical or administrative or security arguments for or 
> against this?  For example, what are the criteria one has to fulfill in order 
> to get such a certificate?  Or is there a "defensive certification" security 
> line of reasoning?
> 
> Now certificate issuing is a real business, so we need to play in that context 
> as well, but I would like to dig a little deeper why things should be done in 
> a certain way.
> 
> I am quite confortable, for example, with * matching subdomains, because if I 
> own example.com, then I can create any level of subdomain I want, without 
> making a real difference to user/client program.  But then I don't really get 
> the point of having * inside of words -- would "www*.domain.com" also match 
> dots then?

Hmm. I can't seem to find that reference anymore. The only one of my
"www*" references I can find ATM is GoDaddy which just has it as an
exapmle of what "*.domain.com" would match :S

Perhaps the best method would actually be to match only "*." at the
beginning of the CN for now, and see if people complain? I would much
like someone who knows more about what would be reasonable to speak up
here, but it seems we don't have anybody here who knows...

//Magnus

> Perhaps the best method would actually be to match only "*." at the
> beginning of the CN for now, and see if people complain? I would much
> like someone who knows more about what would be reasonable to speak up
> here, but it seems we don't have anybody here who knows...

I would encourage you to adopt a solution where * matches only a
single pathname component.  This seems to be the intention of both
RFC2818 and RFC2595.  It is also the behavior of IE7; FF2 seems to
deviate from the spec.

http://www.hanselman.com/blog/SomeTroubleWithWildcardSSLCertificatesFireFoxAndRFC2818.aspx

There are several other advantages of this approach that seem worth mentioning:

1. If you make it match a single pathname component now, and later
decide that you were wrong and change your mind, it is guaranteed not
to break any working installations.  The reverse is not true.

2. I can't see any possible way that matching a single component could
create security holes that would be eliminated by matching multiple
components, but I'm more skeptical about the other direction.  What
about the old DNS hack where you create a DNS record for
example.com.sample.com and hijack connections intended for example.com
made by people whose default DNS suffix is sample.com?  There may be
reason to believe this isn't a problem, but matching less seems like
it can't possibly be a bad thing.

3. It would be truly bizarre if www*.example.com matched
www17.some.stuff.in.the.middle.example.com.  (That having been said, I
wouldn't worry about wildcards intended to match part of a component
too much.  I suspect that it's an extremely rare case, and we can
always add support later if there is demand for it.  Not worrying
about this now will help keep the code simple and free of bugs, always
good in a security-critical context.)

...Robert

Robert Haas wrote:
>> Perhaps the best method would actually be to match only "*." at the
>> beginning of the CN for now, and see if people complain? I would much
>> like someone who knows more about what would be reasonable to speak up
>> here, but it seems we don't have anybody here who knows...
> 
> I would encourage you to adopt a solution where * matches only a
> single pathname component.  This seems to be the intention of both
> RFC2818 and RFC2595.  It is also the behavior of IE7; FF2 seems to
> deviate from the spec.
> 
> http://www.hanselman.com/blog/SomeTroubleWithWildcardSSLCertificatesFireFoxAndRFC2818.aspx

If you look at the wiki page mentioned upthread,
http://wiki.cacert.org/wiki/WildcardCertificates, you will see that it
seems like *all* products other than IE are converging on the non-IE
behavior. Which would be an argument for implementing that method.


> There are several other advantages of this approach that seem worth mentioning:
> 
> 1. If you make it match a single pathname component now, and later
> decide that you were wrong and change your mind, it is guaranteed not
> to break any working installations.  The reverse is not true.

True.


> 2. I can't see any possible way that matching a single component could
> create security holes that would be eliminated by matching multiple
> components, but I'm more skeptical about the other direction.  What
> about the old DNS hack where you create a DNS record for
> example.com.sample.com and hijack connections intended for example.com
> made by people whose default DNS suffix is sample.com?  There may be
> reason to believe this isn't a problem, but matching less seems like
> it can't possibly be a bad thing.

Right, but that's all about being careful not to give out certs like
"*.postgres.*".


> 3. It would be truly bizarre if www*.example.com matched
> www17.some.stuff.in.the.middle.example.com.  (That having been said, I
> wouldn't worry about wildcards intended to match part of a component
> too much.  I suspect that it's an extremely rare case, and we can
> always add support later if there is demand for it.  Not worrying
> about this now will help keep the code simple and free of bugs, always
> good in a security-critical context.)

Yeah.

I think I agree with the idea that we should match wildcards only at the
beginning of the name *for now*, and then see what people actually
request :-) I'm less sure about the single-pathname-component part, but
the argument around backwards compatible is certainly a very valid one..

//Magnus

>> 2. I can't see any possible way that matching a single component could
>> create security holes that would be eliminated by matching multiple
>> components, but I'm more skeptical about the other direction.  What
>> about the old DNS hack where you create a DNS record for
>> example.com.sample.com and hijack connections intended for example.com
>> made by people whose default DNS suffix is sample.com?  There may be
>> reason to believe this isn't a problem, but matching less seems like
>> it can't possibly be a bad thing.
>
> Right, but that's all about being careful not to give out certs like
> "*.postgres.*".

Errrr...no.  The point is that if you've hacked sample.com's DNS
server, you might have a cert for *.sample.com, but you might NOT have
a cert for example.com.

...Robert

Robert Haas wrote:
>>> 2. I can't see any possible way that matching a single component could
>>> create security holes that would be eliminated by matching multiple
>>> components, but I'm more skeptical about the other direction.  What
>>> about the old DNS hack where you create a DNS record for
>>> example.com.sample.com and hijack connections intended for example.com
>>> made by people whose default DNS suffix is sample.com?  There may be
>>> reason to believe this isn't a problem, but matching less seems like
>>> it can't possibly be a bad thing.
>> Right, but that's all about being careful not to give out certs like
>> "*.postgres.*".
> 
> Errrr...no.  The point is that if you've hacked sample.com's DNS
> server, you might have a cert for *.sample.com, but you might NOT have
> a cert for example.com.

Oh, now I see. Yes, it would break on that. But I don't really see the
problem:

* If you have a cert for *.sample.com, you trust sample.com
* All you can do is direct traffic *to* sample.com, which is trusted.

But I guess it could be a potential issue with global CAs, if you just
blindly add them to the trust list.

//Magnus

Magnus Hagander wrote:
> I think I agree with the idea that we should match wildcards only at the
> beginning of the name *for now*, and then see what people actually
> request :-) I'm less sure about the single-pathname-component part, but
> the argument around backwards compatible is certainly a very valid one..

Here's one that (I think) does that. For every step, the code becomes
simpler - which I like when it comes to security code :)

//Magnus

*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***************
*** 55,60 ****
--- 55,61 ----
  #endif

  #ifdef USE_SSL
+
  #include <openssl/ssl.h>
  #include <openssl/bio.h>
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
***************
*** 64,79 ****
  #include <openssl/engine.h>
  #endif

- /* fnmatch() needed for client certificate checking */
- #ifdef HAVE_FNMATCH
- #include <fnmatch.h>
- #else
- #include "fnmatchstub.h"
- #endif
- #endif   /* USE_SSL */
-
-
- #ifdef USE_SSL

  #ifndef WIN32
  #define USER_CERT_FILE        ".postgresql/postgresql.crt"
--- 65,70 ----
***************
*** 443,448 **** verify_cb(int ok, X509_STORE_CTX *ctx)
--- 434,481 ----
      return ok;
  }

+
+ /*
+  * Check if a wildcard certificate matches the server hostname.
+  *
+  * The rule for this is:
+  *  1. We only match the '*' character as wildcard
+  *  2. We match only wildcards at the start of the string
+  *  3. The '*' character does *not* match '.', meaning that we match only
+  *     a single pathname component.
+  *  4. We don't support more than one '*' in a single pattern.
+  *
+  * This is roughly in line with RFC2818, but contrary to what most browsers
+  * appear to be implementing (point 3 being the difference)
+  *
+  * Matching is always cone case-insensitive, since DNS is case insensitive.
+  */
+ static int
+ wildcard_certificate_match(const char *pattern, const char *string)
+ {
+     /* If we don't start with a wildcard, it's not a match (rule 1 & 2) */
+     if (strlen(pattern) < 3 ||
+         pattern[0] != '*' ||
+         pattern[1] != '.')
+         return 0;
+
+     if (strlen(pattern) > strlen(string))
+         /* If pattern is longer than the string, we can never match */
+         return 0;
+
+     if (pg_strcasecmp(pattern+1, string+strlen(string)-strlen(pattern)+1) != 0)
+         /* If string does not end in pattern (minus the wildcard), we don't match */
+         return 0;
+
+     if (strchr(string, '.') < string+strlen(string)-strlen(pattern))
+         /* If there is a dot left of where the pattern started to match, we don't match (rule 3) */
+         return 0;
+
+     /* String ended with pattern, and didn't have a dot before, so we match */
+     return 1;
+ }
+
+
  /*
   *    Verify that common name resolves to peer.
   */
***************
*** 472,478 **** verify_peer_name_matches_certificate(PGconn *conn)
          if (pg_strcasecmp(conn->peer_cn, conn->pghost) == 0)
              /* Exact name match */
              return true;
!         else if (fnmatch(conn->peer_cn, conn->pghost, FNM_NOESCAPE/* | FNM_CASEFOLD*/) == 0)
              /* Matched wildcard certificate */
              return true;
          else
--- 505,511 ----
          if (pg_strcasecmp(conn->peer_cn, conn->pghost) == 0)
              /* Exact name match */
              return true;
!         else if (wildcard_certificate_match(conn->peer_cn, conn->pghost))
              /* Matched wildcard certificate */
              return true;
          else

>>>> 2. I can't see any possible way that matching a single component could
>>>> create security holes that would be eliminated by matching multiple
>>>> components, but I'm more skeptical about the other direction.  What
>>>> about the old DNS hack where you create a DNS record for
>>>> example.com.sample.com and hijack connections intended for example.com
>>>> made by people whose default DNS suffix is sample.com?  There may be
>>>> reason to believe this isn't a problem, but matching less seems like
>>>> it can't possibly be a bad thing.
>>> Right, but that's all about being careful not to give out certs like
>>> "*.postgres.*".
>>
>> Errrr...no.  The point is that if you've hacked sample.com's DNS
>> server, you might have a cert for *.sample.com, but you might NOT have
>> a cert for example.com.
>
> Oh, now I see. Yes, it would break on that. But I don't really see the
> problem:
>
> * If you have a cert for *.sample.com, you trust sample.com
> * All you can do is direct traffic *to* sample.com, which is trusted.
>
> But I guess it could be a potential issue with global CAs, if you just
> blindly add them to the trust list.

Well, that's true, in a way, but sample.com isn't a unified entity.
The idea of hacks like this is that if sample.com is a large company
with a lousy IT department and example.com is, say, a financial web
site where people enter their social security and pin number to log
in, you can, by hijacking the traffic intended for example.com, and
collect personal information.

I'm not quite sure how germane this is under SSL because there may be
tight enough restrictions on the way that host names are interepreted
that it isn't an issue - but things like this were major sources of
security holes back in the early nineties when I was last dealing with
these sorts of issues.

In any case it seems you've chosen to keep this simple for now, which
means that it isn't necessary for either of us to understand where the
pitfalls might be in some more complex approach.

...Robert

Looks good to me, except for a somewhat excessive number of calls to
strlen() on the same input data.

...Robert

On Mon, Dec 1, 2008 at 10:31 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Magnus Hagander wrote:
>> I think I agree with the idea that we should match wildcards only at the
>> beginning of the name *for now*, and then see what people actually
>> request :-) I'm less sure about the single-pathname-component part, but
>> the argument around backwards compatible is certainly a very valid one..
>
> Here's one that (I think) does that. For every step, the code becomes
> simpler - which I like when it comes to security code :)
>
> //Magnus
>
>

I could assign it to a variable, but won't the compiler just optimize
that away?

//Magnus

Robert Haas wrote:
> Looks good to me, except for a somewhat excessive number of calls to
> strlen() on the same input data.
> 
> ...Robert
> 
> On Mon, Dec 1, 2008 at 10:31 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> Magnus Hagander wrote:
>>> I think I agree with the idea that we should match wildcards only at the
>>> beginning of the name *for now*, and then see what people actually
>>> request :-) I'm less sure about the single-pathname-component part, but
>>> the argument around backwards compatible is certainly a very valid one..
>> Here's one that (I think) does that. For every step, the code becomes
>> simpler - which I like when it comes to security code :)
>>
>> //Magnus
>>
>>

Magnus Hagander <magnus@hagander.net> writes:
> I could assign it to a variable, but won't the compiler just optimize
> that away?

Wouldn't count on that, particularly not if you are modifying other
strings at the same time.
        regards, tom lane

On 1 dec 2008, at 18.10, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Magnus Hagander <magnus@hagander.net> writes:
>> I could assign it to a variable, but won't the compiler just optimize
>> that away?
>
> Wouldn't count on that, particularly not if you are modifying other
> strings at the same time.
>
I'm not modifying them. But - point taken, will change.

Another q: given that we no longer need fnmatch(), should we remove it  
from port, or leave it around in case we need it in the future? (it's  
not like we can't get it back if we need it, but...)

/Magnus

Magnus Hagander <magnus@hagander.net> writes:
> Another q: given that we no longer need fnmatch(), should we remove it  
> from port,

Yes.  configure runs slow enough already without testing for functions
we don't need (especially if the test involves more than bare existence,
as in this case).
        regards, tom lane