Improve HTTP/HTTPS module scheduling when service detection fails#1371
Improve HTTP/HTTPS module scheduling when service detection fails#1371notReallySouvik wants to merge 6 commits intoOWASP:masterfrom
Conversation
updated bn.yaml updated the locale readme file added the generated report file to gitignore
…n https services. It paryially fixes issuse OWASP#1357.
Summary by CodeRabbit
WalkthroughThe pull request modifies service detection and discovery logic across two core modules. It adds SSL flag-based service override logic in socket operations and implements a fallback mechanism to ensure discovered services are populated when port scan data is unavailable. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nettacker/core/lib/socket.py (1)
87-90: SSL-based service override may misclassify non-HTTP TLS services.Forcing
service = "https"for any SSL connection will incorrectly label other TLS-wrapped services (e.g., LDAPS on 636, IMAPS on 993, FTPS on 990) as "https". This could cause modules expecting specific services to run against incompatible targets.Consider a more targeted approach that only applies this fallback when the service remains "unknown" after
getservbyport:♻️ Proposed refinement
try: service = socket.getservbyport(port) except OSError: service = "unknown" # If SSL was used during connection, treat service as HTTPS - if ssl_flag: + if ssl_flag and service == "unknown": service = "https"Also note that
tcp_connect_only(lines 52-54) does not have this override, creating an inconsistency in service detection behavior between the two methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/lib/socket.py` around lines 87 - 90, The current unconditional override (if ssl_flag: service = "https") mislabels non-HTTP TLS services; change it to only set service = "https" when ssl_flag is true AND the existing service is unknown/unset (e.g., service is None or "unknown") after calling socket.getservbyport; also apply the same guarded logic to the tcp_connect_only path so both methods behave consistently and avoid misclassifying LDAPS/IMAPS/FTPS.nettacker/core/module.py (1)
90-93: Assigning the same ports to both "http" and "https" may cause redundant scans and errors.The fallback assigns the identical port list to both protocols. This means port 80 would trigger both HTTP (correct) and HTTPS (likely error/timeout) modules, and port 443 would trigger both HTTPS (correct) and HTTP (likely error) modules—doubling scan time and producing noise.
Consider distinguishing ports by convention:
♻️ Proposed refinement to differentiate protocols
# FALLBACK if port_scan didn't run if not services: ports = self.module_inputs.get("ports") or [80, 443] - services = {"http": ports, "https": ports} + # Common HTTPS ports (443, 8443, etc.) go to https; others to http + https_ports = [p for p in ports if p in {443, 8443, 9443}] + http_ports = [p for p in ports if p not in {443, 8443, 9443}] + services = {} + if http_ports: + services["http"] = http_ports + if https_ports: + services["https"] = https_ports + # If all ports filtered out, fall back to trying both + if not services: + services = {"http": ports, "https": ports}Alternatively, if the intent is to be more aggressive and scan all ports with both protocols when service detection fails, add a comment clarifying this is intentional so future maintainers understand the tradeoff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/module.py` around lines 90 - 93, The fallback currently assigns the same port list to both "http" and "https" (ports = self.module_inputs.get("ports") or [80, 443]; services = {"http": ports, "https": ports}), causing redundant/erroneous scans; change this to distinguish protocol defaults (e.g., read separate inputs like "ports_http" and "ports_https" or default to http->[80] and https->[443] and set services = {"http": http_ports, "https": https_ports}) and update the fallback logic in the Module class where self.module_inputs, ports, and services are used; if leaving the aggressive behavior intentional, replace the code with an explicit comment documenting that both protocols are scanned for all ports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nettacker/core/lib/socket.py`:
- Around line 87-90: The current unconditional override (if ssl_flag: service =
"https") mislabels non-HTTP TLS services; change it to only set service =
"https" when ssl_flag is true AND the existing service is unknown/unset (e.g.,
service is None or "unknown") after calling socket.getservbyport; also apply the
same guarded logic to the tcp_connect_only path so both methods behave
consistently and avoid misclassifying LDAPS/IMAPS/FTPS.
In `@nettacker/core/module.py`:
- Around line 90-93: The fallback currently assigns the same port list to both
"http" and "https" (ports = self.module_inputs.get("ports") or [80, 443];
services = {"http": ports, "https": ports}), causing redundant/erroneous scans;
change this to distinguish protocol defaults (e.g., read separate inputs like
"ports_http" and "ports_https" or default to http->[80] and https->[443] and set
services = {"http": http_ports, "https": https_ports}) and update the fallback
logic in the Module class where self.module_inputs, ports, and services are
used; if leaving the aggressive behavior intentional, replace the code with an
explicit comment documenting that both protocols are scanned for all ports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e56cf93-a494-4a87-8580-789b847e9d0f
📒 Files selected for processing (2)
nettacker/core/lib/socket.pynettacker/core/module.py
1 participant
Proposed change
This PR improves module scheduling when HTTP services running over TLS are not detected during service discovery.
Currently, HTTP detection in
port.yamlrelies on plaintext HTTP response patterns such asHTTP/1.1,Content-Type, andServer. When connecting to HTTPS endpoints (e.g. port 8443), the scanner receives TLS handshake bytes instead of HTTP headers, so these patterns do not match and the service may remain classified asdefault_service: unknown.Because of this, modules declaring
library: httpmay be filtered out during scheduling even though the service is actually HTTP over TLS.This patch introduces a small fallback:
port_scandoes not return detected services, use the supplied ports as a fallback.https.This helps HTTP/HTTPS modules run in cases where the service exists but was not identified during the initial scan.
Related to #1357.
Further improvements (such as removing hard-coded ports in modules) can be addressed separately.
Type of change
Checklist