[SSSD] [PATCH] Guard against invalid DP messages

Lukas Slebodnik lslebodn at redhat.com
Fri Nov 13 12:58:08 UTC 2015


On (13/11/15 13:40), Sumit Bose wrote:
>On Wed, Nov 11, 2015 at 12:00:31PM +0100, Jakub Hrozek wrote:
>> On Mon, Nov 09, 2015 at 03:14:31PM +0100, Jakub Hrozek wrote:
>> > On Mon, Nov 09, 2015 at 12:44:17PM +0100, Sumit Bose wrote:
>> > > On Fri, Nov 06, 2015 at 09:32:23PM +0100, Jakub Hrozek wrote:
>> > > > On Fri, Nov 06, 2015 at 09:13:07PM +0100, Jakub Hrozek wrote:
>> > > > > On Fri, Nov 06, 2015 at 04:59:58PM +0100, Sumit Bose wrote:
>> > > > > > > @@ -658,11 +678,7 @@ static void get_subdomains_callback(struct be_req *req,
>> > > > > > >           */
>> > > > > > >          err_maj = dp_err_type;
>> > > > > > >          err_min = errnum;
>> > > > > > > -        if (errstr) {
>> > > > > > > -            err_msg = errstr;
>> > > > > > > -        } else {
>> > > > > > > -            err_msg = dp_err_to_string(dp_err_type);
>> > > > > > > -        }
>> > > > > > > +        err_msg = safe_be_req_err_msg(errstr, dp_err_type);
>> > > > > > 
>> > > > > > Wouldn't it be even more safe to do this check in
>> > > > > > sbus_request_return_and_finish() and not rely on the callers?
>> > > > > 
>> > > > > Something like the attached patch? I think we should check in
>> > > > > sbus_request_return_and_finish() in addition to checking in the callers,
>> > > > > not instead, because the string in the callback is just a diagnostics
>> > > > > message and we can recover by using some fallback.
>> > > > > 
>> > > > > If we get all the way to sbus_request_return_and_finish(), then failing
>> > > > > gracefully is better then abort() that libdbus calls, but it's even better
>> > > > > to finish the request with wrong error message then fail completely.
>> > > > > 
>> > > > > (I also still dislike that libdbus aborts...)
>> > > > 
>> > > > Also, self-nack. dbus_validate_utf8() is not available on RHEL-6:
>> > > >     http://sssd-ci.duckdns.org/logs/job/32/17/rhel6/ci-build-debug/ci-make-tests.log
>> > > > 
>> > > > I guess we can limit these checks to new OSs or use sss_utf8_check() on
>> > > > older platforms..
>> > > 
>> > > Why not always use sss_utf8_check()? Is there an advantage using
>> > > dbus_validate_utf8()?
>> > 
>> > I don't know how dbus_validate_utf8() works internally, so I tried to
>> > stick to the dbus-provided version to check dbus types.
>> > 
>> > But if both do the same thing (If they don't it would be a bug, sure),
>> > we can stick to sss_utf8_check()
>> > 
>> > (Another advantage might be that we could get away with linking against
>> > libdbus only)
>> 
>> Hi,
>> 
>> attached are patches that only use sss_utf8_check() and work on RHEL-6
>> also.
>
>The patches look good and fixing the issue as expected. CI passes as
>well http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6
>failure is unrelated). ACK
>
>I just have a minor comment
>
>>  
>> +static const char *safe_be_req_err_msg(const char *msg_in,
>> +                                       int dp_err_type)
>> +{
>> +    bool ok;
>> +
>> +    if (msg_in == NULL) {
>> +        /* No custom error, just use default */
>> +        return dp_err_to_string(dp_err_type);
>> +    }
>> +
>> +    ok = sss_utf8_check((const uint8_t *) msg_in,
>> +                        strlen(msg_in));
>> +    if (!ok) {
>> +        DEBUG(SSSDBG_MINOR_FAILURE,
>> +              "Back end message is invalid, using default\n");
>
>Maybe something like
>
>        DEBUG(SSSDBG_MINOR_FAILURE,
>              "Back end message [%s] contains invalid non-UTF8 character, " \
                                                                            ^^
                                                                Sumit, FYI. The
                                           concatenation by preporcessor will
                                           happen even without backslash at
                                           then end of line.

LS


More information about the sssd-devel mailing list