Skip to content

Security: gate X-Forwarded-For behind trusted proxies with strict IP#2049

Open
ciphershade7 wants to merge 1 commit into
utksh1:mainfrom
ciphershade7:patch-rate-limiter-trusted-proxies
Open

Security: gate X-Forwarded-For behind trusted proxies with strict IP#2049
ciphershade7 wants to merge 1 commit into
utksh1:mainfrom
ciphershade7:patch-rate-limiter-trusted-proxies

Conversation

@ciphershade7

Copy link
Copy Markdown

Description

This PR addresses a security vulnerability where ScanRateLimiter unconditionally trusted the first IP in the X-Forwarded-For header allowing an attacker to spoof it to bypass rate limits or purposefully get innocent victim IPs blocked.

The update:

  1. It checks request.client.host against settings.trusted_proxies before trusting X-Forwarded-For.
  2. It uses ip address to validate the extracted header values, so any deformed input will be ignored safely.
  3. It sets the direct client IP to None until the end, then applies the final fallback string... eg. unknown_client.

Issue

#1757

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

Ran the targeted rate limiter test suite via "pytest testing/backend/test_rate_limiter.py -v"

tests added to verify the fix:

  1. "test_ignores_forwarded_for_from_untrusted_proxy": verifies that when the direct connection is not in trusted_proxies, X-Forwarded-For is ignored and the direct IP is tracked.
  2. "test_ignores_malformed_forwarded_for_and_uses_proxy_ip_fallback": simulates a trusted proxy sending a deformed forwarded IP ("not-an-ip") and verifies the value error is swallowed so the limiter falls back to tracking the proxy’s IP.

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant