Do not clobber the "tcp" dialer for MySQL #5681
Conversation
This introduces a "nucleitcp" protocol that Nuclei will use when making MySQL connections as part of its templates. Previously, this would register (and de-register!) a custom "tcp" dialer, and that applied globally, so any piece of software that used a MySQL database and included nuclei in SDK mode would have its database connections ripped out from under it due to the dialer hijacking. By using "nucleitcp" as the protocol, we are free to do whatever we want with the dialer and not impact any other packages. Within our `BuildDSN` function, we quietly replace the protocol to "nucleitcp" if it was "tcp", so nuclei developers don't have to do anything special to use this functionality; it will always do it.
|
Thanks so much for your contribution @doug-threatmate ! :) |
|
Thanks for the PR! Couldn't do it, could you do a rebase? |
There was a problem hiding this comment.
@doug-threatmate can you rebase this from dev to ensure build pipeline is working?
WalkthroughThe pull request introduces modifications to the MySQL connection handling in two files. In Changes
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Merged and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/js/libs/mysql/mysql_private.go(1 hunks)pkg/protocols/common/protocolstate/state.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (4)
pkg/js/libs/mysql/mysql_private.go (2)
49-54: LGTM! Protocol switching implementation looks good.The implementation correctly switches from "tcp" to "nucleitcp" protocol while maintaining backward compatibility. This change aligns with the PR objective of preventing interference with other packages using MySQL in SDK mode.
49-54: Verify impact on existing MySQL connections.Let's ensure this change doesn't affect other protocol types and existing MySQL connections.
✅ Verification successful
Protocol switch to "nucleitcp" is safe and intentional
The change is a deliberate design choice to prevent interference with the default TCP dialer when nuclei is used as a package in other applications. The switch is transparent to users, only affects TCP connections, and has proper custom dialer registration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MySQL connection strings in the codebase to verify impact echo "Checking MySQL DSN usage patterns:" rg -g '!{vendor,.*_test.go}' '(?i)mysql:\/\/|mysql.*DSN' echo -e "\nChecking for other protocol types in MySQL connections:" rg -g '!{vendor,.*_test.go}' 'Protocol.*=.*"(?!tcp|nucleitcp)"'Length of output: 682
Script:
#!/bin/bash # Search for MySQL protocol usage echo "Checking MySQL protocol usage:" rg -A 3 'Protocol.*=.*"' pkg/ echo -e "\nChecking nucleitcp references:" rg -A 3 'nucleitcp' echo -e "\nChecking MySQL connection related files:" fd -e go mysqlLength of output: 2926
pkg/protocols/common/protocolstate/state.go (2)
157-166: LGTM! Custom dialer registration with default port handling.The implementation correctly:
- Registers "nucleitcp" protocol to avoid interfering with the default "tcp" dialer
- Adds default MySQL port (3306) when not specified
- Uses the existing Dialer for TCP connections
The comments clearly explain the rationale behind using a different protocol name, which helps with future maintenance.
157-166: Verify thread safety of dialer registration.Let's ensure the dialer registration is thread-safe and won't cause race conditions.
✅ Verification successful
The dialer registration implementation is thread-safe.
The registration occurs once during package initialization, uses a unique protocol name "nucleitcp", and shows no evidence of concurrent access or runtime modifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for concurrent access to mysql.RegisterDialContext echo "Checking for concurrent dialer registration:" rg -A 5 'RegisterDialContext.*nucleitcp' echo -e "\nChecking for other goroutines that might access the dialer:" rg 'go\s+func.*mysql.*Dial'Length of output: 877
Error: No shell scripts found
Script:
#!/bin/bash # Check for any other mysql package imports and initialization echo "Checking for mysql package imports and initialization:" rg -g '!vendor/*' -B 2 -A 2 'mysql\.' echo -e "\nChecking for any dynamic protocol registration:" rg -g '!vendor/*' 'Register.*Context.*mysql'Length of output: 9109
| // Set a custom dialer for the "nucleitcp" protocol. This is just plain TCP, but it's registered | ||
| // with a different name so that we do not clobber the "tcp" dialer in the event that nuclei is | ||
| // being included as a package in another application. | ||
| mysql.RegisterDialContext("nucleitcp", func(ctx context.Context, addr string) (net.Conn, error) { | ||
| // Because we're not using the default TCP workflow, quietly add the default port | ||
| // number if no port number was specified. | ||
| if _, _, err := net.SplitHostPort(addr); err != nil { | ||
| addr += ":3306" | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the MySQL nucleitcp protocol implementation
The custom MySQL TCP dialer implementation lacks test coverage. Please add unit tests to verify:
- Custom dialer registration with "nucleitcp" protocol
- Default port (3306) addition when port is not specified
- Connection establishment behavior
🔗 Analysis chain
Verify test coverage for the new protocol.
Let's ensure there are adequate tests for the new "nucleitcp" protocol functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage of the new protocol
echo "Checking for MySQL protocol tests:"
rg -g '*_test.go' 'nucleitcp|RegisterDialContext'
echo -e "\nChecking for MySQL connection tests:"
rg -g '*_test.go' 'mysql.*Protocol.*=.*"tcp"'
Length of output: 267
Script:
#!/bin/bash
# Find MySQL-related test files
echo "MySQL test files:"
fd -g '*mysql*_test.go'
echo -e "\nTest files in protocol state directory:"
fd -g '*_test.go' pkg/protocols/common/protocolstate/
echo -e "\nChecking for TCP connection tests:"
rg -g '*_test.go' -A 5 'func.*Test.*TCP|func.*Test.*Connection' pkg/protocols/common/
echo -e "\nChecking for integration tests:"
fd -g '*integration*test.go'
Length of output: 545
Proposed changes
This introduces a "nucleitcp" protocol that Nuclei will use when making MySQL connections as part of its templates.
Previously, this would register (and de-register!) a custom "tcp" dialer, and that applied globally, so any piece of software that used a MySQL database and included nuclei in SDK mode would have its database connections ripped out from under it due to the dialer hijacking.
By using "nucleitcp" as the protocol, we are free to do whatever we want with the dialer and not impact any other packages.
Within our
BuildDSNfunction, we quietly replace the protocol to "nucleitcp" if it was "tcp", so nuclei developers don't have to do anything special to use this functionality; it will always do it.Closes #5854
Checklist
Summary by CodeRabbit
New Features
Improvements