[PATCH] Updated Add detailed authentication logging for NTLM authentication.

[Andrew Bartlett]

On Fri, 2017-03-03 at 10:31 +0100, Stefan Metzmacher wrote:
> Hi Andrew,
> 
> > > I haven't looked very closely, but is some places I had the
> > > impression
> > > that a later patch fixes a problem in a former patch.
> > > In order to understand the flow better, it would be useful to
> > > have
> > > this
> > > sorted out and every single commit complies and is supposed to
> > > work without crashing with NULL pointer deferences.
> > 
> > Thanks.  Each commit does compile (we have been using your rebase/x
> > make -j trick), and they are meant to operate, but we will look
> > again
> > for any more cases we need to implement. 
> 
> The main place was the netlogon server changes where one commit
> moves some code up (maybe dereferencinf the wrong union arm)
> and later down again,

I'll look at that one again.  We thought we had fixed that one! :-)

> Maybe 'x make -j test TESTS="..."' with a subset of relevant tests
> would be good. Just add an smb_panic(__location__) to each of the
> relevant code path, SMB1 auth with and without SPNEGO in both servers
> and SMB2 auth in both server, RPC bind in all servers, LDAP simple
> and
> sasl bind
> in the server, both ntlm_auth tools. If you do that one at a time
> with
> the full patchset,
> you should easily find the few relevant tests to trigger each code
> path.

Good point.

> > > Typically it's better to pass new unused arguments from the top,
> > > before they get used at the bottom.
> > > 
> > > I guess you also need to be prepared that a dcerpc bind
> > > negotiates no
> > > presentation context, it's possible to use a dummy uuid and just
> > > do the authentication.
> > 
> > Very interesting.  I'll cover that - but what happens in Samba
> > then,
> > can the user re-use the authentication on another UUID? (it is
> > important to know this for auditing.)
> 
> It's similar as with SMB sessions vs. with multiple tree connects.
> In both cases I guess you'd need a separate audit event for the
> resource access with a given auth_session_info. This is also
> important for DCERPC over smb where you don't have DCERPC
> authentication
> but still access a particular interface with the inherited
> auth_session_info from the transport connection.

I've caught that in the DCE/RPC bind without authentication I think,
but clearly I need some help.

Should I do the logging at the iface->bind() call instead/additionally?

> I guess the auth_description should be just something like
> "DCERPC(auth_type=%d,auth_level=%d)" instead of using the
> interface name.

I can change the service name back to dcerpc if you prefer.  I'm trying
not to build compound strings with printf here, because we want to have
individual details in each element in the json.  

Also, while in-scope for the project if we have time at the end, we
have not yet tackled logging the level of protection.

(If you can point me at where we know we will be doing SMB encryption
it will help to plumb a distinct transport_protection up the stack,
because it isn't seen in what GENSEC negotiates - boo, hiss).

The auth_type part is logged already as the auth description, where we
know if krb5, ntlmssp or schannel was used.

> For SMB we may also want to have the selected protocol dialect
> in the auth_description. I think that is important information.

Can you point me at where I get that string?  Again, this would be the
service (the auth_description is for strings like "ncacn_np", "ntlmssp"
or "krb5", "network" or "interactive" - so far - eg how the user
authenticated). 

Note that I would prefer to incorrectly log everything as "cifs" than
have 10 (and growing) different strings an admin later has to filter on
to know they a auditing file server access.  That is why I limited it
to just SMB and SMB2.  This isn't the one and only debug line in Samba!
:-)

> Maybe it's also useful to have logon_server in the audit log.
> Maybe also the user_flags (thinks like NETLOGON_GRACE_LOGON would be
> important)
> or even more stuff (except the session keys) from the netr_Sam*Info*
> structures.

The plan is to make the JSON reasonably verbose, and easy to extend.  

Gary has got started on that, hopefully we can get in a base and then
we can add some more.  I similarly thought that the logon server would
be a handy thing to log.

> We should also try to make our server much better in filling
> the netr_SamBaseInfo details.
> 
> But testing with Windows 2008R2 just showed that the
> NETLOGON_GRACE_LOGON
> is always 0, even for logons with the old password.

Fun :-)

> > > We may also want to distinguish between the different
> > > LogonSamLogon
> > > levels (interactive vs. network at least).
> > 
> > A very good idea.  We have an unused parameter that would be ideal
> > for
> > that. 
> 
> I thought you whould use use auth_descriptions like
> "SamLogon(Interactive)",
> "SamLogon(InteractiveTransitive)"...

I can use the full names if you like.  The SamLogon is already recorded
as the service. 

> > > I'm also wondering why the log functions gets the user supplied
> > > info
> > > but not the full auth_session_info.
> > 
> > At the point that we see the user_supplied_info, we don't get the
> > auth_session_info, this is generated later.  Because of the
> > differences
> > between the source3 and source4 auth subsystems, I can't even pass
> > in
> > the user_info_dc etc, so we just pick out some key parameters. 
> 
> Ok.
> 
> > > The output format should also be flexible, we can't guarantee
> > > that
> > > we'll generate the exact output forever.
> > 
> > Indeed!  This work is just the first step, designed to be practical
> > to
> > read, the full task from our clients calls for JSON (one one line),
> > which is the next step now we have the plumbing in place.
> 
> Yes, but still the JSON needs to flexible and not written in stone,
> I'm not sure but maybe it would be good to a some kind of version
> number.

That's what we are aiming for.  We can add a version, but because it is
an extensible structure, it isn't really needed, because we can just
add more elements. 

Gary spent much time at Catalyst working on a project for parsing log
entries and storing them in a database, so he knows and is in contact
with those who have lost much hair to regular expressions over horrible
log lines.  So I have pretty high hopes that this will be one of the
better ones ;-)

Thanks!

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba