#1697 sssd: incorrect checks on length values during packet decoding
Closed: Fixed None Opened 6 years ago by jhrozek.

https://bugzilla.redhat.com/show_bug.cgi?id=883886 (Red Hat Enterprise Linux 6)

In several places, length values are checked against the outer PDU length in an
incorrect way.  (This is distinct from bug 883878, where the checks are missing
altogether.)  I don't think a trust boundary is corssed in the following cases,
so this is not a security bug, just a regular bug, but double-checking this
assessment is appreciated.  (The risk is that with such code around, it's used
as a model for code where the check is important.)

As a rule of thumb, these checks should be written in such a way that the value
being checked stands isolated on the left hand side of the comparison against
the remaining length.  The remaining length must be computed from trusted
values (thus avoiding overflow).  The value being checked must be of an
unsigned type.

The following notes are based on the 1.9.2 sources.

src/providers/krb5/krb5_child.c: unpack_buffer:
    if ((p + len ) > size) return EINVAL;
Check should be:
    if (len > size - p) return EINVAL;

src/providers/krb5/krb5_child_handler.c: parse_krb5_child_response:
Problematic checks is:
        if ((p + msg_len) > len) {

src/providers/ldap/ldap_child.c: unpack_buffer: incorrect check:
        if ((p + len ) > size) return EINVAL;

src/providers/ldap/sdap_child_helpers.c: parse_child_response:
incorrect check:
    if ((p + len ) > size) return EINVAL;

src/sss_client/autofs/sss_autofs.c: In,
sss_getautomntent_data_return() the check if (len +
sss_getautomntent_data.ptr > sss_getautomntent_data.len) { is
incorrect, len should stand on the LHS alone (multiple instances
follow).  Same problem in sss_getautomntent_data_return() with vallen,
but _sss_getautomntbyname_r() is correct.

src/sss_client/ssh/sss_ssh_client.c: sss_ssh_get_ent() contains an
incorrect length check:
        if (rep_len-c < len + sizeof(uint32_t)) {

Fields changed

blockedby: =>
blocking: =>
coverity: =>
design: =>
design_review: => 0
feature_milestone: =>
fedora_test_page: =>
milestone: NEEDS_TRIAGE => SSSD 1.10 beta
testsupdated: => 0

Fields changed

selected: => Not need

Moving tickets that are not a priority for SSSD 1.10 into the next release.

milestone: SSSD 1.10 beta => SSSD 1.11 beta

Fields changed

mark: => 0

Required for downstream, but not for Beta

changelog: =>
milestone: SSSD 1.13 beta => SSSD 1.13
review: => 0
sensitive: => 0

This ticket has a downstream BZ link, bumping priority

priority: major => critical

Fields changed

owner: somebody => mzidek

Fields changed

patch: 0 => 1

Patch was submitted, should be included in 1.13.1

milestone: SSSD 1.13.2 => SSSD 1.13.1

resolution: => fixed
status: new => closed

Metadata Update from @jhrozek:
- Issue assigned to mzidek
- Issue set to the milestone: SSSD 1.13.1

2 years ago

Login to comment on this ticket.