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

Status: RESOLVED FIXED

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

Description

[q1]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Botond Ballo [:botond]]

Thanks for spotting this! I'll submit a fix.

Comment 3

[Botond Ballo [: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.)

Comment 4

[Botond Ballo [:botond]]

Attachment: Fix

Comment 5

[Daniel Veditz [:dveditz]]

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

Comment 6

[Botond Ballo [:botond]]

(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.

Comment 7

[Botond Ballo [:botond]]

Do I need any additional approval to land the fix?

Comment 8

[Andrew McCreight [:mccr8]]

(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.

Comment 9

[Botond Ballo [:botond]]

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

[Carsten Book [:Tomcat]]

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