Fix ICMP False Positives in Socket Scan (Issue #1186) #1187
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughICMP handling now records Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/core/lib/test_socket.py (1)
14-48: Consider adding ClassVar annotations.The static analysis tool correctly identifies that these mutable class attributes should be annotated with
typing.ClassVarto explicitly indicate they are class-level attributes rather than instance attributes. While not strictly required, this follows Python typing best practices.🔎 Proposed refactor
+from typing import ClassVar + from unittest.mock import patch import pytest from nettacker.core.lib.socket import create_tcp_socket, SocketEngine class Responses: - tcp_connect_only = {} + tcp_connect_only: ClassVar[dict] = {} # ICMP responses with type/code validation - socket_icmp = {} + socket_icmp: ClassVar[dict] = {} - socket_icmp_echo_reply = { + socket_icmp_echo_reply: ClassVar[dict] = { "host": "127.0.0.1", "response_time": 0.001, "ssl_flag": False, "icmp_type": 0, # Echo Reply - SUCCESS "icmp_code": 0, "status": "alive", } - socket_icmp_dest_unreachable = { + socket_icmp_dest_unreachable: ClassVar[dict] = { "host": "127.0.0.1", "response_time": None, "ssl_flag": False, "icmp_type": 3, # Destination Unreachable "icmp_code": 3, # Port Unreachable "status": "unreachable", } - socket_icmp_network_unreachable = { + socket_icmp_network_unreachable: ClassVar[dict] = { "host": "192.0.2.1", "response_time": None, "ssl_flag": False, "icmp_type": 3, # Destination Unreachable "icmp_code": 0, # Network Unreachable "status": "unreachable", } - socket_icmp_no_response = { + socket_icmp_no_response: ClassVar[dict] = { "host": "10.255.255.1", "response_time": None, "ssl_flag": False, "icmp_type": None, "icmp_code": None, "status": "filtered", }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/lib/socket.py(3 hunks)tests/core/lib/test_socket.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
tests/core/lib/test_socket.pynettacker/core/lib/socket.py
tests/**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests
Files:
tests/core/lib/test_socket.py
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)
Files:
tests/core/lib/test_socket.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/socket.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/socket.py
🧬 Code graph analysis (1)
tests/core/lib/test_socket.py (1)
nettacker/core/lib/socket.py (2)
socket_icmp(94-268)response_conditions_matched(274-341)
🪛 Ruff (0.14.8)
tests/core/lib/test_socket.py
12-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
23-30: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
41-48: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (4)
nettacker/core/lib/socket.py (3)
208-211: LGTM! Proper variable initialization.The initialization of
delay,icmp_response_type, andicmp_response_codetoNonecorrectly preventsUnboundLocalErrorwhen no response is received within the timeout period.
234-268: LGTM! Correct ICMP type/code validation logic.The implementation correctly validates ICMP responses according to RFC 792:
- ICMP Echo Reply (type 0) → status "alive"
- ICMP error responses (e.g., type 3) → status "unreachable"
- No response/timeout → status "filtered"
This properly addresses the false positive issue described in #1186.
324-340: LGTM! Proper ICMP response filtering.The updated logic correctly filters ICMP responses to only count Echo Reply (type 0, status "alive") as successful detection. Non-alive responses are appropriately logged for debugging and return an empty list, which aligns with the behavior of other failed detection methods.
tests/core/lib/test_socket.py (1)
195-219: LGTM! Comprehensive ICMP scenario coverage.The four test methods properly validate all ICMP response scenarios outlined in the PR objectives:
- Echo Reply (type 0) → successful match
- Destination Unreachable (type 3, code 3) → no match
- Network Unreachable (type 3, code 0) → no match
- No response/filtered → no match
The assertions correctly verify both the return value structure and the specific status/type fields, ensuring the new ICMP validation logic works as expected.
webbsssss force-pushed the fix/icmp-false-positives-1186 branch from ae32691 to 779120f Compare There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/core/lib/test_socket.py (1)
9-12: Redundant assignment still present.This issue was flagged in a previous review. Line 9 creates a shared reference between
tcp_connect_onlyandsocket_icmp, then line 12 immediately reassignssocket_icmpto a new dict. This is confusing and should be fixed as suggested in the previous review.🔎 Proposed fix (same as previous review)
- tcp_connect_only = socket_icmp = {} - - # ICMP responses with type/code validation - socket_icmp = {} + tcp_connect_only = {} + + # ICMP responses with type/code validation + socket_icmp = {}
🧹 Nitpick comments (1)
tests/core/lib/test_socket.py (1)
14-48: Optional: Consider ClassVar annotations for test fixtures.Ruff suggests annotating mutable class attributes with
typing.ClassVar. While not required for test fixtures to function correctly, adding these annotations would align with Python typing best practices and silence the static analysis warnings.🔎 Example annotation
+from typing import ClassVar + class Responses: - tcp_connect_only = {} + tcp_connect_only: ClassVar[dict] = {} # ICMP responses with type/code validation - socket_icmp = {} + socket_icmp: ClassVar[dict] = {} - socket_icmp_echo_reply = { + socket_icmp_echo_reply: ClassVar[dict] = { "host": "127.0.0.1", ...Apply similar annotations to all fixture dictionaries (lines 14-48).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/lib/socket.py(3 hunks)tests/core/lib/test_socket.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/lib/socket.pytests/core/lib/test_socket.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/socket.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/socket.py
tests/**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests
Files:
tests/core/lib/test_socket.py
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)
Files:
tests/core/lib/test_socket.py
🪛 Ruff (0.14.8)
tests/core/lib/test_socket.py
12-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
23-30: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
41-48: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (6)
nettacker/core/lib/socket.py (4)
208-210: LGTM! Good defensive initialization.Initializing
delay,icmp_response_type, andicmp_response_codetoNonebefore the receive loop correctly preventsUnboundLocalErrorwhen no response is received (timeout scenario). This ensures the return statements at lines 244-263 always have valid values to work with.
233-236: LGTM! Correct ICMP metadata capture.Storing the ICMP type and code from the response packet enables the validation logic that distinguishes Echo Reply (type 0) from error responses (e.g., type 3). This is essential for fixing the false positives described in the PR objectives.
244-263: LGTM! Status classification aligns with RFC 792.The response structure and status logic correctly implement the PR objectives:
- "alive" for ICMP Echo Reply (type 0) indicating reachable host
- "unreachable" for ICMP error responses (e.g., type 3 Destination Unreachable)
- "filtered" when no response is received
The enriched response dict with
icmp_type,icmp_code, andstatusprovides callers with the necessary context while maintaining backward compatibility viahostandresponse_timefields.
320-335: LGTM! Correctly filters out non-Echo Reply responses.The validation logic ensures only ICMP Echo Reply (status == "alive") is treated as a successful detection. The debug logging for rejected responses (unreachable/filtered) aids troubleshooting without cluttering production logs. This implementation correctly addresses the false positive issue where any ICMP response was previously accepted.
tests/core/lib/test_socket.py (2)
14-48: LGTM! Comprehensive ICMP test fixtures.The four ICMP response fixtures thoroughly cover all scenarios described in the PR objectives:
- Echo Reply (type 0, status "alive") - successful ping
- Destination Unreachable (type 3, code 3, status "unreachable") - port unreachable
- Network Unreachable (type 3, code 0, status "unreachable") - network unreachable
- No Response (icmp_type/code None, status "filtered") - timeout/blocked
The fixture structure with explicit
icmp_type,icmp_code, andstatusfields provides clear test data for validating the ICMP validation logic.
195-219: LGTM! Thorough test coverage for ICMP validation.The four test methods effectively validate the PR objectives:
- ✅ Echo Reply correctly treated as successful detection (returns response, status="alive")
- ✅ Destination Unreachable correctly rejected (returns empty list)
- ✅ Network Unreachable correctly rejected (returns empty list)
- ✅ No Response correctly rejected (returns empty list)
The tests confirm that only ICMP Echo Reply (type 0) is accepted as "alive", fixing the false positive issue where any ICMP response was previously treated as success.
webbsssss force-pushed the fix/icmp-false-positives-1186 branch from 779120f to f81c370 Compare There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/core/lib/test_socket.py (1)
9-12: Consider annotating mutable class attributes withtyping.ClassVar.Static analysis suggests using
typing.ClassVarfor mutable class attributes to clarify intent and avoid potential confusion. While this is test fixture code, adding type annotations would align with best practices.🔎 Proposed refactor
+from typing import ClassVar + +class Responses: - tcp_connect_only = {} + tcp_connect_only: ClassVar[dict] = {} # ICMP responses with type/code validation - socket_icmp = {} + socket_icmp: ClassVar[dict] = {}Apply similar annotations to other mutable class attributes (lines 14-48) if desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/lib/socket.py(3 hunks)tests/core/lib/test_socket.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
tests/core/lib/test_socket.pynettacker/core/lib/socket.py
tests/**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests
Files:
tests/core/lib/test_socket.py
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)
Files:
tests/core/lib/test_socket.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/socket.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/socket.py
🧠 Learnings (1)
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR Repo: OWASP/Nettacker PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-09-07T19:20:58.332Z Learning: Maintain coverage (pytest configured with --cov=nettacker); add tests for new features and bug fixes Applied to files:
tests/core/lib/test_socket.py
🧬 Code graph analysis (1)
tests/core/lib/test_socket.py (1)
nettacker/core/lib/socket.py (3)
tcp_connect_only(42-60)socket_icmp(94-263)response_conditions_matched(269-336)
🪛 Ruff (0.14.8)
tests/core/lib/test_socket.py
9-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
23-30: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
41-48: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (6)
nettacker/core/lib/socket.py (4)
208-211: LGTM: Proper initialization prevents UnboundLocalError.Initializing these variables before the receive loop correctly prevents
UnboundLocalErrorwhen a timeout occurs before receiving a matching packet.
229-236: LGTM: ICMP type/code captured correctly.The code properly captures the ICMP type and code when a matching packet is received, enabling downstream validation.
242-263: LGTM: ICMP type validation correctly implemented.The return logic properly distinguishes between:
- "alive": ICMP Echo Reply (type 0) - the only successful ping response
- "unreachable": Other ICMP types (e.g., type 3 Destination Unreachable)
- "filtered": No response received (timeout)
This correctly implements the PR objectives and aligns with RFC 792.
320-335: LGTM: Response filtering correctly implements ICMP validation.Only ICMP responses with
status == "alive"are treated as successful detections. Non-alive responses are logged for debugging and excluded from matches, correctly preventing false positives from ICMP error messages.tests/core/lib/test_socket.py (2)
14-48: LGTM: Comprehensive ICMP test fixtures.The test fixtures cover all critical ICMP scenarios:
- Echo Reply (type 0) - successful ping
- Destination/Port Unreachable (type 3) - ICMP error
- Network Unreachable (type 3, code 0) - ICMP error
- No response - timeout/filtered
The data structure matches the updated
socket_icmp()return format and includes realistic test IPs.
195-219: LGTM: Thorough test coverage for ICMP validation.The four test methods comprehensively validate the ICMP type checking:
- ✅ Echo Reply (type 0) → counted as success
- ❌ Destination Unreachable (type 3) → rejected
- ❌ Network Unreachable (type 3) → rejected
- ❌ No response (filtered) → rejected
This directly addresses the issue of false positives and ensures only legitimate Echo Replies are treated as successful pings. Based on learnings, the test coverage aligns with project standards.
Aarush289 left a comment
Aarush289 left a comment There was a problem hiding this comment.
Hi,
Thank you for working on this and for submitting the PR.
I noticed that log.debug() has been used at several places in the changes. As far as I’m aware, log.debug() is not currently available/used in Nettacker, so this may need to be adjusted to match the existing logging conventions.
Regarding the fix itself, the core logic looks reasonable to me. However, I have a concern about whether this will work end-to-end. I am not fully sure if the report generation logic is currently hardcoded to a specific response format. If that is the case, these changes might lead to runtime errors or unexpected behavior.
To be safe, I would request you to please run this locally and verify that it works as expected. If possible, could you also attach a screenshot of the final resulting table/output after running the scan with this change? That would help confirm that the integration is working correctly.
Thanks again for your effort .
Best regards,
Aarush
| Hi Aarush, Thank you very much for the detailed feedback. I’ve made the changes you suggested:
For end‑to‑end testing, I ran SocketLibrary.socket_icmp() directly for three scenarios:
 In this environment (WSL/localhost), the loopback response is reported with type 8; however, the important behavior remains: only type 0 is treated as “alive”. When ICMP echo is blocked, the host is no longer reported as alive, which resolves the original false positive. I’ve attached a screenshot of the integration test output for reference. Please let me know if you’d like me to adjust anything further (for example, the status value for the localhost case, or additional tests around reporting). Thanks again for the review. |
The socket_icmp() method treated any ICMP response as host detection, causing false positives when firewalls sent ICMP Type 3 errors. Code only validated packet_id match, not ICMP response type. RFC 792 specifies only Type 0 (Echo Reply) indicates host is alive. 1. Initialize delay/type/code to None (prevents UnboundLocalError) 2. Capture ICMP type and code from response packet 3. Validate type before declaring host alive 4. Only ICMP Echo Reply (type 0) = host alive 5. ICMP Type 3 errors = host unreachable 6. No response (timeout) = filtered/blocked - Modified socket_icmp() to extract ICMP type/code - Added validation: status = 'alive' only if type == 0 - Updated return to include icmp_type, icmp_code, status fields - Modified response_conditions_matched() to validate status field - Added 4 comprehensive unit tests - Unit tests: 4 new tests cover all scenarios * Echo Reply (type 0) = match * Destination Unreachable (type 3) = no match * Network Unreachable (type 3, code 0) = no match * No response (timeout) = no match - Integration tests: Validated with real hosts - RFC 792 compliance: Only type 0 marked as alive - Eliminates false positives from firewall error responses - More accurate ICMP scanning results - Better error diagnostics with type/code fields Fixes OWASP#1186
webbsssss force-pushed the fix/icmp-false-positives-1186 branch from f81c370 to bcb3ce0 Compare There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/core/lib/test_socket.py (1)
9-65: Consider annotating mutable class attributes withtyping.ClassVar.Static analysis correctly flags that mutable class attributes should be annotated with
typing.ClassVarto indicate they are class-level rather than instance-level attributes. While this pattern works in practice for test fixtures, adding the annotations improves type safety and clarity.🔎 Proposed fix
+from typing import ClassVar + from unittest.mock import patch import pytest from nettacker.core.lib.socket import create_tcp_socket, SocketEngine class Responses: - tcp_connect_only = {} + tcp_connect_only: ClassVar[dict] = {} # ICMP responses with type/code validation - socket_icmp = {} + socket_icmp: ClassVar[dict] = {} - socket_icmp_echo_reply = { + socket_icmp_echo_reply: ClassVar[dict] = { "host": "127.0.0.1", "response_time": 0.001, "ssl_flag": False, "icmp_type": 0, # Echo Reply - SUCCESS "icmp_code": 0, "status": "alive", } - socket_icmp_dest_unreachable = { + socket_icmp_dest_unreachable: ClassVar[dict] = { "host": "127.0.0.1", "response_time": None, "ssl_flag": False, "icmp_type": 3, # Destination Unreachable "icmp_code": 3, # Port Unreachable "status": "unreachable", } - socket_icmp_network_unreachable = { + socket_icmp_network_unreachable: ClassVar[dict] = { "host": "192.0.2.1", "response_time": None, "ssl_flag": False, "icmp_type": 3, # Destination Unreachable "icmp_code": 0, # Network Unreachable "status": "unreachable", } - socket_icmp_no_response = { + socket_icmp_no_response: ClassVar[dict] = { "host": "10.255.255.1", "response_time": None, "ssl_flag": False, "icmp_type": None, "icmp_code": None, "status": "filtered", } - tcp_connect_send_and_receive = { + tcp_connect_send_and_receive: ClassVar[dict] = { "response": 'HTTP/1.1 400 Bad Request...', "service": "http", "peer_name": ( "127.0.0.1", 80, ), "ssl_flag": True, } - ssl_version_scan = { + ssl_version_scan: ClassVar[dict] = { "ssl_version": "TLSv1", "weak_version": True, "weak_cipher_suite": True, "ssl_flag": True, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/lib/socket.py(3 hunks)tests/core/lib/test_socket.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nettacker/core/lib/socket.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
tests/core/lib/test_socket.py
tests/**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests
Files:
tests/core/lib/test_socket.py
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)
Files:
tests/core/lib/test_socket.py
🧬 Code graph analysis (1)
tests/core/lib/test_socket.py (1)
nettacker/core/lib/socket.py (3)
tcp_connect_only(42-60)socket_icmp(94-263)response_conditions_matched(269-324)
🪛 Ruff (0.14.8)
tests/core/lib/test_socket.py
9-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
23-30: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
41-48: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (3)
tests/core/lib/test_socket.py (3)
14-48: Excellent test fixture design for ICMP validation.The ICMP response fixtures accurately model the different scenarios per RFC 792 and align perfectly with the PR objectives:
- Echo Reply (type 0) as the only "alive" response
- Destination Unreachable variants (type 3) as "unreachable"
- Timeout/no-response as "filtered"
The fixture structure matches the implementation in
socket.pyand provides comprehensive test coverage for the new ICMP validation logic.
195-202: Well-structured test for ICMP Echo Reply validation.The test correctly validates that ICMP Echo Reply (type 0) is the only response treated as successful, with comprehensive assertions checking both the full response return and specific field values.
203-219: Comprehensive coverage for ICMP error and timeout scenarios.These three tests effectively validate the core PR requirement: only ICMP Echo Reply should be treated as successful detection. The tests cover both variants of Type 3 (Destination Unreachable with different codes) and the no-response/filtered scenario, ensuring false positives are eliminated.
| Hi @webbsssss , |
| Hi @Aarush289, Thank you for the feedback and for pointing out that I should validate the As requested, I have now run Nettacker end-to-end against a real target Command executed Verification
I have attached screenshots of:
My code changes in this branch remain focused on the ICMP false-positive Please let me know if you would like me to run this with any additional Thank you again for your time and guidance. Best regards, |
…e in response_conditions_matched - Fixed unreachable return statement in response_conditions_matched() - Added proper else clause to return [] when status != 'alive' - Ensures proper handling of None response_time for non-Echo-Reply responses
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nettacker/core/lib/socket.py (1)
94-162: Consider documenting the return value structure.The
socket_icmpmethod has an extensive docstring covering history but doesn't document the return value. Given the new fields (icmp_type,icmp_code,status), adding a Returns section would improve maintainability.🔎 Suggested addition to docstring
5 Aug 2021 - Modified by Ali Razmjoo Qalaei (Reformat the code and more human readable) + + Returns: + dict: Response containing: + - host (str): Target hostname + - response_time (float|None): Round-trip time in seconds, None if no response + - ssl_flag (bool): Always False for ICMP + - icmp_type (int|None): ICMP type (0=Echo Reply, 3=Unreachable, etc.) + - icmp_code (int|None): ICMP code for additional context + - status (str): "alive" (Echo Reply), "unreachable" (ICMP error), or "filtered" (no response) """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/lib/socket.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/lib/socket.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/socket.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/socket.py
🔇 Additional comments (4)
nettacker/core/lib/socket.py (4)
208-211: LGTM! Proper initialization prevents UnboundLocalError.Initializing these variables before the receive loop ensures they have defined values even when no ICMP response is received (timeout case).
233-236: LGTM! Correctly captures ICMP response type and code.Storing the ICMP type and code enables proper validation to distinguish between Echo Reply (type 0) and error responses (e.g., type 3 Destination Unreachable).
318-324: LGTM! Correctly filters ICMP responses by status.Only ICMP Echo Reply (status
"alive") is treated as a successful match. Theisinstancecheck provides defensive validation, and the explicitelseclause ensures non-alive responses return an empty list.
229-262: ICMP error responses (Type 3) will not be detected as "unreachable".ICMP Destination Unreachable (Type 3) messages contain an unused 4-byte field followed by the encapsulated original IP header, unlike ICMP Echo Reply (Type 0) which has Identifier and Sequence fields. The packet_id check at line 229 will fail for Type 3 responses because the unused field typically contains zeros, not matching the PID-derived
random_integer. This causes Type 3 errors to be misclassified asstatus="filtered"(timeout) rather thanstatus="unreachable".To properly detect ICMP errors, consider:
- Checking the packet_type before applying the packet_id match (ICMP errors won't have matching IDs in the outer header)
- For error types (3, 11, etc.), validate the request by parsing the encapsulated original ICMP header in the payload instead
🔧 Analysis and Fix AppliedRoot Cause AnalysisThe CI/CD failure in job if sub_step["method"] == "socket_icmp": if isinstance(response, dict) and response.get("status") == "alive": return response return [] # ❌ UNREACHABLE - causes function to fall throughThe Problem
Solution ImplementedFixed the ICMP response handling in if sub_step["method"] == "socket_icmp": # Only count ICMP Echo Reply (type 0) as success if isinstance(response, dict) and response.get("status") == "alive": return response else: # ✅ Explicit else clause return []Why This Works
Commit Details
|
2 participants
Summary
This pull request fixes ICMP ping false positives in the socket_icmp() implementation by validating the ICMP response type and code before marking a host as alive. Only proper ICMP Echo Reply messages are now treated as successful host detection, which aligns the behavior with RFC 792.
Problem
Currently, socket_icmp() treats any ICMP response as a sign that the host is alive. This leads to false positives in scenarios where firewalls or intermediate devices send ICMP Destination Unreachable (type 3) errors instead of forwarding Echo Replies, or when ICMP Echo is blocked but error responses are still generated. In these cases, the host is reported as "alive" even though it is actually filtered or unreachable.
Root Cause
The previous implementation only checked that the received packet had a matching identifier (packet_id == random_integer). It did not validate the ICMP type and code fields. According to RFC 792 and common ICMP usage, Type 0 = Echo Reply indicates the host responded and is considered alive, while Type 3 = Destination Unreachable indicates an error on the path or at the target host. By ignoring the type and code, the scanner could incorrectly classify error packets as successful pings.
Solution
The fix introduces explicit ICMP type/code validation and a clearer status model. First, safe initialization prevents UnboundLocalError in timeout paths by setting delay, icmp_response_type, and icmp_response_code to None before the receive loop. Second, after unpacking the ICMP header, the code now stores icmp_response_type and icmp_response_code for the packet that matches the random identifier. Third, socket_icmp() now returns a richer result including host, response_time, ssl_flag, icmp_type, icmp_code, and status fields. When a valid Echo Reply is received (type 0), status is set to "alive". When the response is not an Echo Reply, status is set to "unreachable". When no response is received before timeout, status is set to "filtered" and response_time stays None. Finally, in SocketEngine.response_conditions_matched(), the ICMP branch now checks if the response status equals "alive" before treating it as a successful match. This ensures only ICMP Echo Reply responses are treated as successful matches, and all other ICMP responses including Destination Unreachable are excluded from positive detection.
Changes
File: nettacker/core/lib/socket.py - Initialized delay, icmp_response_type, and icmp_response_code before the receive loop to handle timeouts safely. On matching packet_id, the code now extracts the send timestamp, computes delay, and stores icmp_response_type and icmp_response_code from the unpacked ICMP header. Reworked the return logic in socket_icmp() to distinguish between alive, unreachable, and filtered cases, and always include icmp_type, icmp_code, and status in the returned dictionary. Updated response_conditions_matched() to only treat responses with status equals alive as matches, and log non-Echo-Reply responses with type, code, and status for easier debugging.
File: tests/core/lib/test_socket.py - Extended the Responses class with structured ICMP test responses including socket_icmp_echo_reply (type 0, code 0, status alive), socket_icmp_dest_unreachable (type 3, code 3, status unreachable), socket_icmp_network_unreachable (type 3, code 0, status unreachable), and socket_icmp_no_response (no type/code, status filtered). Added four focused tests to cover the new behavior: test_socket_icmp_echo_reply_should_match, test_socket_icmp_dest_unreachable_should_not_match, test_socket_icmp_network_unreachable_should_not_match, and test_socket_icmp_no_response_should_not_match. These tests verify that only Echo Reply is accepted as a valid success, and all error/timeout scenarios are rejected.
Testing
All tests were run with Python 3 and pytest. Unit tests confirmed that existing socket tests pass and all 4 new ICMP tests pass, verifying that Echo Reply (type 0) is treated as a match while Destination Unreachable variants (type 3) and no-response/timeout are not treated as matches. Manual integration tests using SocketLibrary.socket_icmp() showed that real hosts return status equals alive with icmp_type equals 0, and when ICMP Echo is blocked locally, socket_icmp no longer returns alive and is classified as unreachable or filtered instead. These tests confirm that ICMP Echo Reply is handled correctly and that false positives caused by ICMP error messages are eliminated.
Impact
This change eliminates ICMP ping false positives caused by treating all ICMP responses as success, and now differentiates between alive (Echo Reply), unreachable (ICMP errors such as Type 3), and filtered (no response/timeout). The socket_icmp() function now returns additional fields icmp_type, icmp_code, and status. Existing callers that only use host and response_time continue to work as before. Behavior is now aligned with RFC 792 and common ICMP semantics by treating only Echo Reply (type 0) as a successful ping.
Notes
I am a student contributor and focused this change on being as minimal and focused as possible, backwards-compatible for existing callers, well-tested with both unit tests and simple integration tests, and easy to review with clear separation between core logic changes and tests. Feedback on the implementation or tests is very welcome.
Fixes #1186