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+

Abhishek Arya
Reported 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.
Attachments
Patch (4.45 KB, patch)
2010-06-18 10:20 PDT, Abhishek Arya 		no flags
DetailsFormatted DiffDiff
Patch (4.26 KB, patch)
2010-06-18 10:57 PDT, Abhishek Arya 		abarth: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Abhishek Arya
Comment 1 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.
Abhishek Arya
Comment 2 2010-06-18 10:20:20 PDT
Created attachment 59129 [details] Patch
Adam Barth
Comment 3 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"
Abhishek Arya
Comment 4 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.
Abhishek Arya
Comment 5 2010-06-18 10:57:06 PDT
Created attachment 59135 [details] Patch
Adam Barth
Comment 6 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.
Abhishek Arya
Comment 7 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.
Abhishek Arya
Comment 8 2010-06-18 14:34:35 PDT
I had a chat with Dave and he is fine with this change.
Adam Barth
Comment 9 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.
David Levin
Comment 10 2010-06-18 16:14:13 PDT
Committed as http://trac.webkit.org/changeset/61451
David Kilzer (:ddkilzer)
Comment 11 2010-07-01 15:22:25 PDT
<rdar://problem/8152839>
David Kilzer (:ddkilzer)
Comment 12 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.
Note You need to log in before you can comment on or make changes to this bug.