Fix CSRF handling and improve concurrency in bruteforce script #36
Conversation
Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the bruteforce script to improve CSRF token handling and implement concurrent password testing. The changes address CSRF bypass issues by creating fresh sessions for each login attempt and replace single-threaded execution with ThreadPoolExecutor for better performance.
Changes:
- Added concurrent password testing using ThreadPoolExecutor (10 workers) instead of sequential testing
- Fixed CSRF token handling by creating a new session per attempt and passing session to get_csrf_token()
- Made username and password field names configurable via user input
- Removed artificial delay in banner printing and simplified banner display
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sys.stdout.flush() | ||
| time.sleep(0.02) | ||
| # Display banner without sleep | ||
| print(banner) |
There was a problem hiding this comment.
The banner variable is referenced here but it's not defined until the bottom of the file (line 193). This will cause a NameError when the BruteForceCracker class is instantiated. The banner should be defined before it's used, or this line should be removed since the banner is already printed at the top of the file (lines 8-20).
| # Cancel remaining futures if possible (Python 3.9+) | ||
| if sys.version_info >= (3, 9): | ||
| executor.shutdown(wait=False, cancel_futures=True) | ||
| break |
There was a problem hiding this comment.
The executor.shutdown() method with cancel_futures parameter is only available in Python 3.9+, but there's no fallback mechanism for earlier versions. When a password is found on Python < 3.9, the code will simply break from the loop but all submitted futures in the current batch will still execute, potentially wasting resources. Consider implementing a proper cancellation mechanism or using a shared flag that the crack_password_wrapper function checks before doing work.
bruteforce.py
Outdated
| print("\n[+] Success!") | ||
| print("Username: ---> " + self.username) | ||
| print("Password: ---> " + password) | ||
| return True |
There was a problem hiding this comment.
There's a potential race condition here. Multiple threads might find the correct password simultaneously and all print the success message before the first one sets found=True and breaks the loop. This could lead to duplicate success messages and confusion about which password was correct. Consider using a thread-safe mechanism like a threading.Event or checking the found flag before printing the success message.
| t.start() | ||
| t.join() | ||
|
|
||
| futures = {executor.submit(crack_password_wrapper, pwd, cracker, counter_lock, counter): pwd for pwd in batch} |
There was a problem hiding this comment.
The futures dictionary maps futures to passwords, but the password value is never used. The password is already returned by the future.result() call. This creates unnecessary data storage. Consider using a set or list instead: futures = [executor.submit(crack_password_wrapper, pwd, cracker, counter_lock, counter) for pwd in batch]
| futures = {executor.submit(crack_password_wrapper, pwd, cracker, counter_lock, counter): pwd for pwd in batch} | |
| futures = [executor.submit(crack_password_wrapper, pwd, cracker, counter_lock, counter) for pwd in batch] |
| f = open("passwords.txt", "r") | ||
| except FileNotFoundError: | ||
| print("Error: passwords.txt not found.") | ||
| return | ||
|
|
||
| counter = [0] | ||
| counter_lock = threading.Lock() | ||
|
|
||
| batch_size = 100 | ||
| max_workers = 10 | ||
| found = False | ||
|
|
||
| with ThreadPoolExecutor(max_workers=max_workers) as executor: | ||
| while True: | ||
| passwords = f.readlines(chunk_size) | ||
| if not passwords: | ||
| # Read a batch of passwords | ||
| batch = [] | ||
| for _ in range(batch_size): | ||
| line = f.readline() | ||
| if not line: | ||
| break | ||
| batch.append(line) | ||
|
|
||
| if not batch: | ||
| break | ||
| t = threading.Thread(target=crack_passwords, args=(passwords, cracker)) | ||
| t.start() | ||
| t.join() | ||
|
|
||
| futures = {executor.submit(crack_password_wrapper, pwd, cracker, counter_lock, counter): pwd for pwd in batch} | ||
|
|
||
| for future in as_completed(futures): | ||
| success, password = future.result() | ||
| if success: | ||
| found = True | ||
| print(f"Password found! Stopping...") | ||
| # Cancel remaining futures if possible (Python 3.9+) | ||
| if sys.version_info >= (3, 9): | ||
| executor.shutdown(wait=False, cancel_futures=True) | ||
| break | ||
|
|
||
| if found: | ||
| break | ||
|
|
||
| f.close() |
There was a problem hiding this comment.
The file is opened but if an exception occurs in the ThreadPoolExecutor block (lines 160-186), the file.close() on line 188 won't be executed. Consider using a context manager (with statement) or a try/finally block to ensure the file is properly closed even if an exception occurs.
bruteforce.py
Outdated
| # Check if login was successful | ||
| if self.error_message in str(response.content): | ||
| # Check if login was successful | ||
| if self.error_message in str(response.content) or self.error_message in response.text: |
There was a problem hiding this comment.
Converting response.content to string and also checking response.text is redundant and inefficient. The str(response.content) creates a string representation that includes the byte prefix (e.g., "b'...'"), which may not match the error_message correctly. It's more efficient and correct to check only response.text, which is the decoded string content.
| if self.error_message in str(response.content) or self.error_message in response.text: | |
| if self.error_message in response.text: |
| for future in as_completed(futures): | ||
| success, password = future.result() | ||
| if success: | ||
| found = True | ||
| print(f"Password found! Stopping...") | ||
| # Cancel remaining futures if possible (Python 3.9+) | ||
| if sys.version_info >= (3, 9): | ||
| executor.shutdown(wait=False, cancel_futures=True) | ||
| break |
There was a problem hiding this comment.
When breaking from the as_completed loop after finding a password, there may still be unprocessed futures in the current batch. The code breaks out of the inner loop (line 183) but doesn't cancel or wait for the remaining futures, which means they'll continue to execute in the background. This could lead to additional network requests and potential duplicate success messages if another password also succeeds. Consider canceling remaining futures or using a shared stop flag.
| data_dict = { | ||
| self.username_field: self.username, | ||
| self.password_field: password, | ||
| "Log In": "submit" |
There was a problem hiding this comment.
The "Log In" submit field is hardcoded while the username and password field names are now configurable. This inconsistency could cause issues with forms that use different submit button names or don't require a submit field at all. Consider making the submit field name configurable as well, or removing it if it's not universally required.
Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
- Added CSRF token extraction and session handling. - Implemented ThreadPoolExecutor for concurrency. - Added pre-flight check to validate error message configuration. - Added fallback logic to detect login failures using password field presence. - Allowed custom field names for username and password. Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
- Added CSRF token extraction and session handling. - Implemented ThreadPoolExecutor for concurrency. - Added pre-flight check to validate error message configuration. - Added fallback logic to detect login failures using password field presence. - Allowed custom field names for username and password. - Updated output to show each username/password attempt. Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
- Added CSRF token extraction and session handling. - Implemented ThreadPoolExecutor for concurrency. - Added pre-flight check to validate error message configuration. - Added URL redirection check to robustly identify successful logins. - Added fallback logic to detect login failures using password field presence. - Allowed custom field names for username and password. - Updated output to show each username/password attempt. Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
- Added CSRF token extraction and session handling. - Implemented ThreadPoolExecutor for concurrency. - Added pre-flight check to validate error message configuration. - Added URL redirection check to robustly identify successful logins. - Added fallback logic to detect login failures using password field presence. - Allowed custom field names for username and password. - Updated output to show each username/password attempt. - Added User-Agent and Referer headers to bypass WAF/Framework checks. Co-authored-by: Antu7 <25433660+Antu7@users.noreply.github.com>
Updated bruteforce.py to correctly handle CSRF tokens by creating a new session for each attempt.
Replaced the single-threaded manual loop with ThreadPoolExecutor for better concurrency.
Added inputs for custom username and password field names.
Removed artificial delay in banner printing.
PR created automatically by Jules for task 18009926677562195797 started by @Antu7