It's possible to pass a bogus NC and still receive a WERR_OK return code.
Created attachment 8063 [details] Proposed patch to fix the problem
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).
(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.
(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
(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.
Assigning to Karolin for inclusion into v4-0-test
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
(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?
I need to retest with the DRSR testsuite to see what was wrong.
No 4.1 blocker => 4.2