Bug 13275 - [SECURITY] generate_random_machine_password() does not prevent against generated password trunction.
Summary: [SECURITY] generate_random_machine_password() does not prevent against genera...
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
Depends on:
Reported: 2018-02-15 19:55 UTC by Jeremy Allison
Modified: 2018-02-19 09:10 UTC (History)
3 users (show)

See Also:

Possible git-am fix. (1.61 KB, patch)
2018-02-15 19:58 UTC, Jeremy Allison
jra: review? (metze)
jra: review? (slow)

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2018-02-15 19:55:56 UTC
generate_random_machine_password() creates a random buffer to act as the CH_UTF16MUNGED source for the machine password, but doesn't ensure that the random value generated doesn't contain an embedded 16-bit NULL.

The ascii_fallback: code path does check for this, but this doesn't get called unless converting CH_UTF16MUNGED -> utf8 and CH_UTF16MUNGED -> CH_UNIX come up with different lengths, which won't happen in the case of an embedded 16-bit NULL.

Found by Lutz Justen <ljusten@google.com>.

Patch to follow.
Comment 1 Jeremy Allison 2018-02-15 19:58:27 UTC
Created attachment 13964 [details]
Possible git-am fix.

Metze, can you take a look at this please ?

If you agree it's an issue, we need a CVE and can roll it into the upcoming security patch.
Comment 2 Stefan Metzmacher 2018-02-15 21:02:22 UTC
(In reply to Jeremy Allison from comment #0)

The conversation from UTF16MUNGED to UTF8 takes care of
0 bytes. See utf16_munged_pull()

                if (codepoint == 0) {
                        codepoint = 1;

for the ascii fallback we have:

                if (state->tmp == 0) {
                        state->tmp = 0x01;

So where do you see truncated passwords?
Comment 3 Jeremy Allison 2018-02-15 23:45:54 UTC
Oh perfect - thanks ! The issue was raised through code review/inspection from other Google engineers, and neither of us looked inside utf16_munged_pull() to see that it already took are of this issue.

I'll close this one out, thanks for responding.

Maybe adding a comment explaining that NUL is taken care of inside utf16_munged_pull() might help, but that's not a bug.

Thanks !