-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixes #9120 - Log Injection Vulnerability in Authentication Endpoint #9147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Changes from all commits
02557b8
bbebd26
f829cff
11671d4
6c66273
30914c5
7b8c8f4
c64b294
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||
| import base64 | ||||||
| import logging | ||||||
| import random | ||||||
| import re | ||||||
| import string | ||||||
| from datetime import timedelta | ||||||
| from functools import wraps | ||||||
|
|
@@ -51,6 +52,27 @@ | |||||
| auth_routes = Blueprint('auth', __name__, url_prefix='/v1/auth') | ||||||
|
|
||||||
|
|
||||||
| def sanitize_for_logging(text): | ||||||
| r""" | ||||||
| Remove control characters from user input before logging to prevent log injection. | ||||||
|
|
||||||
| Security Issue #9120: User-provided data like emails can contain newlines, carriage | ||||||
| returns, or tabs that allow attackers to inject false log entries, corrupt log files, | ||||||
| bypass log analysis tools, or hide malicious activity. | ||||||
|
|
||||||
| Example Attack: email="user@test.com\nFAKE: Admin login successful from 1.2.3.4" | ||||||
|
|
||||||
| Args: | ||||||
| text (str): User-provided input to sanitize | ||||||
|
|
||||||
| Returns: | ||||||
| str: Text with control characters (\n, \r, \t) removed | ||||||
| """ | ||||||
| if not text: | ||||||
| return text | ||||||
| return re.sub(r'[\n\r\t]', '', text) | ||||||
|
|
||||||
|
|
||||||
| def authenticate(allow_refresh_token=False, existing_identity=None): | ||||||
| data = request.get_json() | ||||||
| username = data.get('email', data.get('username')) | ||||||
|
|
@@ -81,14 +103,14 @@ def authenticate(allow_refresh_token=False, existing_identity=None): | |||||
| response_data = {'access_token': access_token} | ||||||
|
|
||||||
| if add_refresh_token: | ||||||
| refresh_token = create_refresh_token(identity.id) | ||||||
| refresh_token_value = create_refresh_token(identity.id) | ||||||
| if include_in_response: | ||||||
| response_data['refresh_token'] = refresh_token | ||||||
| response_data['refresh_token'] = refresh_token_value | ||||||
|
|
||||||
| response = jsonify(response_data) | ||||||
|
|
||||||
| if add_refresh_token and not include_in_response: | ||||||
| set_refresh_cookies(response, refresh_token) | ||||||
| set_refresh_cookies(response, refresh_token_value) | ||||||
|
|
||||||
| return response | ||||||
|
|
||||||
|
|
@@ -108,10 +130,10 @@ def fresh_login(): | |||||
| @auth_routes.route('/token/refresh', methods=['POST']) | ||||||
| @jwt_refresh_token_required | ||||||
| def refresh_token(): | ||||||
| current_user = get_jwt_identity() | ||||||
| user_identity = get_jwt_identity() | ||||||
| expiry_time = timedelta(minutes=90) | ||||||
| new_token = create_access_token( | ||||||
| identity=current_user, fresh=False, expires_delta=expiry_time | ||||||
| identity=user_identity, fresh=False, expires_delta=expiry_time | ||||||
| ) | ||||||
| return jsonify({'access_token': new_token}) | ||||||
|
|
||||||
|
|
@@ -302,11 +324,11 @@ def verify_email(): | |||||
| except Exception: | ||||||
| logging.error('Invalid Token') | ||||||
| raise BadRequestError({'source': ''}, 'Invalid Token') | ||||||
| else: | ||||||
| user.is_verified = True | ||||||
| save_to_db(user) | ||||||
| logging.info('Email Verified') | ||||||
| return make_response(jsonify(message="Email Verified"), 200) | ||||||
|
|
||||||
| user.is_verified = True | ||||||
| save_to_db(user) | ||||||
| logging.info('Email Verified') | ||||||
| return make_response(jsonify(message="Email Verified"), 200) | ||||||
|
|
||||||
|
|
||||||
| @auth_routes.route('/resend-verification-email', methods=['POST']) | ||||||
|
|
@@ -320,22 +342,24 @@ def resend_verification_email(): | |||||
| try: | ||||||
| user = User.query.filter_by(email=email).one() | ||||||
| except NoResultFound: | ||||||
| logging.info('User with email: ' + email + ' not found.') | ||||||
| # Sanitize email to prevent log injection (Issue #9120) | ||||||
| safe_email = sanitize_for_logging(email) | ||||||
| logging.info('User with email: %s not found.', safe_email) | ||||||
| raise UnprocessableEntityError( | ||||||
| {'source': ''}, 'User with email: ' + email + ' not found.' | ||||||
|
||||||
| {'source': ''}, 'User with email: ' + email + ' not found.' | |
| {'source': ''}, 'User with email: ' + safe_email + ' not found.' |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,68 @@ | ||||||
| """ | ||||||
| Test for log injection vulnerability in auth.py (Issue #9120) | ||||||
| Tests that user-provided email addresses cannot inject malicious content into logs | ||||||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Consider aligning the test type with its location (integration vs unit) or expanding it to true integration coverage This file is under Suggested implementation: """
Tests for log injection vulnerability in auth.py (Issue #9120).
This module lives under `tests/all/integration/api/` and therefore provides
true integration coverage by exercising the `/v1/auth/resend-verification-email`
endpoint and asserting on what is written to the logging subsystem. It may
also contain focused unit-style assertions over the sanitization helpers to
guard against regressions in the underlying regex logic.
"""import logging
import re
def test_resend_verification_email_log_sanitization(api_client, caplog):
"""
Integration test for log injection vulnerability (Issue #9120).
Sends a request to the resend-verification-email endpoint with a potentially
malicious email address and verifies that the value written to logs has
been sanitized (no raw control characters that could be used for log
injection), while the base email content is still present.
"""
malicious_email = "attacker@example.com\nERROR: forged entry\n\tat fake_traceback"
# This should mirror the behavior of `sanitize_for_logging` in auth.py so
# the expectation here stays aligned with the implementation.
sanitized_email = re.sub(r"[\r\n\t]", "", malicious_email)
with caplog.at_level(logging.INFO):
response = api_client.post(
"/v1/auth/resend-verification-email",
json={"email": malicious_email},
)
# We don't assert a specific status code here because different environments
# may have different user/account states; we just require that the endpoint
# responds successfully at the HTTP layer and does not error out.
assert 200 <= response.status_code < 500
log_output = "\n".join(caplog.messages)
# Raw, unsanitized value must never appear in logs.
assert malicious_email not in log_output
# Sanitized value should be what actually gets logged.
assert sanitized_email in log_output
def test_log_sanitization_for_email():To wire this up cleanly in your codebase, you may need to:
|
||||||
| """ | ||||||
| import re | ||||||
|
|
||||||
|
|
||||||
| def test_log_sanitization_for_email(): | ||||||
| """ | ||||||
| Unit test for log injection vulnerability (Issue #9120) | ||||||
|
|
||||||
| Tests that email addresses with injection characters are properly sanitized | ||||||
| before being logged to prevent log file corruption. | ||||||
|
|
||||||
| Security Impact: Without sanitization, attackers can: | ||||||
| - Inject false log entries (e.g., fake admin logins) | ||||||
| - Corrupt log file structure | ||||||
| - Bypass log analysis tools | ||||||
| - Hide malicious activity | ||||||
|
|
||||||
| Example attack: email="user@test.com\nFAKE: Admin login successful" | ||||||
| """ | ||||||
| # Simulate the vulnerable code pattern from auth.py line 323 | ||||||
| malicious_inputs = [ | ||||||
| "test@example.com\nFAKE: Admin logged in from 1.2.3.4", | ||||||
| "test@example.com\rFAKE: Password reset", | ||||||
| "test@example.com\t\t\tFAKE_COLUMN", | ||||||
| "test@example.com\n\rMultiline\nInjection\rAttempt", | ||||||
| ] | ||||||
|
|
||||||
| for malicious_email in malicious_inputs: | ||||||
| # This represents the VULNERABLE code pattern: | ||||||
| # logging.info('User with email: ' + email + ' not found.') | ||||||
|
|
||||||
| # Vulnerability demonstration: raw concatenation allows injection | ||||||
| vulnerable_log_message = 'User with email: ' + malicious_email + ' not found.' | ||||||
|
|
||||||
| # Check 1: Vulnerable pattern contains control characters (SECURITY ISSUE) | ||||||
| has_injection = any(char in vulnerable_log_message for char in ['\n', '\r', '\t\t\t']) | ||||||
|
||||||
| has_injection = any(char in vulnerable_log_message for char in ['\n', '\r', '\t\t\t']) | |
| has_injection = any(char in vulnerable_log_message for char in ['\n', '\r', '\t']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): We've found these issues:
reintroduce-else)assign-if-exp)