From b342111edb9c8e487f23850f40f9ba36a187c32c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 29 Jan 2018 13:30:49 -0500 Subject: [PATCH 1/2] Add registry tests for pushing and pulling previously bad repo names --- test/registry_tests.py | 10 ++++++++++ test/test_api_usage.py | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/test/registry_tests.py b/test/registry_tests.py index 47ae85256..5e880c85c 100644 --- a/test/registry_tests.py +++ b/test/registry_tests.py @@ -785,6 +785,16 @@ class V2RegistryLoginMixin(object): class RegistryTestsMixin(object): + def test_previously_bad_repo_name(self): + bad_names = ['logs', 'build', 'tokens', 'foo.bar', 'foo-bar', 'foo_bar'] + + for name in bad_names: + # Push a new repository with two layers. + self.do_push('public', name, 'public', 'password') + + # Pull the repository to verify. + self.do_pull('public', name, 'public', 'password') + def test_application_repo(self): # Create an application repository via the API. self.conduct_api_login('devtable', 'password') diff --git a/test/test_api_usage.py b/test/test_api_usage.py index e01dd0ce7..b1f8b7476 100644 --- a/test/test_api_usage.py +++ b/test/test_api_usage.py @@ -2241,7 +2241,7 @@ class TestGetRepository(ApiTestCase): def test_getrepo_badnames(self): self.login(ADMIN_ACCESS_USER) - bad_names = ['logs', 'build', 'tokens', 'foo-bar', 'foo_bar'] + bad_names = ['logs', 'build', 'tokens', 'foo.bar', 'foo-bar', 'foo_bar'] # For each bad name, create the repo. for bad_name in bad_names: From 188ea98441ba525bbf46b7159225e6d295285007 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 29 Jan 2018 14:52:50 -0500 Subject: [PATCH 2/2] Add new decorator to prevent reflected text attacks Instead of disabling repo names with periods in them, we simply disallow calls to the API when they are GET requests, whose path ends in a dot, and that do not have a referrer from the frontend. --- config.py | 4 ++++ endpoints/api/__init__.py | 5 +++-- endpoints/decorators.py | 24 ++++++++++++++++++++ endpoints/test/test_decorators.py | 35 ++++++++++++++++++++++++++++++ static/js/quay-run.ts | 7 ++++++ static/js/services/user-service.js | 3 --- test/registry_tests.py | 11 ++++------ util/config/schema.py | 5 +++++ 8 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 endpoints/test/test_decorators.py diff --git a/config.py b/config.py index 48216d1e3..c0ac210e9 100644 --- a/config.py +++ b/config.py @@ -503,3 +503,7 @@ class DefaultConfig(ImmutableConfig): # The size of pages returned by the Docker V2 API. V2_PAGINATION_SIZE = 50 + + # If enabled, ensures that API calls are made with the X-Requested-With header + # when called from a browser. + BROWSER_API_CALLS_XHR_ONLY = True diff --git a/endpoints/api/__init__.py b/endpoints/api/__init__.py index e79263cf8..f0d59913e 100644 --- a/endpoints/api/__init__.py +++ b/endpoints/api/__init__.py @@ -21,7 +21,7 @@ from auth.decorators import process_oauth from endpoints.csrf import csrf_protect from endpoints.exception import (Unauthorized, InvalidRequest, InvalidResponse, FreshLoginRequired, NotFound) -from endpoints.decorators import check_anon_protection +from endpoints.decorators import check_anon_protection, require_xhr_from_browser from util.metrics.metricqueue import time_decorator from util.names import parse_namespace_repository from util.pagination import encrypt_page_token, decrypt_page_token @@ -42,7 +42,8 @@ api = ApiExceptionHandlingApi() api.init_app(api_bp) api.decorators = [csrf_protect(), crossdomain(origin='*', headers=['Authorization', 'Content-Type']), - process_oauth, time_decorator(api_bp.name, metric_queue)] + process_oauth, time_decorator(api_bp.name, metric_queue), + require_xhr_from_browser] def resource(*urls, **kwargs): diff --git a/endpoints/decorators.py b/endpoints/decorators.py index 725dcf316..3e91a074c 100644 --- a/endpoints/decorators.py +++ b/endpoints/decorators.py @@ -1,5 +1,7 @@ """ Various decorators for endpoint and API handlers. """ +import logging + from functools import wraps from flask import abort, request, make_response @@ -8,6 +10,9 @@ import features from app import app from auth.auth_context import get_authenticated_context from util.names import parse_namespace_repository +from util.http import abort + +logger = logging.getLogger(__name__) def parse_repository_name(include_tag=False, @@ -92,3 +97,22 @@ def route_show_if(value): return f(*args, **kwargs) return decorated_function return decorator + + +def require_xhr_from_browser(func): + """ Requires that API GET calls made from browsers are made via XHR, in order to prevent + reflected text attacks. + """ + + @wraps(func) + def wrapper(*args, **kwargs): + if app.config.get('BROWSER_API_CALLS_XHR_ONLY', False): + if request.method == 'GET' and request.user_agent.browser: + has_xhr_header = request.headers.get('X-Requested-With') == 'XMLHttpRequest' + if not has_xhr_header: + logger.warning('Disallowed possible RTA to URL %s with user agent %s', + request.path, request.user_agent) + abort(400) + + return func(*args, **kwargs) + return wrapper diff --git a/endpoints/test/test_decorators.py b/endpoints/test/test_decorators.py new file mode 100644 index 000000000..e7866e25d --- /dev/null +++ b/endpoints/test/test_decorators.py @@ -0,0 +1,35 @@ +from data import model +from endpoints.api import api +from endpoints.api.repository import Repository +from endpoints.test.shared import conduct_call +from test.fixtures import * + + +@pytest.mark.parametrize('user_agent, include_header, expected_code', [ + ('curl/whatever', True, 200), + ('curl/whatever', False, 200), + + ('Mozilla/whatever', True, 200), + ('Mozilla/5.0', True, 200), + ('Mozilla/5.0 (Windows NT 5.1; Win64; x64)', False, 400), +]) +def test_require_xhr_from_browser(user_agent, include_header, expected_code, app, client): + # Create a public repo with a dot in its name. + user = model.user.get_user('devtable') + model.repository.create_repository('devtable', 'somerepo.bat', user, 'public') + + # Retrieve the repository and ensure we either allow it through or fail, depending on the + # user agent and header. + params = { + 'repository': 'devtable/somerepo.bat' + } + + headers = { + 'User-Agent': user_agent, + } + + if include_header: + headers['X-Requested-With'] = 'XMLHttpRequest' + + conduct_call(client, Repository, api.url_for, 'GET', params, headers=headers, + expected_code=expected_code) diff --git a/static/js/quay-run.ts b/static/js/quay-run.ts index c0815ef68..3df58baf0 100644 --- a/static/js/quay-run.ts +++ b/static/js/quay-run.ts @@ -11,6 +11,7 @@ provideRun.$inject = [ 'PlanService', '$http', 'CookieService', + 'UserService', 'Features', '$anchorScroll', 'MetaService', @@ -20,6 +21,7 @@ export function provideRun($rootScope: QuayRunScope, planService: any, $http: ng.IHttpService, cookieService: any, + userService: any, features: any, $anchorScroll: ng.IAnchorScrollService, metaService: any): void { @@ -29,6 +31,8 @@ export function provideRun($rootScope: QuayRunScope, restangular.setDefaultRequestParams(['post', 'put', 'remove', 'delete'], {'_csrf_token': (window).__token || ''}); + restangular.setDefaultHeaders({'X-Requested-With': 'XMLHttpRequest'}); + // Handle session expiration. restangular.setErrorInterceptor(function(response) { if (response !== undefined && response.status == 503) { @@ -120,6 +124,9 @@ export function provideRun($rootScope: QuayRunScope, } return $http.pendingRequests.length > 0; }; + + // Load the inital user information. + userService.load(); } diff --git a/static/js/services/user-service.js b/static/js/services/user-service.js index 64e75828b..1196bf030 100644 --- a/static/js/services/user-service.js +++ b/static/js/services/user-service.js @@ -251,8 +251,5 @@ function(ApiService, CookieService, $rootScope, Config, $location, $timeout) { // Update the user in the root scope. userService.updateUserIn($rootScope); - // Load the user the first time. - userService.load(); - return userService; }]); diff --git a/test/registry_tests.py b/test/registry_tests.py index 5e880c85c..a897968cf 100644 --- a/test/registry_tests.py +++ b/test/registry_tests.py @@ -786,14 +786,11 @@ class V2RegistryLoginMixin(object): class RegistryTestsMixin(object): def test_previously_bad_repo_name(self): - bad_names = ['logs', 'build', 'tokens', 'foo.bar', 'foo-bar', 'foo_bar'] + # Push a new repository with two layers. + self.do_push('public', 'foo.bar', 'public', 'password') - for name in bad_names: - # Push a new repository with two layers. - self.do_push('public', name, 'public', 'password') - - # Pull the repository to verify. - self.do_pull('public', name, 'public', 'password') + # Pull the repository to verify. + self.do_pull('public', 'foo.bar', 'public', 'password') def test_application_repo(self): # Create an application repository via the API. diff --git a/util/config/schema.py b/util/config/schema.py index 7fcc11d9c..64b8e58f4 100644 --- a/util/config/schema.py +++ b/util/config/schema.py @@ -591,6 +591,11 @@ CONFIG_SCHEMA = { 'not authenticated as a superuser', 'x-example': 'somesecrethere', }, + 'BROWSER_API_CALLS_XHR_ONLY': { + 'type': 'boolean', + 'description': 'If enabled, only API calls marked as being made by an XHR will be allowed from browsers. Defaults to True.', + 'x-example': False, + }, # Time machine and tag expiration settings. 'FEATURE_CHANGE_TAG_EXPIRATION': {