Skip to content

fix: Unhandled ValueError during authentication#2118

Open
statxc wants to merge 1 commit intofortra:masterfrom
statxc:fix/issue-unhandled-value-error
Open

fix: Unhandled ValueError during authentication#2118
statxc wants to merge 1 commit intofortra:masterfrom
statxc:fix/issue-unhandled-value-error

Conversation

@statxc
Copy link

@statxc statxc commented Feb 11, 2026

Fixes #2099

Problem

When SMB authentication fails due to incomplete server responses (network drops, timeouts), users see:

ValueError: subsection not found

This error message doesn't indicate what failed or which field caused the issue.

Solution

Added exception handling to catch the ValueError and provide a clearer error message:

ValueError: Can't find NUL terminator in field 'NativeOS'

Changes

  • impacket/structure.py: Added try/except block for ASCII string parsing (3 lines)
  • Additionally modify UTF-16le error

@statxc
Copy link
Author

statxc commented Feb 11, 2026

Hi @anadrianmanrique. Could you please review my PR and share any feedback? thank you.

@anadrianmanrique anadrianmanrique added the in review This issue or pull request is being analyzed label Feb 12, 2026
@statxc
Copy link
Author

statxc commented Feb 19, 2026

Hi @alexisbalbachan @anadrianmanrique . could you please review this PR and share any feedback?

@anadrianmanrique
Copy link
Collaborator

@intelliking this PR it's been tagged already under review. Feedback will be provided as soon the review is completed.
thanks

@statxc
Copy link
Author

statxc commented Feb 23, 2026

Hi @anadrianmanrique , Please take a look at this PR when you have a moment. If everything looks good, feel free to merge it. thank you

@alexisbalbachan
Copy link
Collaborator

Hi, thanks for this PR. It’s going in the right direction by making the error more descriptive.
That said, there’s still work needed to fully fix the issue: the ValueError now has a better message, but it is still unhandled.
I’m currently reviewing where we should handle this exception and what behavior we want in that path.
Once that is in place, we can merge this PR.

@anadrianmanrique anadrianmanrique added low Low priority item and removed in review This issue or pull request is being analyzed labels Feb 26, 2026
@statxc
Copy link
Author

statxc commented Feb 27, 2026

Hi, thanks for this PR. It’s going in the right direction by making the error more descriptive. That said, there’s still work needed to fully fix the issue: the ValueError now has a better message, but it is still unhandled. I’m currently reviewing where we should handle this exception and what behavior we want in that path. Once that is in place, we can merge this PR.

Should I wait?

@statxc
Copy link
Author

statxc commented Mar 13, 2026

Hey @alexisbalbachan, have any updates? I opened this PR last month. It’s a simple fix. If there are no further problem, hope to merge this PR 🙏
thank you

@alexisbalbachan
Copy link
Collaborator

Hey @statxc, as mentioned in my previous comment, even though this PR is correct and simple in terms of code it still doesn't solve the issue described in #2099 -> Yes, its message is now more descriptive, but it is still unhandled by smb/smbconnection -> users still get ValueError. The goal of the fix for #2099 is gracefully handling that exception (probably at smb->login()), it should also log the exact reason there, as a debug message) and raising a proper exception for the user (like SessionError stating that the authentication failed).
Right now we're working on high priority tickets for the next release, we'll implement what's missing from this PR after that, or if you prefer, you can implement it yourself to speed up the process.

herbenderbler added a commit to herbenderbler/impacket that referenced this pull request Mar 15, 2026
… log)

When session setup or negotiate response parsing fails (e.g. truncated server
response, no NUL in asciiz field), users previously saw an unhandled
ValueError: subsection not found. Now:

- structure.py: asciiz calcUnpackSize raises descriptive ValueError with
  field name; pass field through code specifier for 'z=""' format.
- smb.py: catch ValueError at all four parse sites (two session-setup paths,
  negotiate + extended-security), LOG.debug the reason, raise SessionError
  (STATUS_LOGON_FAILURE for auth, STATUS_INVALID_PARAMETER for negotiate).
- smbconnection: docstring notes invalid/truncated server response.
- Regression tests: test_structure (asciiz missing NUL, session-like struct),
  test_smb (mock negotiate + session setup, fromString raises -> SessionError).
- IMPLEMENTATION_PLAN_2099.md and ChangeLog entry.

Addresses maintainer feedback on PR fortra#2118 (graceful handling + log + SessionError).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

low Low priority item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled ValueError during authentication

3 participants