Closed Bug 1307557 Opened 8 years ago Closed 8 years ago

Out-of-bounds access in Element::DescribeAttribute()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

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)

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.
Flags: sec-bounty?
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)
Keywords: sec-low
Thanks for spotting this! I'll submit a fix.
Assignee: nobody → botond
Flags: needinfo?(botond)
(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.)
Summary: Out-of-bounds access in Elements::DescribeAttribute() → Out-of-bounds access in Element::DescribeAttribute()
Attached patch FixSplinter Review
Attachment #8798998 - Flags: review?(dveditz)
Blocks: 362937
Keywords: regression
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+
Group: core-security → dom-core-security
(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.
Do I need any additional approval to land the fix?
Flags: needinfo?(dveditz)
(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)
I landed this in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd71fa73ba0

(Didn't know pulsebot doesn't comment on these bugs)
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
Group: dom-core-security
Flags: sec-bounty? → sec-bounty-
Whiteboard: [adv-main52+]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: