|
| 1 | +# Implementation Plan: Fix Unhandled ValueError during authentication (Issue #2099) |
| 2 | + |
| 3 | +## Summary |
| 4 | + |
| 5 | +When the SMB server sends a truncated or malformed session setup response (e.g. connection drop, timeout), parsing the response can raise `ValueError: subsection not found` from `structure.py` because `bytes.index(b'\x00')` is used for asciiz fields and raises when no NUL byte exists. This is unhelpful for users and should be turned into a clear authentication failure. |
| 6 | + |
| 7 | +**References:** [GitHub Issue #2099](https://github.com/fortra/impacket/issues/2099), [PR #2118](https://github.com/fortra/impacket/pull/2118) |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## Root cause |
| 12 | + |
| 13 | +1. **`impacket/structure.py`** |
| 14 | + In `calcUnpackSize()`, for format `'z'` (asciiz), the code does: |
| 15 | + ```python |
| 16 | + return data.index(self.b('\x00'))+1 |
| 17 | + ``` |
| 18 | + If the server sends truncated data (no NUL terminator), `index()` raises `ValueError: subsection not found`, which is opaque. |
| 19 | + |
| 20 | +2. **`impacket/smb.py`** |
| 21 | + `login_extended()` and the first session-setup path call `sessionData.fromString(sessionResponse['Data'])` (around lines 3583 and 3656) without catching parsing errors. So any `ValueError` from the structure layer propagates to the user. |
| 22 | + |
| 23 | +3. **Maintainer expectation (from PR #2118)** |
| 24 | + - Provide a **descriptive error** in the structure layer (e.g. which field and that the NUL terminator is missing). |
| 25 | + - **Handle** that exception in the SMB layer: log the reason (e.g. debug) and raise a proper **SessionError** so callers see an authentication failure, not a raw `ValueError`. |
| 26 | + |
| 27 | +--- |
| 28 | + |
| 29 | +## Implementation plan |
| 30 | + |
| 31 | +### Step 1: Descriptive ValueError in `structure.py` (asciiz) |
| 32 | + |
| 33 | +- **File:** `impacket/structure.py` |
| 34 | +- **Location:** `calcUnpackSize()`, asciiz branch (`format[:1] == 'z'`), around line 537–539. |
| 35 | + |
| 36 | +**Change:** |
| 37 | + |
| 38 | +- Wrap the asciiz size calculation in try/except: |
| 39 | + - On `ValueError` (from `data.index(...)`), re-raise a new `ValueError` with a message that includes the field name and states that the NUL terminator was not found. |
| 40 | +- Use the existing `field` parameter already passed into `calcUnpackSize()` so the message can name the field (e.g. `NativeOS`). |
| 41 | + |
| 42 | +**Example message:** |
| 43 | +`"Can't find NUL terminator in field '%s'" % (field or 'unknown')` |
| 44 | + |
| 45 | +**Note:** The `'u'` (UTF-16le) branch already raises a clear `ValueError` (line 551); no change needed there unless we want to align wording. |
| 46 | + |
| 47 | +--- |
| 48 | + |
| 49 | +### Step 2: Handle parsing errors in SMB session setup |
| 50 | + |
| 51 | +- **Files:** `impacket/smb.py` |
| 52 | +- **Call sites:** |
| 53 | + - First session setup (e.g. after negotiate): `sessionData.fromString(sessionResponse['Data'])` around **line 3583**. |
| 54 | + - NTLM session setup in `login_extended()`: `sessionData.fromString(sessionResponse['Data'])` around **line 3656**. |
| 55 | + - Optionally: negotiation path `_dialects_data.fromString(sessionResponse['Data'])` around **line 2971** for consistency. |
| 56 | + |
| 57 | +**Change:** |
| 58 | + |
| 59 | +- Around each of these `fromString(...)` calls: |
| 60 | + - Catch `ValueError` (and optionally `Exception` limited to parsing/structure errors if the project prefers). |
| 61 | + - Log the exception at **debug** level (e.g. `logging.getLogger(__name__).debug(...)`) so the exact reason (e.g. “Can't find NUL terminator in field 'NativeOS'”) is available when debugging. |
| 62 | + - Raise an appropriate **`SessionError`** so that: |
| 63 | + - Public API (`smbconnection.login()` etc.) continues to document `SessionError` for authentication failures. |
| 64 | + - Users see a clear “authentication failed” style error (e.g. NT status `STATUS_LOGON_FAILURE` or a dedicated message) instead of a generic `ValueError`. |
| 65 | + |
| 66 | +**SessionError form:** |
| 67 | + |
| 68 | +- Use existing `smb.SessionError(error_string, error_class, error_code, nt_status=1, packet=None)`. |
| 69 | +- Use an NT status code such as **`STATUS_LOGON_FAILURE`** (already used in `smb.py`, e.g. `0xC000006D`) so that `smbconnection` can translate it to `SessionError` with a standard message. |
| 70 | +- Ensure the high/low 16-bit split matches how `SessionError.__init__` builds the 32-bit code when `nt_status=1` (e.g. `error_code` high word, `error_class` low word → `(error_code << 16) + error_class` = `0xC000006D`). |
| 71 | + |
| 72 | +**Logging:** |
| 73 | + |
| 74 | +- Add `import logging` at top of `smb.py` if not present. |
| 75 | +- Use a module-level logger, e.g. `logger = logging.getLogger(__name__)`. |
| 76 | +- In the except block: `logger.debug("Session setup response parsing failed: %s", e)` (or equivalent). |
| 77 | + |
| 78 | +--- |
| 79 | + |
| 80 | +### Step 3: Optional – `smbconnection.py` |
| 81 | + |
| 82 | +- **File:** `impacket/smbconnection.py` |
| 83 | +- **Current behavior:** `login()` already catches `(smb.SessionError, smb3.SessionError)` and re-raises as `SessionError(e.get_error_code(), e.get_error_packet())` (lines 293–294). |
| 84 | +- **Change:** None required for this bug. Once `smb.py` raises `SessionError` for parsing failures, `smbconnection` will propagate it correctly. Optionally document in docstring that authentication can fail due to invalid/truncated server response. |
| 85 | + |
| 86 | +--- |
| 87 | + |
| 88 | +## Regression tests |
| 89 | + |
| 90 | +### Test 1: Structure layer – asciiz without NUL (required) |
| 91 | + |
| 92 | +- **File:** `tests/misc/test_structure.py` |
| 93 | +- **Purpose:** Ensure that when unpacking data for an asciiz field that contains no NUL byte, the code raises `ValueError` with a message that mentions the field and “NUL terminator” (or equivalent), and does **not** raise the generic `ValueError: subsection not found`. |
| 94 | + |
| 95 | +**Approach:** |
| 96 | + |
| 97 | +- Define a small structure with a single `'z'` field (or use an existing one). |
| 98 | +- Call `fromString()` with bytes that have no `\x00` (e.g. `b'NoNULhere'`). |
| 99 | +- Assert that `ValueError` is raised and that the exception message contains the field name and a clear reference to the missing NUL terminator. |
| 100 | + |
| 101 | +### Test 2: Structure layer – SMB session response structure (recommended) |
| 102 | + |
| 103 | +- **File:** `tests/misc/test_structure.py` (or a dedicated `tests/SMB_RPC/test_smb_session_parsing.py` if preferred) |
| 104 | +- **Purpose:** Regress the exact code path from the bug: `SMBSessionSetupAndX_Extended_Response_Data.fromString()` with truncated data (no NUL in `NativeOS`). |
| 105 | + |
| 106 | +**Approach:** |
| 107 | + |
| 108 | +- Import `smb.SMBSessionSetupAndX_Extended_Response_Data` and (if needed) set `SecurityBlobLength` on the instance. |
| 109 | +- Build minimal `data` that has the security blob (e.g. length 0) then a string with no NUL (e.g. `b'Truncated'`). |
| 110 | +- Call `fromString(data)` and assert `ValueError` is raised and the message references `NativeOS` and the missing NUL terminator. |
| 111 | + |
| 112 | +### Test 3: SMB layer – SessionError on truncated response (recommended) |
| 113 | + |
| 114 | +- **File:** `tests/SMB_RPC/test_smb.py` or a new unit test file |
| 115 | +- **Purpose:** Ensure that when session setup response parsing fails (e.g. truncated data), the SMB layer raises `SessionError` (or the appropriate public exception) and not a raw `ValueError`. |
| 116 | + |
| 117 | +**Approach (choose one):** |
| 118 | + |
| 119 | +- **Option A (unit, no server):** Patch or mock the code that provides `sessionResponse['Data']` (or the underlying `recvSMB()` / socket) so that it returns truncated data (e.g. no NUL in NativeOS). Call `login()` (or the internal session-setup path) and assert that the raised exception is `SessionError` (or the high-level `smbconnection.SessionError`) with an appropriate error code (e.g. logon failure). |
| 120 | +- **Option B (narrow unit):** Call the same parsing path that `login_extended()` uses (e.g. build `SMBSessionSetupAndX_Extended_Response_Data`, set `SecurityBlobLength`, call `fromString(truncated_data)`), then in a separate test verify that the exception handler in `login_extended()` converts `ValueError` to `SessionError` (e.g. by testing the exception-handling logic in isolation or via a small helper that mimics the try/except). |
| 121 | + |
| 122 | +Implementing **Test 1** and **Test 2** gives strong regression coverage at the structure layer. **Test 3** locks in the SMB behavior and prevents future changes from letting `ValueError` leak out again. |
| 123 | + |
| 124 | +**Tests added in this plan:** |
| 125 | +- **Test 1 & 2** are implemented in `tests/misc/test_structure.py` as `Test_asciiz_no_nul_raises_clear_error` and pass. |
| 126 | +- **Test 3** is added as a skipped stub in `tests/SMB_RPC/test_smb.py` (`Test_Issue2099_SessionError_On_Truncated_Response`). Optional: implement by mocking `recvSMB` to return a valid session-setup packet and patching `SMBSessionSetupAndX_Extended_Response_Data.fromString` to raise `ValueError`, then assert `login()` raises `SessionError`. |
| 127 | + |
| 128 | +--- |
| 129 | + |
| 130 | +## Order of work |
| 131 | + |
| 132 | +1. Implement **Step 1** (structure.py asciiz descriptive ValueError). |
| 133 | +2. Add **Test 1** (and optionally **Test 2**) and run tests to confirm the new message and that no generic “subsection not found” is raised. |
| 134 | +3. Implement **Step 2** (smb.py catch ValueError, log, raise SessionError). |
| 135 | +4. Add **Test 3** and run full SMB/structure tests. |
| 136 | +5. Optionally add the same handling at the negotiation `fromString` (line 2971) and extend tests if desired. **Done:** negotiation path and extended-security parse in `neg_session` now catch `ValueError`, log at debug, and raise `SessionError` (STATUS_INVALID_PARAMETER) for consistency. |
| 137 | + |
| 138 | +--- |
| 139 | + |
| 140 | +## Acceptance criteria |
| 141 | + |
| 142 | +- [x] Parsing session setup response with no NUL in an asciiz field (e.g. `NativeOS`) raises `ValueError` with a message like “Can't find NUL terminator in field 'NativeOS'” (or equivalent), not “subsection not found”. |
| 143 | +- [x] When that parsing fails during SMB login/session setup, the application raises `SessionError` (or the public session error type) with a clear authentication-failure meaning (e.g. STATUS_LOGON_FAILURE), not a raw `ValueError`. |
| 144 | +- [x] The exact parsing failure reason is logged at debug level for troubleshooting. |
| 145 | +- [x] Regression tests (at least Test 1 and Test 2) are in place and pass; Test 3 is recommended for the SMB path. |
0 commit comments