Bug 40812 - Narrow cast for span leads to divide by zero and crash in fixed table layout
Summary: Narrow cast for span leads to divide by zero and crash in fixed table layout
Status: RESOLVED FIXED
Alias: None
Product: Security
Classification: Unclassified
Component: Security (show other bugs)
Version: Other
Hardware: All All
: P2 Minor
Assignee: WebKit Security Group
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-06-17 23:16 PDT by Abhishek Arya
Modified: 2012-05-10 08:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.45 KB, patch)
2010-06-18 10:20 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2010-06-18 10:57 PDT, Abhishek Arya
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.