Closed
Bug 1307557
Opened 8 years ago
Closed 8 years ago
Out-of-bounds access in Element::DescribeAttribute()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: q1, Assigned: botond)
References
Details
(Keywords: regression, sec-low, Whiteboard: [adv-main52+])
Attachments
(1 file)
1.12 KB,
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
Elements::DescribeAttribute() (dom\base\elements.cpp) reads one character beyond the end of an nsAutoString. Since nsAutoString is not necessarily null-terminated, DescribeAttribute() could erroneously insert a '\' character and/or cause a crash. In mitigation, this code appears to be used only for logging and debugging. The bug is in line 2816 [1], which uses |Length()| instead of |Length()-1|: 2806: void 2807: Element::DescribeAttribute(uint32_t index, nsAString& aOutDescription) const 2808: { 2809: // name 2810: mAttrsAndChildren.AttrNameAt(index)->GetQualifiedName(aOutDescription); 2811: 2812: // value 2813: aOutDescription.AppendLiteral("=\""); 2814: nsAutoString value; 2815: mAttrsAndChildren.AttrAt(index)->ToString(value); 2816: for (int i = value.Length(); i >= 0; --i) { 2817: if (value[i] == char16_t('"')) 2818: value.Insert(char16_t('\\'), uint32_t(i)); ... [1] As of trunk revision dfc1afe56b75.
Updated•8 years ago
|
Flags: sec-bounty?
Comment 1•8 years ago
|
||
The only active caller is in APZ code. the computed value is only used in debug output, but it looks like we call Element::Describe()--which calls DescribeElement--all the time.
Flags: needinfo?(botond)
Assignee | ||
Comment 2•8 years ago
|
||
Thanks for spotting this! I'll submit a fix.
Assignee: nobody → botond
Flags: needinfo?(botond)
Assignee | ||
Comment 3•8 years ago
|
||
(It looks like the out-of-bounds access has been around since 2006, added in bug 362937. My patch just moved it around a bit.)
Assignee | ||
Updated•8 years ago
|
Summary: Out-of-bounds access in Elements::DescribeAttribute() → Out-of-bounds access in Element::DescribeAttribute()
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8798998 -
Flags: review?(dveditz)
Updated•8 years ago
|
Blocks: 362937
Keywords: regression
Comment 5•8 years ago
|
||
Comment on attachment 8798998 [details] [diff] [review] Fix Review of attachment 8798998 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Element.cpp @@ +2814,5 @@ > nsAutoString value; > mAttrsAndChildren.AttrAt(index)->ToString(value); > + for (uint32_t i = value.Length(); i > 0; --i) { > + if (value[i - 1] == char16_t('"')) > + value.Insert(char16_t('\\'), i - 1); Seems odd--and possibly error prone in the future--to have to make 3 changes instead of just starting the index with Length()-1. But that's a style issue I guess. r=dveditz
Attachment #8798998 -
Flags: review?(dveditz) → review+
Updated•8 years ago
|
Group: core-security → dom-core-security
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #5) > ::: dom/base/Element.cpp > @@ +2814,5 @@ > > nsAutoString value; > > mAttrsAndChildren.AttrAt(index)->ToString(value); > > + for (uint32_t i = value.Length(); i > 0; --i) { > > + if (value[i - 1] == char16_t('"')) > > + value.Insert(char16_t('\\'), i - 1); > > Seems odd--and possibly error prone in the future--to have to make 3 changes > instead of just starting the index with Length()-1. But that's a style issue > I guess. Length() has type uint32_t. If the length is zero, |Length() - 1| will underflow to give you |2^32 - 1|. The loop index type, prior to my change, was int. On a platform where int is a 32-bit type, that's fine, the |2^32 - 1| will convert to -1. However, on a platform where int is a 64-bit type, we're in trouble. I could have avoided this by changing the loop index type to be int32_t, but since I was changing it anyways, I thought it was best to avoid type conversions altogether and keep everything as a uint32_t.
Assignee | ||
Comment 7•8 years ago
|
||
Do I need any additional approval to land the fix?
Flags: needinfo?(dveditz)
Comment 8•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #7) > Do I need any additional approval to land the fix? No, because this is not sec-high or sec-critical.
Flags: needinfo?(dveditz)
Assignee | ||
Comment 9•8 years ago
|
||
I landed this in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd71fa73ba0 (Didn't know pulsebot doesn't comment on these bugs)
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cd71fa73ba0(In reply to Botond Ballo [:botond] from comment #9) > I landed this in > https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd71fa73ba0 > > (Didn't know pulsebot doesn't comment on these bugs) yeah pulsebot doen't have security rights to access/comment on sec-bugs
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Group: dom-core-security
Flags: sec-bounty? → sec-bounty-
Updated•7 years ago
|
Whiteboard: [adv-main52+]
You need to log in
before you can comment on or make changes to this bug.
Description
•