Closed Bug 1340577 Opened 7 years ago Closed 7 years ago

release-assert |begin| <= |end| when passed to Substring and similar APIs

Categories

(Core :: XPCOM, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 2 obsolete files)

Been discovered recently that constructing e.g. nsDependeCSubstring(begin, end) will happily accept begin > end.  That leads to overflow of the length property constructing a string that starts at an arbitrary position having a ~2^32 length and allowing consumers, probably even content, to access any memory after the |end| marker.

This probably doesn't happen over the code base, but better to add a proper hard-failing check we never pass begin > end to any APIs that accept it and may construct such broken strings.
Attached patch v1 (obsolete) — Splinter Review
All I could find in /xpcom/*String*.h

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9943fdadf45b8c383e0b6c77b2278d71d36fccc2
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8838641 - Flags: review?(nfroyd)
Comment on attachment 8838641 [details] [diff] [review]
v1

Review of attachment 8838641 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message probably needs to be tweaked, since the fix here is a little more general than just modifying Substring.

Some changes I think we should make are noted below.

::: xpcom/glue/nsStringAPI.h
@@ +1442,5 @@
>  
>  inline const nsDependentSubstring
>  Substring(const char16_t* aStart, const char16_t* aEnd)
>  {
> +  MOZ_RELEASE_ASSERT(aStart <= aEnd, "Overflow!");

If we're going to add release assertions to these Substring overloads, we should move them out of line into nsStringAPI.cpp.

::: xpcom/string/nsTDependentString.h
@@ +31,5 @@
>    nsTDependentString_CharT(const char_type* aStart, const char_type* aEnd)
>      : string_type(const_cast<char_type*>(aStart),
>                    uint32_t(aEnd - aStart), F_TERMINATED)
>    {
> +    MOZ_RELEASE_ASSERT(aStart <= aEnd, "Overflow!");

Likewise, these changes should go in nsTDependentString.cpp.

::: xpcom/string/nsTDependentSubstring.h
@@ +30,5 @@
>    void Rebind(const char_type* aData, size_type aLength);
>  
>    void Rebind(const char_type* aStart, const char_type* aEnd)
>    {
> +    MOZ_RELEASE_ASSERT(aStart <= aEnd, "Overflow!");

Likewise, these changes should go in nsTDependentSubstring.cpp.

::: xpcom/string/nsTSubstring.h
@@ +272,5 @@
>    }
>  
>    char_type First() const
>    {
> +    MOZ_RELEASE_ASSERT(mLength > 0, "|First()| called on an empty string");

Likewise, these functions should be out-of-lined.
Attachment #8838641 - Flags: review?(nfroyd) → review-
Depends on: 1340803
Attached patch v2 (obsolete) — Splinter Review
Everything carrying release asserts moved to respective cpps.

already on top of one found blocking bug:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e072bfb14d1d64c3f14ddfb8d003144868131a39
Attachment #8838641 - Attachment is obsolete: true
Attachment #8838871 - Flags: review?(nfroyd)
And I'll update the commit message before landing.
Comment on attachment 8838871 [details] [diff] [review]
v2

Review of attachment 8838871 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8838871 - Flags: review?(nfroyd) → review+
Attached patch v2 [ci message]Splinter Review
Attachment #8838871 - Attachment is obsolete: true
Attachment #8839628 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e552501cd1f
Add release-grade assertions to various XPCOM string API implementations to avoid input causing an overflow. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e552501cd1f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1346419
Depends on: 1362989
Depends on: 1363262
You need to log in before you can comment on or make changes to this bug.