Bug 29373

Summary: libicccm: xcb_get_wm_class_from_reply() causes reads beyond end of buffer
Product: XCB Reporter: Uli Schlachter <psychon>
Component: UtilsAssignee: xcb mailing list dummy <xcb>
Status: RESOLVED FIXED QA Contact: xcb mailing list dummy <xcb>
Severity: minor    
Priority: medium    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Sample app setting an invalid WM_CLASS to trigger the bug in xcb_get_wm_class_from_reply()
Proposed bug fix

Description Uli Schlachter 2010-08-03 01:58:31 UTC 
Created attachment 37536 [details]
Sample app setting an invalid WM_CLASS to trigger the bug in xcb_get_wm_class_from_reply()

Hi,

ICCCM mandates that WM_CLASS must be contain "instance\0class\0". Both entries have to be NULL-terminated. xcb_get_wm_class_from_reply() relies on this, but apparently some apps don't do this.

The code in question looks like this: ( http://cgit.freedesktop.org/xcb/util/tree/icccm/icccm.c#n330 )

  prop->_reply = reply;
  prop->instance_name = (char *) xcb_get_property_value(prop->_reply);

  int name_len = strlen(prop->instance_name);
  if(name_len == xcb_get_property_value_length(prop->_reply))
    name_len--;

  prop->class_name = prop->instance_name + name_len + 1;

If the instance is not NULL terminated, strlen() will read beyond the end of the buffer. If the class is not NULL terminated, the calling application will read beyond the end of the buffer when it tries to use the results of this call.

The attached application sets such an invalid WM_NAME on its window. The value used is 8 bytes long to avoid padding in the wire protocol.

This was found via java's usual brokeness and the awesome WM:
http://awesome.naquadah.org/bugs/index.php?do=details&task_id=790#comment2214

Cheers,
Uli
Comment 1 Peter Harris 2010-08-03 04:55:30 UTC 
Created attachment 37537 [details] [review]
Proposed bug fix

Could you please try the attached patch and let me know if it works for you?

Thanks.
Comment 2 Uli Schlachter 2010-08-03 05:30:58 UTC 
Patch works as expected.
I can't say that I like this since it will mean lots of new bug reports for broken apps, but it fixes this issue. Thanks.

As a side-note: I took a look at libx11's XGetClassHint() and it's affected, too (It uses strlen()).
Comment 3 Peter Harris 2010-08-03 07:21:44 UTC 
Pushed.

Thanks for the report, and thanks for testing.
Comment 4 Alan Coopersmith 2010-08-03 07:44:48 UTC 
BTW, I reported the Java bug (failure to NULL terminate WM_CLASS) to the 
Java team when I hit this during the xwininfo port to xcb:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6961123

(In reply to comment #2)
> As a side-note: I took a look at libx11's XGetClassHint() and it's affected,
> too (It uses strlen()).

It works because it calls XGetWindowProperty to get the string, and Xlib always
appends a trailing NULL to the string it returns from XGetWindowProperty.

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.