Bug 15044 - Fuzzing build failure due to un-initialised value check after winbindd_pam changes
Summary: Fuzzing build failure due to un-initialised value check after winbindd_pam ch...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samuel Cabrero
QA Contact: Samba QA Contact
URL: https://bugs.chromium.org/p/oss-fuzz/...
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-11 21:27 UTC by Andrew Bartlett
Modified: 2022-04-12 18:56 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2022-04-11 21:27:00 UTC
See https://oss-fuzz-build-logs.storage.googleapis.com/log-67843a3f-cf4f-4515-9e01-fcfa9e692aa0.txt for the full log:

 ../../source3/winbindd/winbindd_pam.c:2879:7: error: variable 'validation_level' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
                 if (!(state->request->flags & WBFLAG_BIG_NTLMV2_BLOB) ||
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ../../source3/winbindd/winbindd_pam.c:3003:6: note: uninitialized use occurs here
             validation_level,
             ^~~~~~~~~~~~~~~~
 ../../source3/winbindd/winbindd_pam.c:2879:3: note: remove the 'if' if its condition is always false
                 if (!(state->request->flags & WBFLAG_BIG_NTLMV2_BLOB) ||
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ../../source3/winbindd/winbindd_pam.c:2879:7: error: variable 'validation_level' is used uninitialized whenever '||' condition is true [-Werror,-Wsometimes-uninitialized]
                 if (!(state->request->flags & WBFLAG_BIG_NTLMV2_BLOB) ||
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ../../source3/winbindd/winbindd_pam.c:3003:6: note: uninitialized use occurs here
             validation_level,
             ^~~~~~~~~~~~~~~~
 ../../source3/winbindd/winbindd_pam.c:2879:7: note: remove the '||' if its condition is always false
                 if (!(state->request->flags & WBFLAG_BIG_NTLMV2_BLOB) ||
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ../../source3/winbindd/winbindd_pam.c:2853:27: note: initialize the variable 'validation_level' to silence this warning
         uint16_t validation_level;
                                  ^
                                   = 0
 1 warning and 2 errors generated.
Comment 1 Jeremy Allison 2022-04-11 21:38:02 UTC
Oh I missed that, sorry. I see the problem now. On erroring out to the 'done:' label it calls log_authentication() with the uninitialized 'validation_level'.
Comment 2 Jeremy Allison 2022-04-11 21:43:05 UTC
The call to log_authentication() also takes 'union netr_Validation *validation' which can be NULL if the check for a valid request here:

        if (state->request->data.auth_crap.lm_resp_len > sizeof(state->request->data.auth_crap.lm_resp)
                || state->request->data.auth_crap.nt_resp_len > sizeof(state->request->data.auth_crap.nt_resp)) {
                if (!(state->request->flags & WBFLAG_BIG_NTLMV2_BLOB) ||
                     state->request->extra_len != state->request->data.auth_crap.nt_resp_len) {
                        DEBUG(0, ("winbindd_pam_auth_crap: invalid password length %u/%u\n",
                                  state->request->data.auth_crap.lm_resp_len,
                                  state->request->data.auth_crap.nt_resp_len));
                        result = NT_STATUS_INVALID_PARAMETER;
                        goto done;
                }
        }

fails with NT_STATUS_INVALID_PARAMETER.
Comment 3 Jeremy Allison 2022-04-11 22:29:07 UTC
Also, if winbind_dual_SamLogon() fails and returns !NT_STATUS_IS_OK() then we 'goto done' and winbind_dual_SamLogon() won't have set the &validation_level or &validation parameters that we need to pass to log_authentication().

The problem was d7739859e9cf:

    s3:winbind: Refactor log_authentication(), do not take winbindd_cli_state struct parameter
    
    Later winbindd_dual_pam_auth() will be converted to a local RPC call
    handler and it will not receive a winbindd_cli_state parameter. Avoid
    passing this struct around.

Samuel, how do you think we should fix this up ?
Comment 4 Samuel Cabrero 2022-04-12 09:40:30 UTC
(In reply to Jeremy Allison from comment #3)

Initializing validation_level to 0 will fix the problem. I will send a merge request.

Commit d7739859e9cf also changed the log_authentication() to handle a NULL validation argument. The validation_level is only used when validation != NULL, but the fuzzer complains anyway.
Comment 5 Samba QA Contact 2022-04-12 18:55:03 UTC
This bug was referenced in samba master:

7880537674ccfec42fb37a682dadc1a0f848947e
Comment 6 Jeremy Allison 2022-04-12 18:56:34 UTC
Closing - doesn't need back-port as this never got into a release.