[PATCH] Avoid privileged segfault in GetNCChanges and add many more tests

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Avoid privileged segfault in GetNCChanges and add many more tests

Samba - samba-technical mailing list
This patch set fixes bug 12946 and adds a number of tests to ensure
that there are not other similar issues waiting to be uncovered.

I don't change our server behaviour to match windows exactly, as I
prefer that we keep our server behaviour of checking the ACLs first.

Please review and push!

Thanks,

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




bug-12946-and-tests.patch.txt (74K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
Hi Andrew,

> - sid = pytalloc_get_ptr(py_sid);
> + if (PyObject_TypeCheck(py_sid, &dom_sid_Type)) {
> + sid = pytalloc_get_ptr(py_sid);
> + } else {
> + PyErr_SetString(PyExc_TypeError,
> + "expected security.dom_sid "
> + "for second argument to .from_sddl");
> + return NULL;
> +
> + }
>  
>   secdesc = sddl_decode(NULL, sddl, sid);
>   if (secdesc == NULL) {
> --
Can we please agree on using an "error and out logic" for new code?
There's really no good reason to have the additional indentation
for the "sid = pytalloc_get_ptr(py_sid);" line!
I thought this was part of README.Coding for a long time already,
but I can't find it...

The above example seems harmless, but I notice new examples
almost every time I review patches from you or once you reviewed.
These act as bad examples others will likely copy'n'paste or
at least learn from.

This morning I also noticed

+ if (ret == LDB_SUCCESS) {
+ if (result->count == 0) {
+ ret = LDB_ERR_NO_SUCH_OBJECT;
+ } else {
+ struct ldb_message *match =
+ get_best_match(dn, result);
+ if (match == NULL) {
+ TALLOC_FREE(frame);
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+ *msg = talloc_move(mem_ctx, &match);
+ }
+ }
=> here
+ TALLOC_FREE(frame);
+ return ret;

Which adds a lot of useless indentation and it makes it really
hard to add new code where I added "here", as you need remember
and understand all the logic be had above or protect
the new code by using another "if (ret == LDB_SUCCESS) {"

It should be:

        if (ret != LDB_SUCCESS) {
                TALLOC_FREE(frame);
                return ret;
        }

        if (result->count == 0) {
                TALLOC_FREE(frame);
                return LDB_ERR_NO_SUCH_OBJECT;
        }

        match = get_best_match(dn, result);
        if (match == NULL) {
                TALLOC_FREE(frame);
                return LDB_ERR_OPERATIONS_ERROR;
        }

        *msg = talloc_move(mem_ctx, &match);
        TALLOC_FREE(frame);
        return LDB_SUCCESS; <= This is important to be not "return ret;"

Untangling these kind of spaghetti was the major pain I had when
working on spnego.c, it took days in order to understand the logic
of some functions.

I know we have to live with our existing code for a long time,
but we should really try hard to avoid it for new code.

I started to write such a mail already a few weeks ago,
but discarded it in order to enjoy my vacation...

(I hope this mail is not too harsh, but reviewing this of stuff
ruins my day)

Thanks and sorry...
metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
On Wed, 2017-08-09 at 09:14 +0200, Stefan Metzmacher via samba-
technical wrote:

> Hi Andrew,
>
> > - sid = pytalloc_get_ptr(py_sid);
> > + if (PyObject_TypeCheck(py_sid, &dom_sid_Type)) {
> > + sid = pytalloc_get_ptr(py_sid);
> > + } else {
> > + PyErr_SetString(PyExc_TypeError,
> > + "expected security.dom_sid "
> > + "for second argument to
> > .from_sddl");
> > + return NULL;
> > +
> > + }
> >  
> >   secdesc = sddl_decode(NULL, sddl, sid);
> >   if (secdesc == NULL) {
> > -- 
>
> Can we please agree on using an "error and out logic" for new code?
> There's really no good reason to have the additional indentation
> for the "sid = pytalloc_get_ptr(py_sid);" line!
> I thought this was part of README.Coding for a long time already,
> but I can't find it...
>
> The above example seems harmless, but I notice new examples
> almost every time I review patches from you or once you reviewed.
> These act as bad examples others will likely copy'n'paste or
> at least learn from.
>
> This morning I also noticed
>
> + if (ret == LDB_SUCCESS) {
> + if (result->count == 0) {
> + ret = LDB_ERR_NO_SUCH_OBJECT;
> + } else {
> + struct ldb_message *match =
> + get_best_match(dn, result);
> + if (match == NULL) {
> + TALLOC_FREE(frame);
> + return LDB_ERR_OPERATIONS_ERROR;
> + }
> + *msg = talloc_move(mem_ctx, &match);
> + }
> + }
> => here
> + TALLOC_FREE(frame);
> + return ret;
>
> Which adds a lot of useless indentation and it makes it really
> hard to add new code where I added "here", as you need remember
> and understand all the logic be had above or protect
> the new code by using another "if (ret == LDB_SUCCESS) {"
>
> It should be:
>
> if (ret != LDB_SUCCESS) {
> TALLOC_FREE(frame);
> return ret;
> }
>
> if (result->count == 0) {
> TALLOC_FREE(frame);
> return LDB_ERR_NO_SUCH_OBJECT;
> }
>
> match = get_best_match(dn, result);
> if (match == NULL) {
> TALLOC_FREE(frame);
> return LDB_ERR_OPERATIONS_ERROR;
> }
>
> *msg = talloc_move(mem_ctx, &match);
> TALLOC_FREE(frame);
> return LDB_SUCCESS; <= This is important to be not "return
> ret;"
>
> Untangling these kind of spaghetti was the major pain I had when
> working on spnego.c, it took days in order to understand the logic
> of some functions.
>
> I know we have to live with our existing code for a long time,
> but we should really try hard to avoid it for new code.
>
> I started to write such a mail already a few weeks ago,
> but discarded it in order to enjoy my vacation...
>
> (I hope this mail is not too harsh, but reviewing this of stuff
> ruins my day)
>
> Thanks and sorry...
> metze

Thanks metze,
readability and modifiability of code are very important, and become
even more important as a project grows and ages.

It would be nice to add a style section to the docs that more
forcefully and clearly explain these subtle coding style issues.

Simo.

Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
On Wed, Aug 09, 2017 at 09:10:22AM -0400, Simo via samba-technical wrote:
> Thanks metze,
> readability and modifiability of code are very important, and become
> even more important as a project grows and ages.
>
> It would be nice to add a style section to the docs that more
> forcefully and clearly explain these subtle coding style issues.

patch attached.

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
On Wed, 2017-08-09 at 15:26 +0200, Ralph Böhme wrote:

> On Wed, Aug 09, 2017 at 09:10:22AM -0400, Simo via samba-technical
> wrote:
> > Thanks metze,
> > readability and modifiability of code are very important, and
> > become
> > even more important as a project grows and ages.
> >
> > It would be nice to add a style section to the docs that more
> > forcefully and clearly explain these subtle coding style issues.
>
> patch attached.
>
> Cheerio!
> -slow

Reviewed-by: Simo Sorce <[hidden email]>

Simo.

Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Aug 09, 2017 at 03:26:29PM +0200, Ralph Böhme via samba-technical wrote:
> On Wed, Aug 09, 2017 at 09:10:22AM -0400, Simo via samba-technical wrote:
> > Thanks metze,
> > readability and modifiability of code are very important, and become
> > even more important as a project grows and ages.
> >
> > It would be nice to add a style section to the docs that more
> > forcefully and clearly explain these subtle coding style issues.
>
> patch attached.

updated version removing the unrelated hunk. Sorry!

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
On Wed, 2017-08-09 at 15:42 +0200, Ralph Böhme via samba-technical
wrote:

> On Wed, Aug 09, 2017 at 03:26:29PM +0200, Ralph Böhme via samba-
> technical wrote:
> > On Wed, Aug 09, 2017 at 09:10:22AM -0400, Simo via samba-technical
> > wrote:
> > > Thanks metze,
> > > readability and modifiability of code are very important, and
> > > become
> > > even more important as a project grows and ages.
> > >
> > > It would be nice to add a style section to the docs that more
> > > forcefully and clearly explain these subtle coding style issues.
> >
> > patch attached.
>
> updated version removing the unrelated hunk. Sorry!
>
> Cheerio!
> -slow

still RB by me

Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, 2017-08-09 at 09:14 +0200, Stefan Metzmacher wrote:

> Hi Andrew,
>
> > - sid = pytalloc_get_ptr(py_sid);
> > + if (PyObject_TypeCheck(py_sid, &dom_sid_Type)) {
> > + sid = pytalloc_get_ptr(py_sid);
> > + } else {
> > + PyErr_SetString(PyExc_TypeError,
> > + "expected security.dom_sid "
> > + "for second argument to .from_sddl");
> > + return NULL;
> > +
> > + }
> >  
> >   secdesc = sddl_decode(NULL, sddl, sid);
> >   if (secdesc == NULL) {
> > --
>
> Can we please agree on using an "error and out logic" for new code?
> There's really no good reason to have the additional indentation
> for the "sid = pytalloc_get_ptr(py_sid);" line!
> I thought this was part of README.Coding for a long time already,
> but I can't find it...

Sure.  I'm happy to fix this up.

> The above example seems harmless, but I notice new examples
> almost every time I review patches from you or once you reviewed.
> These act as bad examples others will likely copy'n'paste or
> at least learn from.
>
> This morning I also noticed
>
> + if (ret == LDB_SUCCESS) {
> + if (result->count == 0) {
> + ret = LDB_ERR_NO_SUCH_OBJECT;
> + } else {
> + struct ldb_message *match =
> + get_best_match(dn, result);
> + if (match == NULL) {
> + TALLOC_FREE(frame);
> + return LDB_ERR_OPERATIONS_ERROR;
> + }
> + *msg = talloc_move(mem_ctx, &match);
> + }
> + }
> => here
> + TALLOC_FREE(frame);
> + return ret;
>
> Which adds a lot of useless indentation and it makes it really
> hard to add new code where I added "here", as you need remember
> and understand all the logic be had above or protect
> the new code by using another "if (ret == LDB_SUCCESS) {"
>
> It should be:
>
> if (ret != LDB_SUCCESS) {
> TALLOC_FREE(frame);
> return ret;
> }
>
> if (result->count == 0) {
> TALLOC_FREE(frame);
> return LDB_ERR_NO_SUCH_OBJECT;
> }
>
> match = get_best_match(dn, result);
> if (match == NULL) {
> TALLOC_FREE(frame);
> return LDB_ERR_OPERATIONS_ERROR;
> }
>
> *msg = talloc_move(mem_ctx, &match);
> TALLOC_FREE(frame);
> return LDB_SUCCESS; <= This is important to be not "return ret;"

I'll work with Gary to fix this one up.

> Untangling these kind of spaghetti was the major pain I had when
> working on spnego.c, it took days in order to understand the logic
> of some functions.
>
> I know we have to live with our existing code for a long time,
> but we should really try hard to avoid it for new code.
>
> I started to write such a mail already a few weeks ago,
> but discarded it in order to enjoy my vacation...
>
> (I hope this mail is not too harsh, but reviewing this of stuff
> ruins my day)

Do you have any thoughts on the substantive part of the patch, being
the segfault fix and the tests?

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


Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
On Thu, 2017-08-10 at 06:57 +1200, Andrew Bartlett via samba-technical
wrote:

> On Wed, 2017-08-09 at 09:14 +0200, Stefan Metzmacher wrote:
> > Hi Andrew,
> >
> > > - sid = pytalloc_get_ptr(py_sid);
> > > + if (PyObject_TypeCheck(py_sid, &dom_sid_Type)) {
> > > + sid = pytalloc_get_ptr(py_sid);
> > > + } else {
> > > + PyErr_SetString(PyExc_TypeError,
> > > + "expected security.dom_sid "
> > > + "for second argument to
> > > .from_sddl");
> > > + return NULL;
> > > +
> > > + }
> > >  
> > >   secdesc = sddl_decode(NULL, sddl, sid);
> > >   if (secdesc == NULL) {
> > > -- 
> >
> > Can we please agree on using an "error and out logic" for new code?
> > There's really no good reason to have the additional indentation
> > for the "sid = pytalloc_get_ptr(py_sid);" line!
> > I thought this was part of README.Coding for a long time already,
> > but I can't find it...
>
> Sure.  I'm happy to fix this up.
The attached patch series includes this fix.

Please review.

Thanks!

Andrew Bartlett

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




bug-12946-and-tests.patch.txt (73K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Andrew,

>>
>> Can we please agree on using an "error and out logic" for new code?
>> There's really no good reason to have the additional indentation
>> for the "sid = pytalloc_get_ptr(py_sid);" line!
>> I thought this was part of README.Coding for a long time already,
>> but I can't find it...
>
> Sure.  I'm happy to fix this up.
..
>
> I'll work with Gary to fix this one up.

Thanks! Please keep that in mind for future coding and review,
it'll make the life easier for all of us in the end.

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Andrew,

here's my version with some comments in it.

In general please check if we need the BUG: line
in every commit, and move commits without towards the end of the
patchset. (The BUG: line should be before the Signed-off-by: line
followed by an empty line...)

metze


tmp.diff.txt (75K) Download Attachment
signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
On Thu, 2017-08-10 at 13:04 +0200, Stefan Metzmacher wrote:
> Hi Andrew,
>
> here's my version with some comments in it.

Thanks,  I'll look at that next week (this 'simple fix' for the
bugzilla bug kind of ballooned!).

> In general please check if we need the BUG: line
> in every commit, and move commits without towards the end of the
> patchset.

The only commit needing the BUG is the one I tagged.  The rest is just
for master, to prove we don't regress in the future.

> (The BUG: line should be before the Signed-off-by: line
> followed by an empty line...)

I don't mind some rules, but where did this one come from?

(I often add the BUG: tag with a script, which is why they end up
there).

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


Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
Am 10.08.2017 um 13:14 schrieb Andrew Bartlett:

> On Thu, 2017-08-10 at 13:04 +0200, Stefan Metzmacher wrote:
>> Hi Andrew,
>>
>> here's my version with some comments in it.
>
> Thanks,  I'll look at that next week (this 'simple fix' for the
> bugzilla bug kind of ballooned!).
>
>> In general please check if we need the BUG: line
>> in every commit, and move commits without towards the end of the
>> patchset.
>
> The only commit needing the BUG is the one I tagged.  The rest is just
> for master, to prove we don't regress in the future.
Ok, then I may already push some of the good once, which are independent.

>> (The BUG: line should be before the Signed-off-by: line
>> followed by an empty line...)
>
> I don't mind some rules, but where did this one come from?

It's just the way a few thousand other commits look like.

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, 2017-08-10 at 13:04 +0200, Stefan Metzmacher wrote:

> Hi Andrew,
>
> here's my version with some comments in it.
>
> In general please check if we need the BUG: line
> in every commit, and move commits without towards the end of the
> patchset. (The BUG: line should be before the Signed-off-by: line
> followed by an empty line...)
>
> metze

Regarding:

> Subject: [PATCH 05/10] TODO: s4-drsuapi: Refuse to replicate an NC is that not
>  actually an NC
>
> This prevents replication of an OU, you must replicate a whole NC per Windows 2012R2
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
>
> TODO: shouldn't we use dsdb_search_one()?

No, I think dsdb_search_dn() is correct, as used often elsewhere.  We
could remove the check of count > 1, we can't get that unless we have
DB corruption, but we of course just had that...

I'm heads-down in the binary index work, but I'm grateful to Tim who
will tidy this up for us, so you can expect to see an improved set of
patches soon.

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


Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
Hi,

Sorry for the delay. I just realized that the bug-fix portion of this
work hasn't been delivered. Could we get that change (attached) into
master and 4.7? I'll send the rest of the patches in a follow-up email
shortly.

Thanks,
Tim


On 14/08/17 19:20, Andrew Bartlett via samba-technical wrote:

> On Thu, 2017-08-10 at 13:04 +0200, Stefan Metzmacher wrote:
>> Hi Andrew,
>>
>> here's my version with some comments in it.
>>
>> In general please check if we need the BUG: line
>> in every commit, and move commits without towards the end of the
>> patchset. (The BUG: line should be before the Signed-off-by: line
>> followed by an empty line...)
>>
>> metze
>
> Regarding:
>
>> Subject: [PATCH 05/10] TODO: s4-drsuapi: Refuse to replicate an NC is that not
>>  actually an NC
>>
>> This prevents replication of an OU, you must replicate a whole NC per Windows 2012R2
>>
>> Signed-off-by: Andrew Bartlett <[hidden email]>
>>
>> TODO: shouldn't we use dsdb_search_one()?
>
> No, I think dsdb_search_dn() is correct, as used often elsewhere.  We
> could remove the check of count > 1, we can't get that unless we have
> DB corruption, but we of course just had that...
>
> I'm heads-down in the binary index work, but I'm grateful to Tim who
> will tidy this up for us, so you can expect to see an improved set of
> patches soon.
>
> Thanks,
>
> Andrew Bartlett
>

drs-bug-fix.txt (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi,

Attached is an updated patch-set that should hopefully address your
review comments. It applies on top of the bug-fix patch I just sent.

This patch-set also contains a couple of new patches I added. I updated
the tests to send several bad GetNCChanges requests one after the other,
and it highlighted a problem where Samba could incorrectly 'accept'
these bad requests (note that although Samba didn't reject these bad
requests outright, it also didn't return any actual objects in the
response). I had to fix this problem in order to get the tests working
as expected.

Cheers,
Tim

On 14/08/17 19:20, Andrew Bartlett via samba-technical wrote:

> On Thu, 2017-08-10 at 13:04 +0200, Stefan Metzmacher wrote:
>> Hi Andrew,
>>
>> here's my version with some comments in it.
>>
>> In general please check if we need the BUG: line
>> in every commit, and move commits without towards the end of the
>> patchset. (The BUG: line should be before the Signed-off-by: line
>> followed by an empty line...)
>>
>> metze
>
> Regarding:
>
>> Subject: [PATCH 05/10] TODO: s4-drsuapi: Refuse to replicate an NC is that not
>>  actually an NC
>>
>> This prevents replication of an OU, you must replicate a whole NC per Windows 2012R2
>>
>> Signed-off-by: Andrew Bartlett <[hidden email]>
>>
>> TODO: shouldn't we use dsdb_search_one()?
>
> No, I think dsdb_search_dn() is correct, as used often elsewhere.  We
> could remove the check of count > 1, we can't get that unless we have
> DB corruption, but we of course just had that...
>
> I'm heads-down in the binary index work, but I'm grateful to Tim who
> will tidy this up for us, so you can expect to see an improved set of
> patches soon.
>
> Thanks,
>
> Andrew Bartlett
>

drs-changes.txt (77K) Download Attachment