bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Stack buffer overflow in content/media/webm/EbmlComposer.cpp

RESOLVED DUPLICATE of bug 966044

Status

()

Core
Audio/Video
RESOLVED DUPLICATE of bug 966044
5 years ago
2 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

({sec-low})

unspecified
x86_64
Linux
sec-low
Points:
---

Firefox Tracking Flags

(firefox27 unaffected, firefox28 unaffected, firefox29 affected, firefox-esr24 unaffected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
15:07.96 In file included from /usr/include/string.h:638:0,
15:07.96                  from ../../../dist/system_wrappers/string.h:3,
15:07.96                  from ../../../dist/include/nsTArray.h:16,
15:07.96                  from /home/padenot/src/trees/mozilla-inbound/content/media/webm/EbmlComposer.h:8,
15:07.96                  from /home/padenot/src/trees/mozilla-inbound/content/media/webm/EbmlComposer.cpp:6,
15:07.96                  from /home/padenot/src/trees/mozilla-inbound/obj-x86_64-unknown-linux-gnu/content/media/webm/Unified_cpp_content_media_webm0.cpp:2:
15:07.96 In function ‘char* strcpy(char*, const char*)’,
15:07.96     inlined from ‘void mozilla::EbmlComposer::GenerateHeader()’ at /home/padenot/src/trees/mozilla-inbound/content/media/webm/EbmlComposer.cpp:50:43:
15:07.96 /usr/include/x86_64-linux-gnu/bits/string3.h:104:86: error: call to char* __builtin___strcpy_chk(char*, const char*, long unsigned int) will always overflow destination buffer [-Werror]

"A_VORBIS" is 9 char long (because of the \0). Maybe we don't want the '\0' in the container, in which case we can just put a "- 1" on the size.
(Assignee)

Comment 1

5 years ago
Created attachment 8366634 [details] [diff] [review]
length-fix
(Assignee)

Updated

5 years ago
Attachment #8366634 - Flags: review?(giles)
(Assignee)

Comment 2

5 years ago
Created attachment 8366635 [details] [diff] [review]
possible patch

gahh, forgot to qref.
Attachment #8366634 - Attachment is obsolete: true
Attachment #8366634 - Flags: review?(giles)
Attachment #8366635 - Flags: review?(giles)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 964761
Comment on attachment 8366635 [details] [diff] [review]
possible patch

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

Whoops, thanks for the fix.

The spec is unclear whether the codec_id field should have a terminating null or not; it seems to be optional. However the libmkv writer we're using depends on being able to call strlen() on the string we pass in (and then strips it before writing) so we need 9 bytes here.
Attachment #8366635 - Flags: review?(giles) → review+
(Assignee)

Comment 5

5 years ago
Comment on attachment 8366635 [details] [diff] [review]
possible patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
No clue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The problem is obvious. Merely compiling the code shows the issue if enough warning flags are enabled.

Which older supported branches are affected by this flaw?
Nightly only.

If not all supported branches, which bug introduced the flaw?
Bug 891705

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
N/A, nightly only.

How likely is this patch to cause regressions; how much testing does it need?
This is super obvious.
Attachment #8366635 - Flags: sec-approval?

Comment 6

5 years ago
If this is trunk only, it doesn't need sec-approval+ to go in.

That said, it would be good to get a security rating on this.

Updated

5 years ago
status-firefox27: --- → ?
status-firefox28: --- → ?
status-firefox29: --- → affected
status-firefox-esr24: --- → ?
(Assignee)

Updated

5 years ago
status-firefox27: ? → unaffected
status-firefox28: ? → unaffected
status-firefox-esr24: ? → unaffected
This has not been re-fixed in public bug 966044, so I think we can grant sec-approval and open this bug.

Comment 8

5 years ago
Can we use static_assert and MOZ_ARRAY_LENGTH to get a compile time assertion that cid_string is big enough?
I looked at that, but it we'd have to promote the strings to constants so we could reference them more than once. That would be fine but I like efaust's fix of just passing string constants directly better, no problematic allocation needed.
As far as rating goes, I think it's sec-low. The overflow is one byte past the last thing allocated on the stack. Assuming the compiler doesn't rearrange any variables that shouldn't corrupt anything, just crash if we overflow the stack.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 966044

Comment 12

5 years ago
Since this is a sec-low, it doesn't need approval (that only applies to higher rated issues, see https://wiki.mozilla.org/Security/Bug_Approval_Process). 

I'll clear the request. I'm glad it is fixed though!
Keywords: sec-low

Updated

5 years ago
Attachment #8366635 - Flags: sec-approval?

Updated

5 years ago
No longer blocks: 891705

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.