Skip to content
Permalink
Browse files Browse the repository at this point in the history
CVE-2019-19775: Close open redirect in thumbnail view.
This closes an open redirect vulnerability, one case of which was
found by Graham Bleaney and Ibrahim Mohamed using Pysa.

Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
  • Loading branch information
andersk authored and timabbott committed Dec 13, 2019
1 parent 1bf0a92 commit b7c87a4
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 10 deletions.
3 changes: 2 additions & 1 deletion zerver/lib/bugdown/__init__.py
Expand Up @@ -9,6 +9,7 @@
import logging
import traceback
import urllib
import urllib.parse
import re
import os
import html
Expand Down Expand Up @@ -553,7 +554,7 @@ def run(self, root: Element) -> None:
found_imgs = walk_tree(root, lambda e: e if e.tag == "img" else None)
for img in found_imgs:
url = img.get("src")
if not url.startswith("http://"):
if urllib.parse.urlsplit(url).scheme != "http":
# Don't rewrite images on our own site (e.g. emoji).
continue
img.set("src", get_camo_url(url))
Expand Down
18 changes: 10 additions & 8 deletions zerver/lib/thumbnail.py
Expand Up @@ -4,6 +4,7 @@
import os
import sys
import urllib
from urllib.parse import urljoin, urlsplit, urlunsplit
from django.conf import settings
from libthumbor import CryptoURL

Expand All @@ -19,7 +20,8 @@ def is_thumbor_enabled() -> bool:
return settings.THUMBOR_URL != ''

def user_uploads_or_external(url: str) -> bool:
return url.startswith('http') or url.lstrip('/').startswith('user_uploads/')
u = urlsplit(url)
return u.scheme != "" or u.netloc != "" or u.path.startswith("/user_uploads/")

def get_source_type(url: str) -> str:
if not url.startswith('/user_uploads/'):
Expand All @@ -33,16 +35,16 @@ def get_source_type(url: str) -> str:
def generate_thumbnail_url(path: str,
size: str='0x0',
is_camo_url: bool=False) -> str:
if not (path.startswith('https://') or path.startswith('http://')):
path = '/' + path
path = urljoin("/", path)
u = urlsplit(path)

if not is_thumbor_enabled():
if path.startswith('http://'):
return get_camo_url(path)
return path
if u.scheme == "" and u.netloc == "":
return urlunsplit(u)
return get_camo_url(path)

if not user_uploads_or_external(path):
return path
if u.scheme == "" and u.netloc == "" and not u.path.startswith("/user_uploads/"):
return urlunsplit(u)

source_type = get_source_type(path)
safe_url = base64.urlsafe_b64encode(path.encode()).decode('utf-8')
Expand Down
14 changes: 13 additions & 1 deletion zerver/tests/test_thumbnail.py
Expand Up @@ -148,6 +148,9 @@ def run_test_with_image_url(image_url: str) -> None:
image_url = 'http://images.foobar.com/12345'
run_test_with_image_url(image_url)

image_url = '//images.foobar.com/12345'
run_test_with_image_url(image_url)

def test_local_file_type(self) -> None:
def get_file_path_urlpart(uri: str, size: str='') -> str:
url_in_result = 'smart/filters:no_upscale()%s/%s/source_type/local_file'
Expand Down Expand Up @@ -281,7 +284,8 @@ def test_with_thumbor_disabled(self) -> None:
with self.settings(THUMBOR_URL=''):
result = self.client_get("/thumbnail?url=%s&size=full" % (quoted_uri))
self.assertEqual(result.status_code, 302, result)
self.assertEqual(uri, result.url)
base = 'https://external-content.zulipcdn.net/external_content/56c362a24201593891955ff526b3b412c0f9fcd2/68747470733a2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67'
self.assertEqual(base, result.url)

uri = 'http://www.google.com/images/srpr/logo4w.png'
quoted_uri = urllib.parse.quote(uri, safe='')
Expand All @@ -291,6 +295,14 @@ def test_with_thumbor_disabled(self) -> None:
base = 'https://external-content.zulipcdn.net/external_content/7b6552b60c635e41e8f6daeb36d88afc4eabde79/687474703a2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67'
self.assertEqual(base, result.url)

uri = '//www.google.com/images/srpr/logo4w.png'
quoted_uri = urllib.parse.quote(uri, safe='')
with self.settings(THUMBOR_URL=''):
result = self.client_get("/thumbnail?url=%s&size=full" % (quoted_uri,))
self.assertEqual(result.status_code, 302, result)
base = 'https://external-content.zulipcdn.net/external_content/676530cf4b101d56f56cc4a37c6ef4d4fd9b0c03/2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67'
self.assertEqual(base, result.url)

def test_with_different_THUMBOR_URL(self) -> None:
self.login(self.example_email("hamlet"))
fp = StringIO("zulip!")
Expand Down

0 comments on commit b7c87a4

Please sign in to comment.