Moodle
  1. Moodle
  2. MDL-36818

Wrong setting for CURLOPT_SSL_VERIFYHOST in CAS

    Details

    • Testing Instructions:
      Hide

      DanP: I do not feel the time required to setup a SSL'd CAS server is necessary here. I could add testing instructions to test CAS without SSL, but it wouldn't be testing anything.

      Therefore i'm going to suggest passing this based on the code and the fact its been applied upstream. If someone from the community has a ssl-enabled CAS setup, it'd be great if we could get them to test it.

      Show
      DanP: I do not feel the time required to setup a SSL'd CAS server is necessary here. I could add testing instructions to test CAS without SSL, but it wouldn't be testing anything. Therefore i'm going to suggest passing this based on the code and the fact its been applied upstream. If someone from the community has a ssl-enabled CAS setup, it'd be great if we could get them to test it.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
      MDL-36818-master
    • Rank:
      46339

      Description

      S3 repository and CAS client make improper use of CURLOPT_SSL_VERIFYHOST in curl library - they set it to the value of 1 instead of 2.

      From the libcurl documentation:

      > When CURLOPT_SSL_VERIFYHOST is 2, that certificate must indicate that the
      > server is the server to which you meant to connect, or the connection fails.
      >
      > Curl considers the server the intended one when the Common Name field or a
      > Subject Alternate Name field in the certificate matches the host name in the
      > URL to which you told Curl to connect.
      >
      > When the value is 1, the certificate must contain a Common Name field, but it
      > doesn't matter what name it says. (This is not ordinarily a useful setting).

      Thanks to Alessandro Ghedini for reporting it.

      The fixes has been sent to upstream developers:
      https://github.com/tpyo/amazon-s3-php-class/pull/36
      https://github.com/Jasig/phpCAS/pull/58

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that Tomaz.

          If you have posted fixes to public repositories, could you please remove them and attach stand-alone patches to this issue instead? This is the current approach we are taking (see http://docs.moodle.org/dev/Process#Security_issues)

          I'm sure Petr will triage this issue fully soon.

          Show
          Michael de Raadt added a comment - Thanks for reporting that Tomaz. If you have posted fixes to public repositories, could you please remove them and attach stand-alone patches to this issue instead? This is the current approach we are taking (see http://docs.moodle.org/dev/Process#Security_issues ) I'm sure Petr will triage this issue fully soon.
          Hide
          Tomasz Muras added a comment -

          Hi Michael,

          This issue is reported by Debian Developer & was passed to Debian Security team. They re creating public CVEs as part of their procedure, so we need to assume that this information is public.

          Tomek

          Show
          Tomasz Muras added a comment - Hi Michael, This issue is reported by Debian Developer & was passed to Debian Security team. They re creating public CVEs as part of their procedure, so we need to assume that this information is public. Tomek
          Hide
          Petr Škoda added a comment -

          Thanks for the report.

          Show
          Petr Škoda added a comment - Thanks for the report.
          Hide
          Thijs Kinkhorst added a comment -

          Patch for this issue

          Show
          Thijs Kinkhorst added a comment - Patch for this issue
          Hide
          Thijs Kinkhorst added a comment -

          Is there a reason that this issue is still open in phpCAS as included in Moodle?

          Attached is the patch we're using in production for months with CAS, without issues.

          Show
          Thijs Kinkhorst added a comment - Is there a reason that this issue is still open in phpCAS as included in Moodle? Attached is the patch we're using in production for months with CAS, without issues.
          Hide
          Frédéric Massart added a comment -

          Thanks Thijs,

          We don't usually patch the external libraries ourselves, but that happens that we cherry-pick some of the security issues. I will try to bring light on this issue.

          What worries me most is actually:

          All phpCAS versions before 1.3.2 have multiple security issues: CVE-2012-5583, CVE-2010-2795, CVE-2010-2796,CVE-2010-3690,CVE-2010-3691,CVE-2010-3692, CVE-2012-1104, CVE-2012-1105. Please upgrade to the latest version.
          https://wiki.jasig.org/display/CASC/phpCAS

          We are using 1.1.3... perhaps it would be worth updating the whole library, unfortunately this poses problems when it comes to backporting the patch.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks Thijs, We don't usually patch the external libraries ourselves, but that happens that we cherry-pick some of the security issues. I will try to bring light on this issue. What worries me most is actually: All phpCAS versions before 1.3.2 have multiple security issues: CVE-2012-5583, CVE-2010-2795, CVE-2010-2796,CVE-2010-3690,CVE-2010-3691,CVE-2010-3692, CVE-2012-1104, CVE-2012-1105. Please upgrade to the latest version. https://wiki.jasig.org/display/CASC/phpCAS We are using 1.1.3... perhaps it would be worth updating the whole library, unfortunately this poses problems when it comes to backporting the patch. Cheers, Fred
          Hide
          Frédéric Massart added a comment -

          FYI, the S3 issue has been resolved in MDL-40615.

          Show
          Frédéric Massart added a comment - FYI, the S3 issue has been resolved in MDL-40615.
          Hide
          Dan Poltawski added a comment -

          Fred, yes we do need to backport the new library there is an issue open for it: MDL-31938

          But its a bit risky and we need some decent testing as its changes a little bit.

          Show
          Dan Poltawski added a comment - Fred, yes we do need to backport the new library there is an issue open for it: MDL-31938 But its a bit risky and we need some decent testing as its changes a little bit.
          Hide
          Thijs Kinkhorst added a comment -

          I agree that upgrading to 1.3.2 is the proper long-term solution but that is best discussed in MDL-31938. Since it has changed quite a bit I can imagine that is more something for the next release branch and not for a point update, though.

          You indeed name a number of issues. CVE-2010-* have been fixed in the phpCAS version in current Moodle already. As for CVE-2012-1104, that also applies but is a very rare use case (proxy CAS) and does not have a surgical fix. That is probably best left to the 1.3.2 upgrade. As for CVE-2012-1105, this only applies when one enables DEBUG (requires editing the source code of the CAS library) and needs untrusted local users. So I say it's not extremely important to backport, although possible.

          So my case is really that for the one issue that really is an issue, a surgical fix exists. I would therefore recommend to apply this surgical fix to STABLE as a short term fix. And in parallel, to upgrade to phpCAS 1.3.2 in trunk.

          Show
          Thijs Kinkhorst added a comment - I agree that upgrading to 1.3.2 is the proper long-term solution but that is best discussed in MDL-31938 . Since it has changed quite a bit I can imagine that is more something for the next release branch and not for a point update, though. You indeed name a number of issues. CVE-2010-* have been fixed in the phpCAS version in current Moodle already. As for CVE-2012-1104, that also applies but is a very rare use case (proxy CAS) and does not have a surgical fix. That is probably best left to the 1.3.2 upgrade. As for CVE-2012-1105, this only applies when one enables DEBUG (requires editing the source code of the CAS library) and needs untrusted local users. So I say it's not extremely important to backport, although possible. So my case is really that for the one issue that really is an issue, a surgical fix exists. I would therefore recommend to apply this surgical fix to STABLE as a short term fix. And in parallel, to upgrade to phpCAS 1.3.2 in trunk.
          Hide
          Dan Poltawski added a comment -

          +1 to your suggestion Thijs.

          Show
          Dan Poltawski added a comment - +1 to your suggestion Thijs.
          Hide
          Dan Poltawski added a comment -

          Thijs, thanks for your patch for this issue, i've wrapped it up into branches for all the supported branches.

          I've just put a comment in the testing instructions that I think its overkill to setup a SSL'd CAS server for testing this. (I don't think we have that setup at the moment).

          TO INTEGRATOR:

          • This security issue has been applied upstream and is publicly disclosed, so I believe we should apply this ASAP. Hence my public branches etc.
          • Note also MDL-31938, which will upgrades the library - which really is the best fix.
          Show
          Dan Poltawski added a comment - Thijs, thanks for your patch for this issue, i've wrapped it up into branches for all the supported branches. I've just put a comment in the testing instructions that I think its overkill to setup a SSL'd CAS server for testing this. (I don't think we have that setup at the moment). TO INTEGRATOR: This security issue has been applied upstream and is publicly disclosed, so I believe we should apply this ASAP. Hence my public branches etc. Note also MDL-31938 , which will upgrades the library - which really is the best fix.
          Hide
          Iñaki Arenaza added a comment -

          Hi Dan,

          regarding the SSL'd CAS server setup for testing this, I don't remember if they VirtualBox machine I provided HQ with had a SSL'd CAS server (although I suspect it did). But I have an updated copy of that vm that uses a private CA certificate plus a private certificate signed by that CA. I did it that way for other purpouses (trying to setup a CAS proxy setup), but it would make it easier to test this issue: you can copy the private CA certificate to the Moodle server machine and put the path of that file in 'Certificate path:' configuration setting.

          If Moodle HQ is interested in the vm, I can put it somewhere so you can download it (it's all OSS, you there's no problem with copying it).

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Dan, regarding the SSL'd CAS server setup for testing this, I don't remember if they VirtualBox machine I provided HQ with had a SSL'd CAS server (although I suspect it did). But I have an updated copy of that vm that uses a private CA certificate plus a private certificate signed by that CA. I did it that way for other purpouses (trying to setup a CAS proxy setup), but it would make it easier to test this issue: you can copy the private CA certificate to the Moodle server machine and put the path of that file in 'Certificate path:' configuration setting. If Moodle HQ is interested in the vm, I can put it somewhere so you can download it (it's all OSS, you there's no problem with copying it). Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Thanks Iñaki, i'm looking with Damyon as we've setup a few CAS installations here recently.

          Show
          Dan Poltawski added a comment - Thanks Iñaki, i'm looking with Damyon as we've setup a few CAS installations here recently.
          Hide
          Dan Poltawski added a comment -

          Hmm, so we're being a bit more through (what I should've done from the start).

          And right now, we can't get the CAS auth to fail, even with the incorrect cert and cert option, and we have verified its going down that code branch.

          Show
          Dan Poltawski added a comment - Hmm, so we're being a bit more through (what I should've done from the start). And right now, we can't get the CAS auth to fail, even with the incorrect cert and cert option, and we have verified its going down that code branch.
          Hide
          Dan Poltawski added a comment -

          I'm reopening this issue, since it seems like their remains an upstream issue?

          It seems to us, like this is missing too - (else the certificate is never verified and that _no_cas_server_validation doesn't get respected):

          diff --git a/auth/cas/CAS/CAS/client.php b/auth/cas/CAS/CAS/client.php
          index d5c4212..97ceb89 100644
          --- a/auth/cas/CAS/CAS/client.php
          +++ b/auth/cas/CAS/CAS/client.php
          @@ -2173,7 +2173,7 @@ class CASClient
                                  curl_setopt($ch, CURLOPT_CAINFO, $this->_cas_server_ca_cert);
                          } else {
                                  curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
          -                       curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, 0);
          +                       curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, !$this->_no_cas_server_validation);
                          }
          
                          // return the CURL output into a variable
          
          Show
          Dan Poltawski added a comment - I'm reopening this issue, since it seems like their remains an upstream issue? It seems to us, like this is missing too - (else the certificate is never verified and that _no_cas_server_validation doesn't get respected): diff --git a/auth/cas/CAS/CAS/client.php b/auth/cas/CAS/CAS/client.php index d5c4212..97ceb89 100644 --- a/auth/cas/CAS/CAS/client.php +++ b/auth/cas/CAS/CAS/client.php @@ -2173,7 +2173,7 @@ class CASClient curl_setopt($ch, CURLOPT_CAINFO, $ this ->_cas_server_ca_cert); } else { curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2); - curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, 0); + curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, !$ this ->_no_cas_server_validation); } // return the CURL output into a variable
          Hide
          Dan Poltawski added a comment -

          Hmm, but looking at the new version of the library it seems that this VERIFYPEER option is set to false by design under that code branch.

          In which case the VERIFY_HOST change in that final } else { branch is completely useess.

          Show
          Dan Poltawski added a comment - Hmm, but looking at the new version of the library it seems that this VERIFYPEER option is set to false by design under that code branch. In which case the VERIFY_HOST change in that final } else { branch is completely useess.
          Hide
          Iñaki Arenaza added a comment -

          Dan, I agree with you. In fact, in the new version of the library, they removed the VERIFY_HOST line completely as you say. Which IMHO makes sense. That else branch is executed when you don't provide any cert to verify the peer (neither the host cert, nor the CA cert) and you explicity set _no_cas_server_validation (by calling phpCAS::setNoCasServerValidation()). Otherwise, a phpCAS:error() would be triggered a few lines before.

          So that else branch is only executed when the cliente doesn't want to validate the CAS server at all. Setting VERIFY_PEER to 0 (and ignoring VERIFY_HOST) is the way to make that happen. So I guest that's why the new version of the library does it that way.

          +1 from me to fix this issue the same way upstream does.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Dan, I agree with you. In fact, in the new version of the library, they removed the VERIFY_HOST line completely as you say. Which IMHO makes sense. That else branch is executed when you don't provide any cert to verify the peer (neither the host cert, nor the CA cert) and you explicity set _no_cas_server_validation (by calling phpCAS::setNoCasServerValidation()). Otherwise, a phpCAS:error() would be triggered a few lines before. So that else branch is only executed when the cliente doesn't want to validate the CAS server at all. Setting VERIFY_PEER to 0 (and ignoring VERIFY_HOST) is the way to make that happen. So I guest that's why the new version of the library does it that way. +1 from me to fix this issue the same way upstream does. Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Yeah, I suppose i'd prefer it if upstream removed that line completely, but its a very old version of the library.

          Thanks for confirming my thinking.

          Show
          Dan Poltawski added a comment - Yeah, I suppose i'd prefer it if upstream removed that line completely, but its a very old version of the library. Thanks for confirming my thinking.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment -

          This looks OK - and I finished testing of it - so it will be integrated soon - but not just right now because it's a bit too late for the weekly.

          Putting back on the queue with a +1 for integration early next week.

          Notes from testing: 23 gives horrible E_STRICT errors all over the place - oh well. This should still go back to 23. master does not need this patch because we upgraded the phpcas library already. This should have the security issue label removed because this is (a) relatively minor and (b) already publicly disclosed.

          Show
          Damyon Wiese added a comment - This looks OK - and I finished testing of it - so it will be integrated soon - but not just right now because it's a bit too late for the weekly. Putting back on the queue with a +1 for integration early next week. Notes from testing: 23 gives horrible E_STRICT errors all over the place - oh well. This should still go back to 23. master does not need this patch because we upgraded the phpcas library already. This should have the security issue label removed because this is (a) relatively minor and (b) already publicly disclosed.
          Hide
          Damyon Wiese added a comment -

          Integrated to 23, 24 and 25 but not master (already has a newer version of the library).

          I'm going to pass the test for this because I tested this in integration on Friday.

          Show
          Damyon Wiese added a comment - Integrated to 23, 24 and 25 but not master (already has a newer version of the library). I'm going to pass the test for this because I tested this in integration on Friday.
          Hide
          Damyon Wiese added a comment -

          Passing test.

          Show
          Damyon Wiese added a comment - Passing test.
          Hide
          Dan Poltawski added a comment -

          Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

          Show
          Dan Poltawski added a comment - Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: