Bug 7448 - smbd crash when sambaLMPassword and sambaNTPassword entries missing from ldap
smbd crash when sambaLMPassword and sambaNTPassword entries missing from ldap
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: User & Group Accounts
3.5.3
All Linux
: P3 minor
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-21 06:31 UTC by Roel van Meer
Modified: 2010-05-25 10:25 UTC (History)
1 user (show)

See Also:


Attachments
Fix segfault at authentication when nt_pw is NULL (520 bytes, patch)
2010-05-21 06:38 UTC, Roel van Meer
no flags Details
Fix segfault at authentication when nt_pw is NULL v2 (487 bytes, patch)
2010-05-21 06:45 UTC, Roel van Meer
no flags Details
git-am format patch for 3.5.x. (866 bytes, patch)
2010-05-21 16:21 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Roel van Meer 2010-05-21 06:31:18 UTC
When smbd is using the ldapsam backend, and the sambaLMPassword and sambaNTPassword entries are missing from a user account, smbd crashes when an attempt is made to login with this account.
Comment 1 Roel van Meer 2010-05-21 06:38:19 UTC
Created attachment 5726 [details]
Fix segfault at authentication when nt_pw is NULL

This patch fixes the segfault. It ensures that nt_pw is not NULL, just before the call that caused the crash. Not entirely sure if NT_STATUS_WRONG_PASSWORD is the right return code, though.
Comment 2 Roel van Meer 2010-05-21 06:45:17 UTC
Created attachment 5727 [details]
Fix segfault at authentication when nt_pw is NULL v2

V2 of patch: we better check nt_pw at the start of the block, though in normal use case it doesn't matter much.
Comment 3 Jeremy Allison 2010-05-21 16:21:48 UTC
Created attachment 5731 [details]
git-am format patch for 3.5.x.
Comment 4 Jeremy Allison 2010-05-21 16:22:22 UTC
Perfectly correct - thanks ! Applied to master.
Re-assigning to Karolin for inclusion in 3.5.x.

Jeremy.
Comment 5 Karolin Seeger 2010-05-25 02:53:54 UTC
Sorry, I do need a review from a second Samba developer first...
Comment 6 Volker Lendecke 2010-05-25 03:17:33 UTC
Comment on attachment 5731 [details]
git-am format patch for 3.5.x.

I'll give it a +, because it seems to fix the immediate problem. But the patch looks wrong: What happens in the ntlm_password_check() case for example?

Nevertheless, Karolin, please pull this for 3.5
Comment 7 Roel van Meer 2010-05-25 03:34:39 UTC
As far as I could see, ntlm_password_check() checks both before using them, but I haven't explicitly tested it.

Maybe we should add an early check that returns NT_STATUS_WRONG_PASSWORD if neither lm_hash nor nt_hash are set: in that case, it's not useful even trying to run the lower functions, since they always need at least one stored password to verify against.
Comment 8 Karolin Seeger 2010-05-25 03:45:15 UTC
As we do have enough time to wait for a "proper" fix for 3.5.4, I am going to wait a bit before picking the patch. 
Comment 9 Roel van Meer 2010-05-25 04:06:32 UTC
(In reply to comment #6)
> (From update of attachment 5731 [details])
> I'll give it a +, because it seems to fix the immediate problem. But the patch
> looks wrong: What happens in the ntlm_password_check() case for example?

The patch was meant to protect the call to SMBsesskeygen_ntv1(). As far as I can see, both hash_password_check() and ntlm_password_check() do their own checking for presence of stored hashes. If you'd like to see this done differently, could you point me in the right direction?
Comment 10 Jeremy Allison 2010-05-25 10:20:29 UTC
Karolin, Roel van Meer is correct. I went through the code paths and all other uses of nt_pw are correctly checked. I'm afraid this is the "proper patch" :-). I can't see a different way of doing this. Please pull for 3.5.x. Volker, if you want to confirm feel free but I agree with Roel.

Jeremy.
Comment 11 Karolin Seeger 2010-05-25 10:25:07 UTC
Okay, thanks for commenting again!
Pushed patch to v3-5-test.
Closing out bug report.

Please feel free to re-open if there are any issues left.

Roel, thanks a lot for reporting and providing the patch!