Bug 9262 - checks for the validity of the NC in updaterefs are not complete
checks for the validity of the NC in updaterefs are not complete
Status: NEW
Product: Samba 4.0
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
4.0.0rc2
All All
: P5 normal
: ---
Assigned To: Andrew Bartlett
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-08 06:48 UTC by Matthieu Patou
Modified: 2015-07-31 08:17 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch to fix the problem (1.93 KB, patch)
2012-10-13 07:41 UTC, Matthieu Patou
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthieu Patou 2012-10-08 06:48:11 UTC
It's possible to pass a bogus NC and still receive a WERR_OK return code.
Comment 1 Matthieu Patou 2012-10-13 07:41:59 UTC
Created attachment 8063 [details]
Proposed patch to fix the problem
Comment 2 Andrew Bartlett 2012-10-14 06:17:56 UTC
Comment on attachment 8063 [details]
Proposed patch to fix the problem

This looks good, but the GUID is never validated here.  All checks are only of the DN string form. 

(We could/should validate the GUID as well, if sent over DRS).
Comment 3 Matthieu Patou 2012-10-14 07:37:37 UTC
(In reply to comment #2)
> Comment on attachment 8063 [details]
> Proposed patch to fix the problem
> 
> This looks good, but the GUID is never validated here.  All checks are only of
> the DN string form. 
It is the drs_ObjectIdentifier_to_dn function use the nc->guid and nc->sid (if present) and nc->dn to create a DN.

Then we search for this DN and store it in nc_root, then we compare nc_root and this dn if they are not the same we bail out.

So if you pass a good DN but a bogus GUID the search will return either no DN or the DN of another object, in both cases it will fail in tests.
If you pass a correct GUID but a bogus DN, the search will return you the DN of the NC but the comparison between the specified DN and the nc_root will fail.
Comment 4 Matthieu Patou 2012-10-14 07:51:52 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 8063 [details] [details]
> > Proposed patch to fix the problem
> > 
> > This looks good, but the GUID is never validated here.  All checks are only of
> > the DN string form. 
> It is the drs_ObjectIdentifier_to_dn function use the nc->guid and nc->sid (if
> present) and nc->dn to create a DN.
> 
> Then we search for this DN and store it in nc_root, then we compare nc_root and
> this dn if they are not the same we bail out.
> 
> So if you pass a good DN but a bogus GUID the search will return either no DN
> or the DN of another object, in both cases it will fail in tests.
> If you pass a correct GUID but a bogus DN, the search will return you the DN of
> the NC but the comparison between the specified DN and the nc_root will fail.

Well there is still a case when it's gonna work when it shouldn't: you pass the GUID of CN=Computers,DC=domain,DC=tld but specify DC=domain,DC=tld as the dn then the search for the nc will return you the nc with a dn: DC=domain,DC=tld
Comment 5 Andrew Bartlett 2012-10-14 10:51:46 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 8063 [details] [details]
> > Proposed patch to fix the problem
> > 
> > This looks good, but the GUID is never validated here.  All checks are only of
> > the DN string form. 
> It is the drs_ObjectIdentifier_to_dn function use the nc->guid and nc->sid (if
> present) and nc->dn to create a DN.
> 
> Then we search for this DN and store it in nc_root, then we compare nc_root and
> this dn if they are not the same we bail out.
> 
> So if you pass a good DN but a bogus GUID the search will return either no DN
> or the DN of another object, in both cases it will fail in tests.
> If you pass a correct GUID but a bogus DN, the search will return you the DN of
> the NC but the comparison between the specified DN and the nc_root will fail.

That's not actually how it works.

Indeed, have you checked what happens if you specify just a GUID, or a GUID and an DN?  Just a GUID will fail (the find_nc function doesn't search the DB, so can't determine paternity without the string form).  Specifying the GUID and DN may well also fail, but later on, as we are strict about not allowing a GUID and and DN in an ldb_dn, except in ldb modules.  (This is to prevent attacks like Jelmer suggested).

What currently happens if we don't check this properly?  If it's not catastrophic, I'm thinking to move to -1 on this, as I think this needs more work.
Comment 6 Michael Adam 2012-10-14 19:18:29 UTC
Assigning to Karolin for inclusion into v4-0-test
Comment 7 Matthieu Patou 2012-10-14 21:01:53 UTC
No this is not one of the critical patch that needs to be in rc3.

The 'hic' is that this patch is already in master because I did it with metze and because it fixed the failed test that we had in the DRSR test suite and because it didn't break master.

I'll come soon with a better patch
Comment 8 Karolin Seeger 2013-06-27 09:12:14 UTC
(In reply to comment #7)
> No this is not one of the critical patch that needs to be in rc3.
> 
> The 'hic' is that this patch is already in master because I did it with metze
> and because it fixed the failed test that we had in the DRSR test suite and
> because it didn't break master.
> 
> I'll come soon with a better patch

Any news on this one?
Comment 9 Matthieu Patou 2013-07-01 05:39:20 UTC
I need to retest with the DRSR testsuite to see what was wrong.
Comment 10 Stefan Metzmacher 2013-08-29 07:07:38 UTC
No 4.1 blocker => 4.2