Bug 40812

Summary: Narrow cast for span leads to divide by zero and crash in fixed table layout
Product: Security Reporter: Abhishek Arya <inferno>
Component: SecurityAssignee: WebKit Security Group <webkit-security-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: abarth, ddkilzer, hyatt, jamesr, levin, yong.li.webkit
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch abarth: review+

Description Abhishek Arya 2010-06-17 23:16:44 PDT
credit:Kuzzcc
bug: http://code.google.com/p/chromium/issues/detail?id=46754
testcase (indented for better readability):
<html>
  <head>
    <style type="text/css">
        table
	{
            table-layout: fixed;
            width: 1px;
        }
    </style>
  </head>
  <body>
  <table> 
    <td width="30">ABC</td>
    <td colspan="65536">DEF</td>
  </table>
  </body>
</html>

span (int) get narrow casted to short in rendertable.cpp (since m_columns[pos].span is short), so 65536 becomes 0 (even though we want to have it greater than 1, see HTMLTableCellElement.cpp - if (attr->name() == colspanAttr) {
        m_colSpan = max(1, attr->value().toInt()); )

void RenderTable::appendColumn(int span)
{
    // easy case.
    int pos = m_columns.size();
    int newSize = pos + 1;
    m_columns.grow(newSize);
    m_columns[pos].span = span;

since autospan is 0, we get crash in fixedtablelayout.cpp.
            if (m_width[i].isAuto()) {
                int span = m_table->spanOfEffCol(i);
                int w = remainingWidth * span / autoSpan;

This bug is secseverity none for chromium (since it just crashes tab_ and low for safari (since it crashes browser completely). it should be an easy fix since span is used at int at all places, it does make sense to use it an unsigned int in declaration. unsigned part is kept in a good way to keep its value positive.
Comment 1 Abhishek Arya 2010-06-18 10:20:08 PDT
HTML5 spec also says to have larger values for colspan. so, going from short to int shouldn't be that bad. Also, conversions, one comparison between signed and unsigned int shouldn't be bad since initially we start with colspan b/w 1 and +INT_MAX (m_colSpan = max(1, attr->value().toInt()) and it stays within that range.

http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#htmltablecellelement

interface HTMLTableCellElement : HTMLElement {
           attribute unsigned long colSpan;
           attribute unsigned long rowSpan;
  [PutForwards=value] readonly attribute DOMSettableTokenList headers;
  readonly attribute long cellIndex;
};

I will file a seperate bug to cover some code cleanups, like converting all signed int use of span to unsigned int and remove the width attribute which is no longer used.
Comment 2 Abhishek Arya 2010-06-18 10:20:20 PDT
Created attachment 59129 [details]
Patch
Comment 3 Adam Barth 2010-06-18 10:24:16 PDT
Comment on attachment 59129 [details]
Patch

Interesting.  What's the memory impact of this change?WebCore/rendering/RenderTableSection.cpp:234
 +              if (cSpan < static_cast<int>(columns[m_cCol].span))
I don't quite understand why this cast is needed.  Presumably cSpan is a signed int.  Would it be better to make cSpan an unsigned?

WebCore/rendering/RenderTable.h:78
 +          unsigned int span;
Usually we just say "unsigned" when we mean "unsigned int"
Comment 4 Abhishek Arya 2010-06-18 10:33:06 PDT
Thanks a lot Adam for your fast review. 

WebCore/rendering/RenderTable.h:78
 +          unsigned int span;
Usually we just say "unsigned" when we mean "unsigned int"
---
Ok, i will remember this. i like the unsigned int version :)

Interesting.  What's the memory impact of this change?WebCore/rendering/RenderTableSection.cpp:234
 +              if (cSpan < static_cast<int>(columns[m_cCol].span))
I don't quite understand why this cast is needed.  Presumably cSpan is a signed int.  Would it be better to make cSpan an unsigned?
----
this is the only comparison b/w signed and unsigned int span and fails to compile on mac. as i said in comment #1, the cspan and like 30 more places all use span as signed int. The code needs to be cleaned up to change all spans to unsigned ints and non-used width attribute needs to be removed. for now, it is ok, since at input, we restrict it to 1 to +INT_MAX. Me or Cris Neckar might work on this cleanup.
Comment 5 Abhishek Arya 2010-06-18 10:57:06 PDT
Created attachment 59135 [details]
Patch
Comment 6 Adam Barth 2010-06-18 13:02:42 PDT
Comment on attachment 59135 [details]
Patch

Thanks.  We might want to ask jamesr or hyatt to make sure we're not using too much extra memory.
Comment 7 Abhishek Arya 2010-06-18 13:10:02 PDT
James, Dave, can you please take a look to see if it is ok to use extra memory for colspan in this patch.
Comment 8 Abhishek Arya 2010-06-18 14:34:35 PDT
I had a chat with Dave and he is fine with this change.
Comment 9 Adam Barth 2010-06-18 14:54:42 PDT
(In reply to comment #8)
> I had a chat with Dave and he is fine with this change.

Thanks for checking.  Looks like this is ready to land.
Comment 10 David Levin 2010-06-18 16:14:13 PDT
Committed as http://trac.webkit.org/changeset/61451
Comment 11 David Kilzer (:ddkilzer) 2010-07-01 15:22:25 PDT
<rdar://problem/8152839>
Comment 12 David Kilzer (:ddkilzer) 2010-07-01 15:24:47 PDT
Removing Security since <http://code.google.com/p/chromium/issues/detail?id=46754> is public and this is a divide-by-zero crash.