[sssd PR#392][opened] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Author: jhrozek
 Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found
Action: opened

PR body:
"""
If a referral returned during AD GPO processing cannot be assigned to a
known domain, at the moment SSSD accesses memory that was freed previously
with ldap_free_urldesc().

This patch moves the ldap_free_urldesc() call to both the error handler and
the success branch after we are done working with the LDAPURLDesc instance.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/392/head:pr392
git checkout pr392

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[fidencio]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

fidencio commented:
"""
Code-wise it looks good. I've left a comment that may or may not be addressed depending on the contributor's opinion.

I'm firing a CI build soon for this patch.
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331081220

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[fidencio]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

fidencio commented:
"""
CI: http://vm-058-233.$%7Babc%7D/logs/job/78/05/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331126072

[sssd PR#392][+Accepted] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[fidencio]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

Label: +Accepted

[sssd PR#392][-Accepted] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

Label: -Accepted

[sssd PR#392][+Changes requested] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

Label: +Changes requested

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

lslebodn commented:
"""
This is something which should we test but it does not have any upstream ticket.
Or details how issue was noticed; how to reproduce ...
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331130099

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

jhrozek commented:
"""
I noticed this while looking at logs in a downstream bugzilla. Do you want a ticket to be filed?
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331130921

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

lslebodn commented:
"""
> I noticed this while looking at logs in a downstream bugzilla. Do you want a ticket to be filed?

We needn't have a ticket if patchset contains also test/unittest :-)
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331132631

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

lslebodn commented:
"""
> I noticed this while looking at logs in a downstream bugzilla. Do you want a ticket to be filed?

We needn't have a ticket if patchset contains also test/unittest :-)
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331132631

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

jhrozek commented:
"""
This is a minor fix for a use-after free during a failure in a static function. Writing a test would mean mocking quite a few interfaces (I guess, haven't tried). And currently I don't have any time for that.

If you think we can't accept this fix without a test, please just close this PR as rejected.
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331134478

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

lslebodn commented:
"""
I do not suggest rejecting this PR. Therefore upstream ticket + real integration(downstream) test will be enough. But for this we need a details in ticket. Or will it be complicated to write real integration test as well?
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331136553

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

jhrozek commented:
"""
Since this is use-after-free and the only affected place is a DEBUG message and even for the affected customer, the issue didn't cause a crash, the test would be sanity only, but otherwise I guess not too complex.
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331138141

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

lslebodn commented:
"""
On (21/09/17 12:13), Jakub Hrozek wrote:
> Since this is use-after-free and the only affected place is a DEBUG message and even for the affected customer, the issue didn't cause a crash, the test would be sanity only, but otherwise I guess not too complex.

If you are 100% sure that it is issue only in debug message then
we needn't have ticket for it. It was not clear from commit message
and I didn't try to dive to to details.

Based on commit message, I had an assumption that consequences of
use-after-free are little bit worse. Feel free to update commit message an
push. Use-after-free errors usually deserve test, but it does not worth check
garbage in debug message.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-331165778

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

jhrozek commented:
"""
yes, it's only about debug message. See:
```
    state->ref_domain = find_domain_by_name(state->host_domain,
                                            lud->lud_host, true);
---> here we use the valid lud for the last time before freeing it on the next line
    ldap_free_urldesc(lud);
    if (!state->ref_domain) {
        DEBUG(SSSDBG_CRIT_FAILURE,
              "Could not find domain matching [%s]\n",
              lud->lud_host);
---> use after free, after that we abort
        ret = EIO;
        goto done;
    }

state->conn = ad_get_dom_ldap_conn(state->access_ctx->ad_id_ctx,
                                       state->ref_domain);
    if (!state->conn) {
        DEBUG(SSSDBG_OP_FAILURE,
              "No connection for %s\n", state->ref_domain->name);
        ret = EINVAL;
        goto done;
    }

/* Get the hostname we're going to connect to.
     * We'll need this later for performing the samba
     * connection.
     */
ret = ldap_url_parse(state->conn->service->uri, &lud);
---> here we assign a new value to lud
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-332302996

[sssd PR#392][-Changes requested] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

Label: -Changes requested

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[fidencio]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

fidencio commented:
"""
Is there any request pending on this PR?

I'm adding back the "Accepted" label as per my review.
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-334071886

[sssd PR#392][+Accepted] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[fidencio]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

Label: +Accepted

[sssd PR#392][comment] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

jhrozek commented:
"""
* master: 381bc154ef06fd3cc0660ce0fd62504367f420f5
"""

See the full comment at https://github.com/SSSD/sssd/pull/392#issuecomment-334553504

[sssd PR#392][closed] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Author: jhrozek
 Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/392/head:pr392
git checkout pr392

[sssd PR#392][+Pushed] GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/392
Title: #392: GPO: Don't use freed LDAPURLDesc if domain for AD DC cannot be found

Label: +Pushed