Merge lp:~dpb/landscape-charm/strip-url-newline-1411353 into lp:~landscape/landscape-charm/trunk

[David Britton]

Strip newlines from url before passing to cURL.

Strip newlines from url before passing to cURL.

[Adam Collard (adam-collard)]

Change is good, I think the tests need to be modified as suggested below, to make them easier to grok

review: Approve

[Andreas Hasenack (ahasenack)]

+1, just a minor comment about the test.

review: Approve

[lp:~dpb/landscape-charm/strip-url-newline-1411353]

218. By David Britton on 2015-01-15

sparkiegeek[diff]: check url explicitly
andreas[diff]: catch \r as well

[David Britton (dpb)]

Thanks, I've addressed all feedback.

On Thu, Jan 15, 2015 at 1:12 PM, Andreas Hasenack <email address hidden>
wrote:

> Review: Approve
>
> +1, just a minor comment about the test.
>
>
> Diff comments:
>
> > === modified file 'hooks/hooks.py'
> > --- hooks/hooks.py 2015-01-12 10:40:19 +0000
> > +++ hooks/hooks.py 2015-01-15 19:11:45 +0000
> > @@ -501,11 +501,13 @@
> > notify_website_relation()
> >
> >
> > -def _download_file(url):
> > +def _download_file(url, Curl=pycurl.Curl):
> > """ Download from a url and save to the filename given """
> > + # Fix for CVE-2014-8150, urls cannot end with newline
> > + url = url.rstrip()
> > buf = cStringIO.StringIO()
> > juju.juju_log("Fetching License: %s" % url)
> > - curl = pycurl.Curl()
> > + curl = Curl()
> > curl.setopt(pycurl.URL, str(url))
> > curl.setopt(pycurl.WRITEFUNCTION, buf.write)
> > curl.perform()
> >
> > === modified file 'hooks/test_hooks.py'
> > --- hooks/test_hooks.py 2014-12-10 22:08:57 +0000
> > +++ hooks/test_hooks.py 2015-01-15 19:11:45 +0000
> > @@ -12,6 +12,23 @@
> > import yaml
> >
> >
> > +class CurlStub(object):
> > +
> > + def __init__(self, result=None, infos=None, error=None):
> > + pass
> > +
> > + def setopt(self, option, value):
> > + if option == pycurl.URL:
> > + if "\n" in value:
>
> The curl patch (http://curl.haxx.se/CVE-2014-8150.patch) blocks both \n
> and \r, can we do the same in the test just to be consistent?
>
> > + raise AssertionError("URL cannot contain newline")
> > +
> > + def perform(self):
> > + pass
> > +
> > + def close(self):
> > + pass
> > +
> > +
> > class TestJuju(object):
> > """
> > Testing object to intercept juju calls and inject data, or make sure
> > @@ -908,13 +925,19 @@
> > filename = self.makeFile()
> > with open(filename, "w") as fp:
> > fp.write("foobar")
> > - output = hooks._download_file("file://%s" % filename)
> > + output = hooks._download_file("file:///%s" % filename)
> > self.assertIn("foobar", output)
> >
> > + def test__download_file_success_trailing_newline(self):
> > + """Test that newlines are stripped before passing to curl.
> CVE-2014-8150."""
> > + # put two newlines, since that could be a common pattern in a
> text
> > + # file when using an editor
> > + hooks._download_file("http://example.com/\n\n", Curl=CurlStub)
> > +
> > def test__download_file_failure(self):
> > """The fail path of download file raises an exception."""
> > self.assertRaises(
> > - pycurl.error, hooks._download_file, "file://FOO/NO/EXIST")
> > + pycurl.error, hooks._download_file, "file:///FOO/NO/EXIST")
> >
> > def test__replace_in_file(self):
> > """
> > @@ -962,7 +985,7 @@
> > source = self.makeFile()
> > with open(source, "w") as fp:
> > fp.write("LICENSE_FILE_TEXT from curl")
> > - hooks.juju.config["license-file"] = "file://%s" % source
> > + hooks.juju.config["license-file"] = "file:///%s" % source
> > ho...

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-01-12 10:40:19 +0000
+++ hooks/hooks.py	2015-01-15 20:51:34 +0000
@@ -501,11 +501,13 @@
     notify_website_relation()
 
 
-def _download_file(url):
+def _download_file(url, Curl=pycurl.Curl):
     """ Download from a url and save to the filename given """
+    # Fix for CVE-2014-8150, urls cannot end with newline
+    url = url.rstrip()
     buf = cStringIO.StringIO()
     juju.juju_log("Fetching License: %s" % url)
-    curl = pycurl.Curl()
+    curl = Curl()
     curl.setopt(pycurl.URL, str(url))
     curl.setopt(pycurl.WRITEFUNCTION, buf.write)
     curl.perform()
 
=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-12-10 22:08:57 +0000
+++ hooks/test_hooks.py	2015-01-15 20:51:34 +0000
@@ -12,6 +12,26 @@
 import yaml
 
 
+class CurlStub(object):
+
+    urls = []
+
+    def __init__(self, result=None, infos=None, error=None):
+        pass
+
+    def setopt(self, option, value):
+        if option == pycurl.URL:
+            if "\n" in value or "\r" in value:
+                raise AssertionError("URL cannot contain linefeed or newline")
+            self.urls.append(value)
+
+    def perform(self):
+        pass
+
+    def close(self):
+        pass
+
+
 class TestJuju(object):
     """
     Testing object to intercept juju calls and inject data, or make sure
@@ -908,13 +928,20 @@
         filename = self.makeFile()
         with open(filename, "w") as fp:
             fp.write("foobar")
-        output = hooks._download_file("file://%s" % filename)
+        output = hooks._download_file("file:///%s" % filename)
         self.assertIn("foobar", output)
 
+    def test__download_file_success_trailing_newline(self):
+        """Test that newlines are stripped before passing to curl. CVE-2014-8150."""
+        # put two newlines, since that could be a common pattern in a text
+        # file when using an editor
+        hooks._download_file("http://example.com/\n\n", Curl=CurlStub)
+        self.assertEqual(["http://example.com/"], CurlStub.urls)
+
     def test__download_file_failure(self):
         """The fail path of download file raises an exception."""
         self.assertRaises(
-            pycurl.error, hooks._download_file, "file://FOO/NO/EXIST")
+            pycurl.error, hooks._download_file, "file:///FOO/NO/EXIST")
 
     def test__replace_in_file(self):
         """
@@ -962,7 +989,7 @@
         source = self.makeFile()
         with open(source, "w") as fp:
             fp.write("LICENSE_FILE_TEXT from curl")
-        hooks.juju.config["license-file"] = "file://%s" % source
+        hooks.juju.config["license-file"] = "file:///%s" % source
         hooks._install_license()
         self.assertFileContains(
             hooks.LANDSCAPE_LICENSE_DEST, "LICENSE_FILE_TEXT from curl")