Bug 17803

Summary: Panic from dbus_signature_validate (CVE-2008-3834, CVE-2009-1189)
Product: dbus Reporter: schelte
Component: coreAssignee: Colin Walters <walters>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: minor    
Priority: low CC: herrold, hp, n-roeser, walters
Version: 1.2.x   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: validate type before we evaluate it
0001-Bug-17803-Fix-both-test-case-and-validation-logic.patch

Description schelte 2008-09-27 03:13:20 UTC
Function dbus_signature_validate() panics when asked to validate the type signature "a{(ii)i}". 

According to the docs, dbus_signature_validate() can be used to check a type signature for validity. So, it should indicate the mentioned type signature is invalid. It should not abort the whole program.

This was tested on SuSE 11.0, with libdbus 1.2.1.

Code to reproduce the error:

#include <dbus/dbus.h>

int main ()
{
   return !dbus_signature_validate("a{(ii)i}", NULL);
}

Result:

process 7016: arguments to dbus_type_is_basic() were incorrect, assertion "_dbus_type_is_valid (typecode) || typecode == DBUS_TYPE_INVALID" failed in file dbus-signature.c line 310.
This is normally a bug in some application using the D-Bus library.
  D-Bus not built with -rdynamic so unable to print a backtrace
Aborted
Comment 1 Havoc Pennington 2008-09-27 07:47:08 UTC
Indeed, this is a bug. (Should add a unit test to avoid this happening again, along with the patch)
Comment 2 Havoc Pennington 2008-09-28 10:49:47 UTC
The issue seems to be this code that "peeks ahead" at a type code that has not been validated yet:

      if (last == DBUS_DICT_ENTRY_BEGIN_CHAR &&
          !dbus_type_is_basic (*p))
        {
          result = DBUS_INVALID_DICT_KEY_MUST_BE_BASIC_TYPE;
          goto out;
        }
Comment 3 Colin Walters 2008-09-29 14:13:37 UTC
Created attachment 19288 [details] [review]
validate type before we evaluate it

This looks like it should be sufficient.  After a brief study of the function I didn't see any obvious similar problems.
Comment 4 Havoc Pennington 2008-09-30 19:21:17 UTC
Comment on attachment 19288 [details] [review]
validate type before we evaluate it

Looks great to me
Comment 5 Colin Walters 2008-10-01 10:50:34 UTC
Thanks for the review, applied:

commit 7b10b46c5c8658449783ce45f1273dd35c353bce
Author: Colin Walters <walters@verbum.org>
Date:   Wed Oct 1 13:49:48 2008 -0400

    Bug 17803: Panic from dbus_signature_validate
    
    	* dbus/dbus-marshal-validate.c: Ensure we validate
    	a basic type before calling is_basic on it.
    	* dbus-marshal-validate-util.c: Test.

Comment 6 Josh Bressers 2008-10-06 09:44:36 UTC
This flaw has been assigned CVE-2008-3834
Comment 7 Nico R. 2009-03-31 15:32:02 UTC
Shouldn't a comma be appended to the line preceding the line which is added to dbus/dbus-marshal-validate-util.c?
The patch currently does not add a test, but changes the existing last one in the array (the compiler concatenates the string constants).
Comment 8 Colin Walters 2009-04-01 09:05:02 UTC
Ok, so both the test case *and* the validation logic patch were wrong.  Wow.

Comment 9 Colin Walters 2009-04-01 09:05:35 UTC
Created attachment 24436 [details] [review]
0001-Bug-17803-Fix-both-test-case-and-validation-logic.patch
Comment 10 Colin Walters 2009-04-16 09:30:22 UTC
John or Havoc, any chance either of you could take a quick look at this updated patch?
Comment 11 Colin Walters 2009-04-16 10:44:36 UTC
This updated flaw has been assigned CVE-2009-1189.
Comment 12 Havoc Pennington 2009-04-16 12:41:45 UTC
Ouch, I guess it's true you should be sure the test fails before fixing it! (I'm always too lazy myself, but sometimes it bites)

The patch looks correct to me, but then, it did last time too...

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.