URL preview blacklisting fixes (#5155)

Prevents a SynapseError being raised inside of a IResolutionReceiver and instead opts to just return 0 results. This thus means that we have to lump a failed lookup and a blacklisted lookup together with the same error message, but the substitute should be generic enough to cover both cases.
This commit is contained in:
Andrew Morgan 2019-05-10 10:32:44 -07:00 committed by GitHub
parent c2bb7476c9
commit 2f48c4e1ae
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 47 additions and 31 deletions

1
changelog.d/5155.misc Normal file
View file

@ -0,0 +1 @@
Prevent an exception from being raised in a IResolutionReceiver and use a more generic error message for blacklisted URL previews.

View file

@ -90,9 +90,32 @@ class IPBlacklistingResolver(object):
def resolveHostName(self, recv, hostname, portNumber=0): def resolveHostName(self, recv, hostname, portNumber=0):
r = recv() r = recv()
d = defer.Deferred()
addresses = [] addresses = []
def _callback():
r.resolutionBegan(None)
has_bad_ip = False
for i in addresses:
ip_address = IPAddress(i.host)
if check_against_blacklist(
ip_address, self._ip_whitelist, self._ip_blacklist
):
logger.info(
"Dropped %s from DNS resolution to %s due to blacklist" %
(ip_address, hostname)
)
has_bad_ip = True
# if we have a blacklisted IP, we'd like to raise an error to block the
# request, but all we can really do from here is claim that there were no
# valid results.
if not has_bad_ip:
for i in addresses:
r.addressResolved(i)
r.resolutionComplete()
@provider(IResolutionReceiver) @provider(IResolutionReceiver)
class EndpointReceiver(object): class EndpointReceiver(object):
@staticmethod @staticmethod
@ -101,34 +124,16 @@ class IPBlacklistingResolver(object):
@staticmethod @staticmethod
def addressResolved(address): def addressResolved(address):
ip_address = IPAddress(address.host)
if check_against_blacklist(
ip_address, self._ip_whitelist, self._ip_blacklist
):
logger.info(
"Dropped %s from DNS resolution to %s" % (ip_address, hostname)
)
raise SynapseError(403, "IP address blocked by IP blacklist entry")
addresses.append(address) addresses.append(address)
@staticmethod @staticmethod
def resolutionComplete(): def resolutionComplete():
d.callback(addresses) _callback()
self._reactor.nameResolver.resolveHostName( self._reactor.nameResolver.resolveHostName(
EndpointReceiver, hostname, portNumber=portNumber EndpointReceiver, hostname, portNumber=portNumber
) )
def _callback(addrs):
r.resolutionBegan(None)
for i in addrs:
r.addressResolved(i)
r.resolutionComplete()
d.addCallback(_callback)
return r return r

View file

@ -31,6 +31,7 @@ from six.moves import urllib_parse as urlparse
from canonicaljson import json from canonicaljson import json
from twisted.internet import defer from twisted.internet import defer
from twisted.internet.error import DNSLookupError
from twisted.web.resource import Resource from twisted.web.resource import Resource
from twisted.web.server import NOT_DONE_YET from twisted.web.server import NOT_DONE_YET
@ -328,9 +329,18 @@ class PreviewUrlResource(Resource):
# handler will return a SynapseError to the client instead of # handler will return a SynapseError to the client instead of
# blank data or a 500. # blank data or a 500.
raise raise
except DNSLookupError:
# DNS lookup returned no results
# Note: This will also be the case if one of the resolved IP
# addresses is blacklisted
raise SynapseError(
502, "DNS resolution failure during URL preview generation",
Codes.UNKNOWN
)
except Exception as e: except Exception as e:
# FIXME: pass through 404s and other error messages nicely # FIXME: pass through 404s and other error messages nicely
logger.warn("Error downloading %s: %r", url, e) logger.warn("Error downloading %s: %r", url, e)
raise SynapseError( raise SynapseError(
500, "Failed to download content: %s" % ( 500, "Failed to download content: %s" % (
traceback.format_exception_only(sys.exc_info()[0], e), traceback.format_exception_only(sys.exc_info()[0], e),

View file

@ -297,12 +297,12 @@ class URLPreviewTests(unittest.HomeserverTestCase):
# No requests made. # No requests made.
self.assertEqual(len(self.reactor.tcpClients), 0) self.assertEqual(len(self.reactor.tcpClients), 0)
self.assertEqual(channel.code, 403) self.assertEqual(channel.code, 502)
self.assertEqual( self.assertEqual(
channel.json_body, channel.json_body,
{ {
'errcode': 'M_UNKNOWN', 'errcode': 'M_UNKNOWN',
'error': 'IP address blocked by IP blacklist entry', 'error': 'DNS resolution failure during URL preview generation',
}, },
) )
@ -318,12 +318,12 @@ class URLPreviewTests(unittest.HomeserverTestCase):
request.render(self.preview_url) request.render(self.preview_url)
self.pump() self.pump()
self.assertEqual(channel.code, 403) self.assertEqual(channel.code, 502)
self.assertEqual( self.assertEqual(
channel.json_body, channel.json_body,
{ {
'errcode': 'M_UNKNOWN', 'errcode': 'M_UNKNOWN',
'error': 'IP address blocked by IP blacklist entry', 'error': 'DNS resolution failure during URL preview generation',
}, },
) )
@ -339,7 +339,6 @@ class URLPreviewTests(unittest.HomeserverTestCase):
# No requests made. # No requests made.
self.assertEqual(len(self.reactor.tcpClients), 0) self.assertEqual(len(self.reactor.tcpClients), 0)
self.assertEqual(channel.code, 403)
self.assertEqual( self.assertEqual(
channel.json_body, channel.json_body,
{ {
@ -347,6 +346,7 @@ class URLPreviewTests(unittest.HomeserverTestCase):
'error': 'IP address blocked by IP blacklist entry', 'error': 'IP address blocked by IP blacklist entry',
}, },
) )
self.assertEqual(channel.code, 403)
def test_blacklisted_ip_range_direct(self): def test_blacklisted_ip_range_direct(self):
""" """
@ -414,12 +414,12 @@ class URLPreviewTests(unittest.HomeserverTestCase):
) )
request.render(self.preview_url) request.render(self.preview_url)
self.pump() self.pump()
self.assertEqual(channel.code, 403) self.assertEqual(channel.code, 502)
self.assertEqual( self.assertEqual(
channel.json_body, channel.json_body,
{ {
'errcode': 'M_UNKNOWN', 'errcode': 'M_UNKNOWN',
'error': 'IP address blocked by IP blacklist entry', 'error': 'DNS resolution failure during URL preview generation',
}, },
) )
@ -439,12 +439,12 @@ class URLPreviewTests(unittest.HomeserverTestCase):
# No requests made. # No requests made.
self.assertEqual(len(self.reactor.tcpClients), 0) self.assertEqual(len(self.reactor.tcpClients), 0)
self.assertEqual(channel.code, 403) self.assertEqual(channel.code, 502)
self.assertEqual( self.assertEqual(
channel.json_body, channel.json_body,
{ {
'errcode': 'M_UNKNOWN', 'errcode': 'M_UNKNOWN',
'error': 'IP address blocked by IP blacklist entry', 'error': 'DNS resolution failure during URL preview generation',
}, },
) )
@ -460,11 +460,11 @@ class URLPreviewTests(unittest.HomeserverTestCase):
request.render(self.preview_url) request.render(self.preview_url)
self.pump() self.pump()
self.assertEqual(channel.code, 403) self.assertEqual(channel.code, 502)
self.assertEqual( self.assertEqual(
channel.json_body, channel.json_body,
{ {
'errcode': 'M_UNKNOWN', 'errcode': 'M_UNKNOWN',
'error': 'IP address blocked by IP blacklist entry', 'error': 'DNS resolution failure during URL preview generation',
}, },
) )