PoolObject::parseMultiname and Verifier::checkTypeName can recurse arbitrarily deep

VERIFIED FIXED in flash10.1

Status

P2
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: stejohns, Assigned: stejohns)

Tracking

unspecified
flash10.1
Dependency tree / graph
Bug Flags:
in-testsuite ?
flashplayer-qrb +

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
When parsing parameterized types, the code recurses as deep as the level of paramterizing. A carefully constructed ABC could make use of this to force a stack overflow.
(Assignee)

Updated

9 years ago
Assignee: nobody → stejohns
(Assignee)

Comment 1

9 years ago
Correction: with the proposed patch for bug 556543, PoolObject::parseMultiname can't recurse arbitrarily deep, but Verifier::checkTypeName (and Interpreter::getTraits) can.
(Assignee)

Comment 2

9 years ago
PoolObject::resolveTypeName is also susceptible.
(Assignee)

Comment 3

9 years ago
I've fiddled around with various rewrite attempts to unroll the recursion, but it's awkward and feels a bit too fiddly for this stage of the game, so I'm tempted to just insert a call to stackCheck() before the recursive bits. Thoughts?
(Assignee)

Comment 4

9 years ago
Created attachment 436800 [details] [diff] [review]
Patch

Minimal fix that merely inserts calls to stackCheck in the possible recursion cases.
Attachment #436800 - Flags: review?(edwsmith)

Updated

9 years ago
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1

Updated

9 years ago
Blocks: 502589

Updated

9 years ago
Attachment #436800 - Flags: review?(edwsmith) → review+

Comment 5

9 years ago
Comment on attachment 436800 [details] [diff] [review]
Patch

patch is okay as long as the bug has been vetted for exploitability.  the fuzzer bugs were marked as security by default, but get declassified unless the represent a real exploit in shipping code.  

all the other stack overflow checking bugs for example (see 504506) are not marked as security.

Comment 6

9 years ago
Declassify.
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Group: tamarin-security
(Assignee)

Comment 7

9 years ago
http://hg.mozilla.org/tamarin-redux/rev/2026a6bc0146
http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/2026a6bc0146
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 8

9 years ago
Regression media depends on abcasm multiname support being added: See Bug 558956
Depends on: 558956

Comment 9

9 years ago
(In reply to comment #8)
> Regression media depends on abcasm multiname support being added: See Bug
> 558956

Looks like abcasm needs TypeName and a way to create a qualified QName.  This also interacts with another bug in the parser -- abcasm also needs what amount to soft statement breaks at line breaks.

Updated

9 years ago
QA Contact: vm → cpeyer

Updated

9 years ago
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.